Skip to content
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

Merged
merged 3 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand Down Expand Up @@ -182,6 +183,7 @@ public new SyntaxTriviaList GetTrailingTrivia()
/// <summary>
/// Deserialize a syntax node from the byte stream.
/// </summary>
[Obsolete(SerializationDeprecationException.Text, error: false)]
public static SyntaxNode DeserializeFrom(Stream stream, CancellationToken cancellationToken = default)
{
if (stream == null)
Expand All @@ -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());
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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 :)

using var reader = ObjectReader.TryGetReader(stream, leaveOpen: true, cancellationToken);

if (reader == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
using Roslyn.Test.Utilities;
using Xunit;

#pragma warning disable CS0618 // Type or member is obsolete
Copy link
Member Author

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.


namespace Microsoft.CodeAnalysis.CSharp.UnitTests
{
public class SerializationTests
Expand Down Expand Up @@ -305,3 +307,5 @@ public class C
}
}
}

#pragma warning restore CS0618 // Type or member is obsolete
18 changes: 18 additions & 0 deletions src/Compilers/Core/Portable/Syntax/SyntaxNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Linq;
using System.Text;
using System.Threading;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

Expand Down Expand Up @@ -1391,6 +1392,7 @@ public bool IsEquivalentTo(SyntaxNode node, bool topLevel = false)
/// Serializes the node to the given <paramref name="stream"/>.
/// Leaves the <paramref name="stream"/> open for further writes.
/// </summary>
[Obsolete(SerializationDeprecationException.Text, error: false)]
public virtual void SerializeTo(Stream stream, CancellationToken cancellationToken = default)
{
if (stream == null)
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as other place.

Copy link
Member

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.

using var writer = new ObjectWriter(stream, leaveOpen: true, cancellationToken);
writer.WriteValue(Green);
}

/// <summary>
/// Specialized exception subtype to make it easier to search telemetry streams for this specific case.
/// </summary>
private protected sealed class SerializationDeprecationException : Exception
{
public const string Text = "Syntax serialization support is deprecated and will be removed in a future version of this API";

public SerializationDeprecationException()
: base(Text)
{

}
}

#region Core Methods

/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/Test/Utilities/CSharp/CSharpTestSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,15 @@ private CSharpTestSource(object value)

private static void CheckSerializable(SyntaxTree tree)
{
#pragma warning disable CS0618 // Type or member is obsolete
using var stream = new MemoryStream();
var root = tree.GetRoot();
root.SerializeTo(stream);
stream.Position = 0;

// verify absence of exception:
_ = CSharpSyntaxNode.DeserializeFrom(stream);
#pragma warning restore CS0618 // Type or member is obsolete
}

public SyntaxTree[] GetSyntaxTrees(CSharpParseOptions parseOptions, string sourceFileName = "")
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/Test/Utilities/VisualBasic/BasicTestSource.vb
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ Public Structure BasicTestSource
End Function

Private Shared Sub CheckSerializable(tree As SyntaxTree)
#Disable Warning BC40000 ' Type or member is obsolete
Using stream = New MemoryStream()
Dim root = tree.GetRoot()
root.SerializeTo(stream)
stream.Position = 0

' verify absence of exception
VisualBasicSyntaxNode.DeserializeFrom(stream)
#Enable Warning BC40000 ' Type or member is obsolete
End Using
End Sub

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Imports System.Collections.ObjectModel
Imports System.ComponentModel
Imports System.Reflection
Imports System.Threading
Imports Microsoft.CodeAnalysis.ErrorReporting
Imports Microsoft.CodeAnalysis.PooledObjects
Imports Microsoft.CodeAnalysis.Text
Imports Microsoft.CodeAnalysis.VisualBasic.Symbols
Expand Down Expand Up @@ -125,6 +126,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
''' <summary>
''' Deserialize a syntax node from a byte stream.
''' </summary>
<Obsolete(SerializationDeprecationException.Text, False)>
Public Shared Function DeserializeFrom(stream As IO.Stream, Optional cancellationToken As CancellationToken = Nothing) As SyntaxNode
If stream Is Nothing Then
Throw New ArgumentNullException(NameOf(stream))
Expand All @@ -134,6 +136,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
Throw New InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeReadFrom)
End If

' Report NFW to see if this Is being used in the wild.
FatalError.ReportAndPropagate(New SerializationDeprecationException())

Using reader = ObjectReader.TryGetReader(stream, leaveOpen:=True, cancellationToken:=cancellationToken)
If reader Is Nothing Then
Throw New ArgumentException(CodeAnalysisResources.Stream_contains_invalid_data, NameOf(stream))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
Imports Roslyn.Test.Utilities
Imports Xunit

Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests
#Disable Warning BC40000 ' Type or member is obsolete

Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests
Public Class SerializationTests

Private Sub RoundTrip(text As String, Optional expectRecursive As Boolean = True)
Expand Down Expand Up @@ -461,3 +462,5 @@ End Module
End Sub
End Class
End Namespace

#Enable Warning BC40000 ' Type or member is obsolete
4 changes: 4 additions & 0 deletions src/Tools/IdeCoreBenchmarks/SerializationBenchmarks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

#pragma warning disable CS0618 // Type or member is obsolete
Copy link
Member

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.


namespace IdeCoreBenchmarks
{
[MemoryDiagnoser]
Expand Down Expand Up @@ -96,3 +98,5 @@ public void DeserializeSyntaxNode()
}
}
}

#pragma warning restore CS0618 // Type or member is obsolete
4 changes: 4 additions & 0 deletions src/Workspaces/CoreTest/SerializationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ public void VersionStamp_RoundTripText()
Assert.Equal(versionStamp, deserializedVersionStamp);
}

#pragma warning disable CS0618 // Type or member is obsolete

private static void TestSymbolSerialization(Document document, string symbolName)
{
var model = document.GetSemanticModelAsync().Result;
Expand Down Expand Up @@ -91,5 +93,7 @@ private static void TestSymbolSerialization(Document document, string symbolName

Assert.True(id.Equals(did));
}

#pragma warning restore CS0618 // Type or member is obsolete
}
}