Add NullableAnnotation/WithNullableAnnotation APIs to ITypeSymbol. by AlekseyTs · Pull Request #39498 · dotnet/roslyn

AlekseyTs · 2019-10-24T16:48:42Z

This change also separates implementation of ISymbol interfaces from internal symbols in C# compiler.

Perf results from this change #39498 (comment).

AlekseyTs · 2019-10-24T16:54:14Z

It looks like CodeFlow doesn't show all files. Sorry for the inconvenience. #Resolved

AlekseyTs · 2019-10-24T16:55:44Z

src/Workspaces/CSharpTest/CodeGeneration/AddImportsTests.cs

I will open an issue to follow-up on this baseline adjustment.

Please disable the test and point to the tracking issue; the IDE policy is to disable tests rather than assert the "wrong" thing.

AlekseyTs · 2019-10-24T16:58:35Z

src/Workspaces/Core/Portable/Shared/Utilities/SymbolEquivalenceComparer.EquivalenceVisitor.cs

I will open an issue to follow-up on this.

Workaround seems most reasonable for now.

This change also separates implementation of ISymbol interfaces from internal symbols in C# compiler.

AlekseyTs · 2019-10-24T21:27:45Z

src/Workspaces/Core/Portable/Shared/Utilities/SymbolEquivalenceComparer.EquivalenceVisitor.cs

                    throw new InvalidOperationException("Stack too deep.");
                }
 #endif
+                // This is a temporary workaround sufficient to get existing tests passing. This component should be 


// This is a temporary workaround sufficient to get existing tests passing. This component should be [](start = 16, length = 101)

I will open an issue to follow-up on this. #Resolved

AlekseyTs · 2019-10-24T21:31:33Z

I re-based the PR to separate test changes into the second commit. Reviewing the first commit in CodeFlow will include all product changes now. #Resolved

cston · 2019-10-24T22:04:42Z

src/Compilers/Core/Portable/Symbols/ISymbolInternal.cs

+        bool Equals(ISymbolInternal? other, TypeCompareKind compareKind);
+
+        /// <summary>
+        /// Gets the <see cref="ISymbol"/> for the immediately containing symbol.


ISymbol [](start = 32, length = 7)

ISymbolInternal #Resolved

src/Compilers/CSharp/Portable/Symbols/PublicModel/Symbol.cs

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

src/Compilers/CSharp/Portable/Symbols/PublicModel/Symbol.cs

src/Compilers/CSharp/Portable/Symbols/PublicModel/DiscardSymbol.cs

src/Compilers/CSharp/Portable/Symbols/PublicModel/ErrorTypeSymbol.cs

src/Compilers/CSharp/Portable/Symbols/PublicModel/NamedTypeSymbol.cs

tmat · 2019-10-30T00:15:43Z

src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs

                {
                    // Don't allocate until we see at least one interface.
-                    seen = new HashSet<NamedTypeSymbol>(TypeSymbol.EqualsCLRSignatureComparer);
+                    seen = new HashSet<NamedTypeSymbol>(Symbols.SymbolEqualityComparer.CLRSignature);


CLRSignature [](start = 87, length = 12)

nit: ClrSignature #WontFix

tmat · 2019-10-30T00:23:04Z

src/Compilers/CSharp/Test/Emit/PDB/PDBTests.cs

  <entryPoint declaringType=""C"" methodName=""F"" />
  <methods/>
-</symbols>", debugEntryPoint: f, options: PdbValidationOptions.ExcludeScopes | PdbValidationOptions.ExcludeSequencePoints | PdbValidationOptions.ExcludeCustomDebugInformation);
+</symbols>", debugEntryPoint: f.GetPublicSymbol(), options: PdbValidationOptions.ExcludeScopes | PdbValidationOptions.ExcludeSequencePoints | PdbValidationOptions.ExcludeCustomDebugInformation);


.GetPublicSymbol() [](start = 31, length = 18)

Call to GetPublicSymbol() is repeated a lot. Would it make sense for GetMember<T> helper to return the public symbol directly, or do we have many cases that need the internal symbol? If the latter, then I'd prefer calling GetPublicSymbol() in the declaration of f to avoid repetition. #WontFix

Call to GetPublicSymbol() is repeated a lot. Would it make sense for GetMember helper to return the public symbol directly, or do we have many cases that need the internal symbol? If the latter, then I'd prefer calling GetPublicSymbol() in the declaration of f to avoid repetition.

I don't think we want GetMember<T> helper to return the public symbol directly when called on an internal symbol. Also, in this file there are only two call-sites of VerifyPdb that use the helper.

In reply to: 340385499 [](ancestors = 340385499)

tmat · 2019-10-30T00:24:14Z

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/LocalSlotMappingTests.cs

            var diff1 = compilation1.EmitDifference(
                generation0,
-                ImmutableArray.Create(new SemanticEdit(SemanticEditKind.Update, method0, method1, syntaxMap: null, preserveLocalVariables: true)));
+                ImmutableArray.Create(SemanticEdit.Create(SemanticEditKind.Update, method0, method1, syntaxMap: null, preserveLocalVariables: true)));


