Extend scope of type parameters in attributes by jcouv · Pull Request #40525 · dotnet/roslyn

jcouv · 2019-12-20T08:40:25Z

Also fixes the type parameter scoping on records (fixes #60379)
Fixes #60194

This PR adds type parameters inside nameof in attributes on methods (and local functions).

Some context on current rules:
Type parameters are in scope in within a method declaration (attributes on type parameters, on parameters, in parameter default values, and obviously in parameter types and type constraints), but we hide them when binding identifiers, except inside nameof, or when binding types (ie. sizeof/typeof/is/as/…).
So = default(T) and = nameof(T) will bind to type parameter, whereas = T will bind to something else (like a const from outer scope).
But type parameters were not in scope in attributes on the method itself at all. (note: :this contrasts with attributes on a type, where the type parameters are simply in scope)

Overview:

LocalBinderFactory.VisitInvocationExpression adds a NameofBinder for some invocation expressions (that pass syntactic check MayBeNameofOperator).
NameofBinder is a no-op when it detects a proper "nameof" method in its enclosing scope.
When the NameofBinder is chained on a ContextualAttributeBinder, it pulls in additional identifiers in scope from type parameters on attribute target (given by ContextualAttributeBinder). Since type parameters are in scope (albeit filtered out) in most locations we need them to be (attributes on types, type parameter, parameters), we just need to add them in attributes on methods.
Binder.TryBindNameofOperator gets a binder from binder factories, so that the above scopes kick in.
SyntaxTreeSemanticModel.CreateModelForAttribute adds a ContextualAttribute binder (with proper attribute target) in the RootBinder for the AttributeSemanticModel it creates.
MemberSemanticModel.GetEnclosingBinderInternalWithinRoot adds a NameofBinder on some invocation expressions.
Changes to BinderFactoryVisitor.VisitTypeDeclarationCore and SyntaxTreeSemanticModel.GetDeclarationName relate to records.

Relates to #40524 (test plan)

AlekseyTs · 2022-04-06T14:43:50Z

src/Compilers/CSharp/Portable/Binder/NameofBinder.cs

+            {
+                if ((_lazyIsInsideNameof & _lazyIsInsideNameof_IsInitialized) == 0)
+                {
+                    bool isInsideNameof = !NextRequired.InvocableNameofInScope() || base.IsInsideNameof;


!NextRequired.InvocableNameofInScope() || base.IsInsideNameof

In combination with other logic in this class, this logic feels incorrect. I think our intent is to optimistically add NameofBinder into the chain. Later on, we are supposed to check if the specific invocation expression is supposed to be treated as a "nameof expression". Only in that case the binder is supposed to have any impact and handle any requests. In all other cases, it shouldn't handle any requests and pretend as if it is not in the chain by simply delegating to the Next. So, for the purpose of determining whether this binder should have any meaningful impact, asking what base thinks is incorrect because the base answers question about different nodes, if any. It feels like we need a property that will answer the question whether this binder is a "true" nameof binder. And that property should drive the behavior instead of the overridden IsInsideNameof because it doesn't provide information specific to this binder. #Closed

Thanks for the clarification. Makes sense

AlekseyTs · 2022-04-06T14:46:14Z

src/Compilers/CSharp/Portable/Binder/NameofBinder.cs

        }

-        protected override SyntaxNode EnclosingNameofArgument => _nameofArgument;
+        protected override SyntaxNode? EnclosingNameofArgument => IsInsideNameof ? _nameofArgument : base.EnclosingNameofArgument;


IsInsideNameof

The logic doesn't look correct now because IsInsideNameof can return true even when !NextRequired.InvocableNameofInScope() is false. #Closed

AlekseyTs · 2022-04-06T14:47:10Z

src/Compilers/CSharp/Portable/Binder/NameofBinder.cs

+
+        internal override void LookupSymbolsInSingleBinder(LookupResult result, string name, int arity, ConsList<TypeSymbol> basesBeingResolved, LookupOptions options, Binder originalBinder, bool diagnose, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
+        {
+            if (_withTypeParametersBinder is not null && IsInsideNameof)


IsInsideNameof

The logic doesn't look correct now because IsInsideNameof can return true even when !NextRequired.InvocableNameofInScope() is false. #Closed

AlekseyTs · 2022-04-06T15:07:50Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+
+            var tree = comp.SyntaxTrees.Single();
+            var parentModel = comp.GetSemanticModel(tree);
+            var localFuncPosition = tree.GetText().ToString().IndexOf("[My(a)]", StringComparison.Ordinal);


tree.GetText().ToString().IndexOf("[My(a)]", StringComparison.Ordinal);

I usually locate the syntax node or token and get its SpanStart rather than doing textual search in a string. #Closed

This pattern is also used in a number of places and is unambiguous. I'll leave as-is.

AlekseyTs · 2022-04-06T15:10:40Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+            var methodPosition = tree.GetText().ToString().IndexOf("[My(b)]", StringComparison.Ordinal);
+
+            var attr = parseAttributeSyntax("[My(nameof(TParameter))]", TestOptions.Regular10);
+            VerifyTParameterSpeculation(parentModel, localFuncPosition, attr, found: false);


VerifyTParameterSpeculation(parentModel, localFuncPosition, attr, found: false);

I do not think this tests scenario 2B because the location is supposed to be inside an existing attribute. The location used here is not inside the existing attribute #Closed

I'd wrote down the following:

Scenario 2B:
TryGetSpeculativeSemanticModel with a new attribute

The reason for this is that the only TryGetSpeculativeSemanticModel overloads take statements, attributes, type syntaxes, etc, but I didn't find one that just takes a plain expressions.

The reason for this is that the only TryGetSpeculativeSemanticModel overloads take statements, attributes, type syntaxes, etc, but I didn't find one that just takes a plain expressions.

I am not following. I am talking about position where to speculate, not about with what syntax to speculate.

I see. We can try that, although it seems strange to speculate with an attribute inside an attribute.

I don't know. One might arrive to that by thinking: "I want to see what happens if I replace existing attribute application with a new one." And passes location related to the attribute about to be replaced.

Added a test

AlekseyTs · 2022-04-06T15:12:33Z

Done with review pass (commit 4), tests are not looked at.

AlekseyTs · 2022-04-06T15:55:50Z

src/Compilers/CSharp/Portable/Binder/NameofBinder.cs

+        // One bit encodes whether this was computed, the other bit encodes a boolean value
+        private int _lazyIsInsideNameof;
+        private const byte _lazyIsInsideNameof_IsInitialized = 1 << 0;
+        private const byte _lazyIsInsideNameof_Value = 1 << 1;


I think we already have ThreeState type that we use in cases like this #Closed

jcouv · 2022-04-06T17:10:50Z

@dotnet/roslyn-compiler for second review. Thanks

RikkiGibson

Implementation LGTM with some minor comments, haven't looked at tests yet.

RikkiGibson · 2022-04-07T00:22:59Z

src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs

+
+                do
+                {
+                    if (current is ContextualAttributeBinder contextualAttributeBinder)


A similar traversal is also done in GetForwardedToAssembly in Binder_Symbols.cs. Should we introduce a virtual method on Binder for this? It's fine if the answer is "no", thought I would check either way. #Pending

RikkiGibson · 2022-04-07T00:25:24Z

src/Compilers/CSharp/Portable/Binder/NameofBinder.cs

+    /// To do so, it works together with <see cref="ContextualAttributeBinder"/>.
+    /// 
+    /// For other attributes (on types, type parameters or parameters) we use a WithTypeParameterBinder directly
+    /// in the binder chain and some filtering (<see cref="LookupOptions.MustNotBeMethodTypeParameter"/>) to keep


Should we file a "Design Debt" issue to track replacing LookupOptions.MustNotBeMethodTypeParameter with a "keep the WithTypeParametersBinder on the side" strategy? #Resolved

No. I tried doing that last week, but we reached the conclusion that it would be too much of a breaking change. So we have to keep the current behavior.

RikkiGibson · 2022-04-07T00:28:35Z

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

        /// Creates an AttributeSemanticModel that allows asking semantic questions about an attribute node.
        /// </summary>
-        public static AttributeSemanticModel Create(SyntaxTreeSemanticModel containingSemanticModel, AttributeSyntax syntax, NamedTypeSymbol attributeType, AliasSymbol aliasOpt, Binder rootBinder, ImmutableDictionary<Symbol, Symbol> parentRemappedSymbolsOpt)
+        public static AttributeSemanticModel Create(SyntaxTreeSemanticModel containingSemanticModel, AttributeSyntax syntax, NamedTypeSymbol attributeType, AliasSymbol aliasOpt, Symbol attributeTarget, Binder rootBinder, ImmutableDictionary<Symbol, Symbol> parentRemappedSymbolsOpt)


It looks like nullable annotations are enabled in this file. Should we use the type Symbol? attributeTarget here? #Pending

RikkiGibson · 2022-04-07T00:29:42Z

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

                // body unless the position is either:
                // a) in a type-only context inside an expression, or
-                // b) inside of an XML name attribute in an XML doc comment.
+                // b) inside of an XML name attribute in an XML doc comment,


Just curious, did you compare the pattern you used here with whatever is used already for doc comments? #Resolved

For cref and such, BinderFactoryVisitor just uses a binder for a position inside the member declaration. Then no filter is used. See MakeCrefBinder for example.

RikkiGibson · 2022-04-07T00:33:27Z

src/Compilers/CSharp/Portable/Binder/NameofBinder.cs

+
+        internal override bool IsInsideNameof => IsNameofOperator || base.IsInsideNameof;
+
+        protected override SyntaxNode? EnclosingNameofArgument => IsNameofOperator ? _nameofArgument : base.EnclosingNameofArgument;


It looks like this implementation might be important in a nested scenario of some kind? Basically, I wasn't sure if it matters that we delegate to the base here when this is not a nameof operator, versus just returning null when this is not a nameof operator. #Resolved

Yes, a nameof inside a nameof.

RikkiGibson · 2022-04-07T00:35:09Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

+
+Possible workarounds are:
+
+1. Rename the type parameter to avoid shadowing the name from outer scope.


2 nits:

Constant is not accessible from the attribute, perhaps it should be made public to avoid confusion.

Qualifying the name of the constant also seems like an option here. e.g. [MyAttribute(nameof(C.TParameter.Constant))].

jcouv · 2022-04-07T16:34:57Z

@AlekseyTs @RikkiGibson Please take another look. Thanks

AlekseyTs · 2022-04-07T17:05:55Z

@jcouv It looks like you need to merge main into the feature branch in order to overcome the Test_Windows_Desktop_Spanish_Release_64 failure.

AlekseyTs · 2022-04-07T17:10:41Z

src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs

+
+                    binder = binder.Next;
+                }
+                while (binder != null);


while (binder != null);

Would it be worth adding an assert after the loop? It looks like we expect to never reach that point. #Closed

AlekseyTs · 2022-04-07T17:22:15Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+            parentModel = comp.GetSemanticModel(tree);
+
+            VerifyTParameterSpeculation(parentModel, localFuncPosition, attr, found: false);
+            VerifyTParameterSpeculation(parentModel, methodPosition, attr, found: false);


VerifyTParameterSpeculation

I'd like to better understand why we are failing to locate the type parameter here. Perhaps we can chat about this offline #Closed

AlekseyTs · 2022-04-07T17:47:15Z

Done with review pass (commit 7). Other than the question about test behavior, LGTM

AlekseyTs · 2022-04-07T20:31:01Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+";
+            // C# 10
+            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular10);
+            //comp.VerifyDiagnostics(


//comp.VerifyDiagnostics(

Consider removing or uncommenting.

AlekseyTs

LGTM (commit 8)

…nameof-scope

jcouv added the Area-Compilers label Dec 20, 2019

jcouv self-assigned this Dec 20, 2019

jcouv added this to the Compiler.Net5 milestone Dec 20, 2019

jcouv force-pushed the nameof-scope branch from ce5a225 to 177ad9a Compare December 20, 2019 18:58

jcouv force-pushed the nameof-scope branch 2 times, most recently from 6bd8e4e to 5076a55 Compare December 30, 2019 23:13

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 6, 2020

jcouv force-pushed the nameof-scope branch from 5e464a4 to da2edb4 Compare January 7, 2020 07:28

jcouv force-pushed the nameof-scope branch from da2edb4 to bc9ac67 Compare February 4, 2020 20:09

jaredpar modified the milestones: Compiler.Net5, 16.8 Jun 23, 2020

jinujoseph modified the milestones: 16.8, 16.9 Nov 12, 2020

Base automatically changed from master to main March 3, 2021 23:52

jinujoseph modified the milestones: 16.9, 17.0 Jun 1, 2021

Happypig375 mentioned this pull request Jul 25, 2021

nameof with forward lookup (such as to members and parameters of the applied target) fsharp/fslang-suggestions#967

Closed

5 tasks

jinujoseph modified the milestones: 17.0, 17.1 Dec 10, 2021

jcouv force-pushed the nameof-scope branch 3 times, most recently from 5ab7314 to 14578af Compare February 22, 2022 23:35

This was referenced Mar 10, 2022

Unexpected scoping on type parameter of local function #59775

Closed

Fix type parameter scoping for local functions #59968

Closed

jcouv force-pushed the nameof-scope branch 3 times, most recently from fb7b673 to fd18833 Compare March 14, 2022 20:42

This was referenced Mar 14, 2022

Fix parameter scoping on lambdas #60169

Closed

Fix type parameter scoping for local functions #60098

Merged

jcouv force-pushed the nameof-scope branch from fd18833 to ae71929 Compare March 16, 2022 07:13

AlekseyTs reviewed Apr 6, 2022

View reviewed changes

Address feedback

8d971f0

jcouv requested a review from AlekseyTs April 6, 2022 17:10

Add test for speculative model inside existing attribute

2a93ab5

RikkiGibson self-assigned this Apr 6, 2022

RikkiGibson reviewed Apr 7, 2022

View reviewed changes

Address feedback

8a1afc4

AlekseyTs reviewed Apr 7, 2022

View reviewed changes

Check LangVer from compilation

68590fe

jcouv requested a review from AlekseyTs April 7, 2022 20:07

RikkiGibson approved these changes Apr 7, 2022

View reviewed changes

AlekseyTs reviewed Apr 7, 2022

View reviewed changes

AlekseyTs approved these changes Apr 7, 2022

View reviewed changes

Julien Couvreur added 2 commits April 7, 2022 14:28

uncomment

8d65bef

Merge remote-tracking branch 'dotnet/features/nameof-parameter' into …

4151841

…nameof-scope

jcouv enabled auto-merge (squash) April 8, 2022 03:21

jcouv merged commit 8fa902f into dotnet:features/nameof-parameter Apr 8, 2022

jcouv deleted the nameof-scope branch April 8, 2022 03:54

jcouv mentioned this pull request May 13, 2022

Records with ; body break references to members from record attribute #60379

Closed


		internal override bool IsInsideNameof => IsNameofOperator \|\| base.IsInsideNameof;

		protected override SyntaxNode? EnclosingNameofArgument => IsNameofOperator ? _nameofArgument : base.EnclosingNameofArgument;


		Possible workarounds are:

		1. Rename the type parameter to avoid shadowing the name from outer scope.

Conversation

jcouv commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Apr 6, 2022

Uh oh!

AlekseyTs Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Apr 6, 2022

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Apr 7, 2022

Uh oh!

AlekseyTs commented Apr 7, 2022

jcouv commented Dec 20, 2019 •

edited

Loading

AlekseyTs Apr 6, 2022 •

edited

Loading

AlekseyTs Apr 6, 2022 •

edited

Loading

AlekseyTs Apr 6, 2022 •

edited

Loading

AlekseyTs Apr 6, 2022 •

edited

Loading

AlekseyTs Apr 6, 2022 •

edited

Loading

AlekseyTs Apr 6, 2022 •

edited

Loading

RikkiGibson Apr 7, 2022 •

edited by jcouv

Loading

RikkiGibson Apr 7, 2022 •

edited by jcouv

Loading

RikkiGibson Apr 7, 2022 •

edited by jcouv

Loading

RikkiGibson Apr 7, 2022 •

edited by jcouv

Loading

RikkiGibson Apr 7, 2022 •

edited by jcouv

Loading

AlekseyTs Apr 7, 2022 •

edited

Loading

AlekseyTs Apr 7, 2022 •

edited

Loading