-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate API and add NFW telemetry for it #70365
Deprecate API and add NFW telemetry for it #70365
Conversation
eba7ef1
to
725ba83
Compare
@arkalyanms @phil-allen-msft for QB. |
@@ -194,6 +196,8 @@ public static SyntaxNode DeserializeFrom(Stream stream, CancellationToken cancel | |||
throw new InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeReadFrom); | |||
} | |||
|
|||
// Report NFW to see if this is being used in the wild. | |||
FatalError.ReportAndPropagate(new SerializationDeprecationException()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonmalinowski does this look correct wrt to reporting a good NFW we can trak on our side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use ReportAndCatch; as discussed it doesn't change anything beyond the return code (if it were being used in an exception filter), but this as written might make people think this will actually throw this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not available at this layer :)
@@ -13,6 +13,8 @@ | |||
using Roslyn.Test.Utilities; | |||
using Xunit; | |||
|
|||
#pragma warning disable CS0618 // Type or member is obsolete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonmalinowski the presumption here is that we do not actual send NFWs for compiler unit tests.
725ba83
to
1aa6da4
Compare
1aa6da4
to
1bba049
Compare
@@ -1403,10 +1405,26 @@ public virtual void SerializeTo(Stream stream, CancellationToken cancellationTok | |||
throw new InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeWrittenTo); | |||
} | |||
|
|||
// Report NFW to see if this is being used in the wild. | |||
FatalError.ReportAndPropagate(new SerializationDeprecationException()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as other place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the check that is causing the MVID test to fo fail.
@@ -13,6 +13,8 @@ | |||
using Microsoft.CodeAnalysis.CSharp.Syntax; | |||
using Microsoft.CodeAnalysis.Text; | |||
|
|||
#pragma warning disable CS0618 // Type or member is obsolete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these you could also the class itself as [Obsolete] which might make it clearer this obsolete in it's own right.
@jaredpar ptal. |
dadd680
to
2489912
Compare
2489912
to
50f4112
Compare
@dotnet/roslyn-compiler for a second pair of eyes. @arkalyanms @phil-allen-msft as a reminder to take this for QB. |
Ready for official submission when PR merges. Tracked via https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1903056 |
@arkalyanms integration tests seem to be the same ones we're having issues with everywhere. |
@333fred could you potentially review this? Since you're aware of hte API review around it? Thanks! |
In 17.9 we'd like to remove the logic to serialize syntax nodes to streams (see #70275 for more details on this and why it should be safe).
However, to build confidence that this will not be an issue, we're pre-emptively deprecating the API (but still keeping the logic for it) in 17.8 and also including an NFW so we can see if this gets hit in the wild for cases we don't know about.
API change was approved in API review.