SemanticEdit.Create [](start = 38, length = 19)

Would we need SemanticEdit.Create if GetMember<T> helper returned a public symbol? #WontFix

Would we need SemanticEdit.Create if GetMember helper returned a public symbol?

GetMember<MethodSymbol> cannot return IMethodSymbol without changes at the call site. In general, I do not think it is desirable for GetMember to return a public symbol when called on an internal symbol.

In reply to: 340385708 [](ancestors = 340385708)

tmat · 2019-10-30T00:27:46Z

src/Compilers/CSharp/Portable/Symbols/Symbol.cs

-    internal abstract partial class Symbol : ISymbol, ISymbolInternal, IFormattable
+    internal abstract partial class Symbol : ISymbolInternal, IFormattable
    {
+        private ISymbol _lazyISymbol;


_lazyISymbol [](start = 24, length = 12)

_lazyPublicSymbol? #WontFix

tmat · 2019-10-30T00:28:10Z

src/Compilers/CSharp/Portable/Symbols/Symbol.cs

+            }
+        }
+
+        ISymbol ISymbolInternal.GetISymbol() => this.ISymbol;


GetISymbol [](start = 32, length = 10)

GetPublicSymbol? #WontFix

tmat · 2019-10-30T00:28:18Z

src/Compilers/CSharp/Portable/Symbols/Symbol.cs

+            }
+        }
+
+        ISymbol ISymbolInternal.GetISymbol() => this.ISymbol;


ISymbol [](start = 53, length = 7)

PublicSymbol? #WontFix

tmat · 2019-10-30T00:28:52Z

src/Compilers/CSharp/Portable/Symbols/Symbol.cs

-            {
-                throw new ArgumentException(nameof(typeArguments));
-            }
+        protected abstract ISymbol CreateISymbol();


CreateISymbol [](start = 35, length = 13)

CreatePublicSymbol? #WontFix

tmat · 2019-10-30T00:32:33Z

Reviewed iteration 6. #Resolved

AlekseyTs · 2019-10-30T21:17:14Z

@jasonmalinowski, @ryzngard Please provide feedback at least on the new public API on ITypeSymbol and on changes on IDE side. Thanks. #Resolved

jasonmalinowski · 2019-10-30T21:39:28Z

@AlekseyTs Looking at it in between meetings today, so will be done in a bit. #Resolved

CyrusNajmabadi · 2019-10-30T21:45:49Z

src/Compilers/Core/Portable/Symbols/ITypeSymbol.cs

+        /// <summary>
+        /// Nullable annotation associated with the type, or <see cref="NullableAnnotation.None"/> if there are none.
+        /// </summary>
+        NullableAnnotation NullableAnnotation { get; }


amazing! (seriously). awesome job!

just to check. so there's no way to go from an ITypeSymbol to the nullability it was 'declared' with right? @333fred @jasonmalinowski is htat ok? is that something a feature needs? what's the expectation on how they would get it?

i think but am not certain, that you should be ok with semantic model depending on what semantic model returns for ops. For example, what does GetTypeInfo return? does it return both "DeclaredType" and "Type"? If so, great, if not... what is end consumption code expected to do?

Same with GetSymbolInfo does it return the symbol (say a field symbol) with teh current nullability at the current location, or does it return the symbol as it was declared?

I'm just trying to wrap my head around how someone is expected to consume things and what patterns will be more 'correct' and what will likely result in incorrect data/decision being made.

Thanks!

GetTypeInfo returns the current state, and GetSymbolInfo returns the declared information. The idea is that consumers will need to figure out what they need to know, and call the appropriate API, much like the differences between those APIs today.

Interesting. Note: i personally think that GetTypeInfo should be updated to include 'declared' info as well. i think it's asking a lot to have to make people jump through these hoops.

In particular, GetSymbolInfo is not easy to use. And hte relationship between the two and when/how people can expect the latter to work is not easy to understand and internalize.

So just my 2c that it woudl be nice to have the api dedicated to understanding types at the semantic level be updated to understand this totally natural view of what a type is.

Thanks! :)

So when we moved the IDE to our existing wrappers, we didn't really run into this as a problem. We might have to switch a few places to now call GetSymbolInfo, but they're going to be places like where we're checking if we can assign something safely, and those could have (and perhaps should have) been using GetSymbolInfo.

(If anything, we "discovered" a few cases where GetTypeInfo worked that looked questionable to me in the first place, and switched the IDE to GetSymbolInfo for safety no matter what!)

CyrusNajmabadi · 2019-10-30T21:49:37Z

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationTypeSymbol.cs

+        ITypeSymbol ITypeSymbol.WithNullableAnnotation(NullableAnnotation nullableAnnotation)
+        {
+            throw new System.NotImplementedException();
+        }


@jasonmalinowski presumably this will be easy to support on the IDE side.

CyrusNajmabadi · 2019-10-30T21:51:16Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

@@ -9,6 +9,10 @@ Microsoft.CodeAnalysis.Operations.VariableDeclarationKind.AsynchronousUsing = 2
 Microsoft.CodeAnalysis.Operations.VariableDeclarationKind.Default = 0 -> Microsoft.CodeAnalysis.Operations.VariableDeclarationKind
 Microsoft.CodeAnalysis.Operations.VariableDeclarationKind.Using = 1 -> Microsoft.CodeAnalysis.Operations.VariableDeclarationKind
 Microsoft.CodeAnalysis.Operations.IVariableDeclarationGroupOperation.DeclarationKind.get -> Microsoft.CodeAnalysis.Operations.VariableDeclarationKind
+Microsoft.CodeAnalysis.ITypeSymbol.NullableAnnotation.get -> Microsoft.CodeAnalysis.NullableAnnotation
+Microsoft.CodeAnalysis.ITypeSymbol.WithNullableAnnotation(Microsoft.CodeAnalysis.NullableAnnotation nullableAnnotation) -> Microsoft.CodeAnalysis.ITypeSymbol


amazing that this is the only public change :) i'm glad we have this file as it helped me know that there wasn't really anything else i needed to consider. #Resolved

CyrusNajmabadi · 2019-10-30T21:52:03Z

@jasonmalinowski do you intend to make a followup PR that moves the IDE onto this api first? i.e. are you going to prove this out before this gets checked in? or will this be checked in, and then you'll adopt at your own pace? Thanks! #Resolved

jasonmalinowski

Reviewed everything under src/EditorFeatures, src/Workspaces, any of the shared test utilities, and glanced at some of the "interesting" compiler bits.

Overall approach looks like a solid approach; I do worry that this will allocate even more than the current IDE wrapping, since this will be wrapping all symbols and not just types. Specifically, that may impact some features like classification. We should do an RPS run on this prior to merge to ensure we aren't regressing things there, because the existing IDE data where type-symbol-only wrapping didn't cause problems isn't quite applicable here.

The oddity about VB symbols behaving differently is something we'll have to keep in mind if we ever see that coming up. There are cases where it would matter, but they are pretty niche scenarios, so it may not be something worth addressing until we have a compelling need.

The changes in the IDE area seem good and the workaround added seems fine. Once you have the tracking items made let us know so we can fix up some of those quickly.

jasonmalinowski · 2019-10-30T20:16:32Z

src/Workspaces/Core/Portable/Utilities/NullableHelpers/TypeSymbolWithNullableAnnotation.cs

@@ -203,6 +203,13 @@ public string ToMinimalDisplayString(SemanticModel semanticModel, int position,
            }

            #endregion
+
+            NullableAnnotation ITypeSymbol.NullableAnnotation => this.Nullability;


Did we want to put this change into a branch so we can also do the IDE cleanup at the same time? Otherwise we're making wrappers around wrappers.

Did we want to put this change into a branch so we can also do the IDE cleanup at the same time? Otherwise we're making wrappers around wrappers.

Merging this change into a feature branch is an option. However, this change is quite big and is likely to encounter many merge conflicts and build/tests breaks due to changes in the relationship between types and interfaces they implement if we keep it on the side for a long time. I experienced this while working on this change. Merging with mater was causing all kinds of breaks.

jasonmalinowski · 2019-10-30T20:21:00Z

src/Workspaces/CoreTest/ReferencedSymbolTests.cs

@@ -304,7 +304,7 @@ public bool Equals(ISymbol other)

            public bool Equals(ISymbol other, SymbolEqualityComparer equalityComparer)


@CyrusNajmabadi @sharwell Any idea what these tests are doing? Any reason we can't either be using "real symbols" or at least our code generation symbols to avoid yet another implementation like this?

(this question doesn't need to be answered before this merges)

I literally have no idea what this test is trying to accomplish. seems nigh entirely useless.

jasonmalinowski · 2019-10-30T20:26:02Z

src/Workspaces/CSharpTest/CodeGeneration/AddImportsTests.cs

Please disable the test and point to the tracking issue; the IDE policy is to disable tests rather than assert the "wrong" thing.

jasonmalinowski · 2019-10-30T20:27:55Z

src/EditorFeatures/Test/Utilities/SymbolEquivalenceComparerTests.cs

@@ -1299,12 +1299,12 @@ public void AssemblyComparer1()
            var sourceV1 = "[assembly: System.Reflection.AssemblyVersion(\"1.0.0.0\")] public class T {}";
            var sourceV2 = "[assembly: System.Reflection.AssemblyVersion(\"2.0.0.0\")] public class T {}";

-            var a1 = CS.CSharpCompilation.Create("a", new[] { CS.SyntaxFactory.ParseSyntaxTree(source) }, references, CSharpDllOptions);
-            var a2 = CS.CSharpCompilation.Create("a", new[] { CS.SyntaxFactory.ParseSyntaxTree(source) }, references, CSharpDllOptions);
+            var a1 = (Compilation)CS.CSharpCompilation.Create("a", new[] { CS.SyntaxFactory.ParseSyntaxTree(source) }, references, CSharpDllOptions);


Why the cast to (Compilation) now?

Why the cast to (Compilation) now?

Getting to symbols through CSharpCompilation gives back internal symbols, they do not implement public APIs directly.

jasonmalinowski · 2019-10-30T20:34:16Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

@@ -9,6 +9,10 @@ Microsoft.CodeAnalysis.Operations.VariableDeclarationKind.AsynchronousUsing = 2
 Microsoft.CodeAnalysis.Operations.VariableDeclarationKind.Default = 0 -> Microsoft.CodeAnalysis.Operations.VariableDeclarationKind
 Microsoft.CodeAnalysis.Operations.VariableDeclarationKind.Using = 1 -> Microsoft.CodeAnalysis.Operations.VariableDeclarationKind
 Microsoft.CodeAnalysis.Operations.IVariableDeclarationGroupOperation.DeclarationKind.get -> Microsoft.CodeAnalysis.Operations.VariableDeclarationKind
+Microsoft.CodeAnalysis.ITypeSymbol.NullableAnnotation.get -> Microsoft.CodeAnalysis.NullableAnnotation
+Microsoft.CodeAnalysis.ITypeSymbol.WithNullableAnnotation(Microsoft.CodeAnalysis.NullableAnnotation nullableAnnotation) -> Microsoft.CodeAnalysis.ITypeSymbol
+*REMOVED*virtual Microsoft.CodeAnalysis.Location.MetadataModule.get -> Microsoft.CodeAnalysis.IModuleSymbol


No concerns about this being a breaking change? It looks fine to me given Location was already not derivable by public callers, but just wanted to confirm. #Resolved

No concerns about this being a breaking change? It looks fine to me given Location was already not derivable by public callers, but just wanted to confirm.

The type has internal constructor, no one but us could override the property. I think that removing virtual-ness shouldn't break anyone else. #Resolved

jasonmalinowski · 2019-10-30T20:40:37Z

src/EditorFeatures/CSharpTest/SymbolId/SymbolKeyCompilationsTests.cs

@@ -119,10 +119,10 @@ public partial class C1
 }
 ";

-            var comp = CreateCompilation(src, assemblyName: "Test");
+            var comp = (Compilation)CreateCompilation(src, assemblyName: "Test");


Why all the casts to Compilation?

Why all the casts to Compilation?

Same response as for the same question above.

jasonmalinowski · 2019-10-30T20:43:23Z

src/EditorFeatures/CSharpTest/SymbolId/SymbolKeyTestBase.cs

@@ -195,13 +195,14 @@ internal static IList<ISymbol> GetSourceSymbols(CSharpCompilation compilation, b
        {
            var list = new List<ISymbol>();
            var localDumper = includeLocal ? new LocalSymbolDumper(compilation) : null;
-            GetSourceMemberSymbols(compilation.SourceModule.GlobalNamespace, list, localDumper);
+            GetSourceMemberSymbols(compilation.SourceModule.GlobalNamespace.GetPublicSymbol(), list, localDumper);


I was unaware we had tests using compiler internal APIs like this -- we should clean this up, but not part of this PR. If we're not already aware of a bug tracking removal of the IVT, could you file one? You're bringing this pretty close to be being clean, so that's great progress.

If we're not already aware of a bug tracking removal of the IVT, could you file one?

I think it would be best if you open that issue because you have a better idea what is it exactly you would want to be addressed, hence will be able to describe it much more clear. Etc.

jasonmalinowski · 2019-10-30T20:45:27Z

src/Workspaces/Core/Portable/Shared/Utilities/SymbolEquivalenceComparer.EquivalenceVisitor.cs

Workaround seems most reasonable for now.

jasonmalinowski · 2019-10-30T22:59:35Z

src/Compilers/VisualBasic/Portable/Symbols/TypeSymbol.vb

+        End Property
+
+        Private Function ITypeSymbol_WithNullability(nullableAnnotation As NullableAnnotation) As ITypeSymbol Implements ITypeSymbol.WithNullableAnnotation
+            Return Me


We'll have to keep an eye out of this causes problems for the cross-language cases. If we're doing a cross language refactoring that is consuming VB symbols but generating into C#, our wrapping today would have let us tag a VB symbol with a NullableAnnotation. I'm not sure if we actually did that in shared code, but you can imagine something like:

Dim x As String = If(objectOfTypeDeclaredInCSharpLibrary.MethodThatDoesNotExist(), "Hello")

could know that MethodThatDoesNotExist() should be a String?, and could be generated as such, if it's coming from a C# library.

The scenarios I think are niche, but it's notable that now those are now due to limitations in the compiler API. I doubt we have any tests that depend on it, and the scenarios aren't critical by any stretch of the imagination.

We'll have to keep an eye out of this causes problems for the cross-language cases. ...

Sure, I think it would be better to have a real scenario rather than trying to prematurely complicate implementation.

jasonmalinowski · 2019-10-30T23:04:57Z

src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs

                    // to exclude the property symbol from being retrieved.
-                    return this.GetDeclaredMember(container, declarationSyntax.Span) as MethodSymbol;
+                    return (this.GetDeclaredMember(container, declarationSyntax.Span) as MethodSymbol).GetPublicSymbol();



Does this (and similar patterns) need a ?.GetPublicSymbol() in case GetDeclaredMember is returning null? It seems like this was previously being defensive and the defense is now lost? #Resolved

Does this (and similar patterns) need a ?.GetPublicSymbol() in case GetDeclaredMember is returning null? It seems like this was previously being defensive and the defense is now lost?

GetPublicSymbol is an extension method that is safe to call with a null receiver. It does the null check itself. #Resolved

The compiler merged support for top-level symbol nullability in dotnet#39498, so we can now delete our own wrappers that were performing the same thing. This is a mostly mechanical change. For places where we were calling LookupSymbols or ClassifyConversion, those places previously called .WithoutNullability() since the compiler API would have thrown if we gave it our wrappers, but the APIs had otherwise no way to pass top-leve nullability. That was an accepted oversight at the time, and the belief is passing in the full symbol will generally be more correct.

AlekseyTs added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Oct 24, 2019

AlekseyTs requested review from a team and jasonmalinowski October 24, 2019 16:48

AlekseyTs requested review from a team as code owners October 24, 2019 16:48

AlekseyTs commented Oct 24, 2019

View reviewed changes

jasonmalinowski requested a review from ryzngard October 24, 2019 18:20

AlekseyTs added 4 commits October 24, 2019 14:17

Add NullableAnnotation/WithNullableAnnotation APIs to ITypeSymbol.

c2f38f5

This change also separates implementation of ISymbol interfaces from internal symbols in C# compiler.

Test changes

cacb2fd

Fix formatting warning

c63408f

More formatting fixes.

0303079

AlekseyTs force-pushed the ITypeSymbolWithNullable branch from 2d3a1e0 to 0303079 Compare October 24, 2019 21:20

AlekseyTs commented Oct 24, 2019

View reviewed changes

333fred assigned 333fred and cston and unassigned 333fred Oct 24, 2019

cston reviewed Oct 24, 2019

View reviewed changes