Scoped implicitly-typed lambda parameters #64680

jcouv · 2022-10-12T21:18:46Z

Fixes #62080
Corresponding spec update: dotnet/csharplang#6551

Relates to test plan #59194 (ref fields)

cston · 2022-10-13T22:34:24Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

@@ -611,7 +613,8 @@ private static TypeWithAnnotations DelegateReturnTypeWithAnnotations(MethodSymbo
            }

            var parameterRefKindsBuilder = ArrayBuilder<RefKind>.GetInstance(ParameterCount);
-            var parameterScopesBuilder = ArrayBuilder<DeclarationScope>.GetInstance(ParameterCount);
+            var parameterDeclarationScopesBuilder = ArrayBuilder<DeclarationScope>.GetInstance(ParameterCount);


parameterDeclarationScopesBuilder

Consider calling this parameterDeclaredScopesBuilder. #Closed

cston · 2022-10-13T22:34:49Z

src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs

@@ -304,9 +307,11 @@ internal override void AddDeclarationDiagnostics(BindingDiagnosticBag diagnostic
            CSharpCompilation compilation,
            UnboundLambda unboundLambda,
            ImmutableArray<TypeWithAnnotations> parameterTypes,
-            ImmutableArray<RefKind> parameterRefKinds)
+            ImmutableArray<RefKind> parameterRefKinds,
+            ImmutableArray<DeclarationScope> parameterDeclarationScopes)


parameterDeclarationScopes

Consider calling this parameterDeclaredScopes. #Closed

cston · 2022-10-13T23:20:06Z

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

@@ -225,5 +225,28 @@ internal static bool IsValidUnscopedRefAttributeTarget(this MethodSymbol method)
                !method.IsConstructor() &&
                !method.IsInitOnly;
        }
+
+#nullable enable
+        internal static ImmutableArray<DeclarationScope> GetByValueParameterDeclarationScopes(this MethodSymbol? method)


GetByValueParameterDeclarationScopes

I think it would be helpful if these methods are named as "...DeclaredScopes" or "...EffectiveScopes". Currently, it's not clear if "...DeclarationScopes" refers to the enum type name or the declared values. #Closed

cston · 2022-10-13T23:21:36Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

@@ -1054,6 +1094,7 @@ public static ReturnInferenceCacheKey Create(NamedTypeSymbol? delegateType, bool

                        parameterTypes = typesBuilder.ToImmutableAndFree();
                        parameterRefKinds = refKindsBuilder.ToImmutableAndFree();
+                        parameterDeclarationScopes = MethodSymbolExtensions.GetByValueParameterDeclarationScopes(invoke);


MethodSymbolExtensions.GetByValueParameterDeclarationScopes(invoke)

invoke.GetByValueParameterDeclarationScopes()? #Closed

cston · 2022-10-13T23:44:51Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+                    {
+                        return false;
+                    }
+                }


return ParameterDeclarationScopes.SequenceEqual(other.ParameterDeclarationScopes); #Closed

cston · 2022-10-13T23:52:38Z

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

+                    {
+                        M2(r => r);
+                        M2((scoped R r) => r); // 1
+                    }


}

Consider testing M2(r => default);. Should overload resolution fail in that case, with ambiguous overloads? #Closed

cston · 2022-10-13T23:56:08Z

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

+                {
+                    void M()
+                    {
+                        D1 d1 = () => r;


r

Consider changing to new R(), to avoid any impact from that error. #Closed

cston · 2022-10-13T23:57:05Z

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

+                    void M()
+                    {
+                        D1 d1 = (scoped R r) => r;
+                    }


}

Consider testing d1 = r => r; as well. #Closed

cston · 2022-10-14T04:24:31Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+                    return this.ParameterDeclarationScopes.IsDefault == other.ParameterDeclarationScopes.IsDefault;
+                }
+
+                if (this.ParameterDeclarationScopes.Length != other.ParameterDeclarationScopes.Length)


if (this.ParameterDeclarationScopes.Length != other.ParameterDeclarationScopes.Length)

Minor: I believe this is included in SequenceEqual() below. #Closed

AlekseyTs · 2022-10-14T18:12:44Z

@jcouv Is there a spec how this is supposed to work?

AlekseyTs · 2022-10-14T18:19:53Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

            {
                Debug.Assert(parameterTypes.Length == parameterRefKinds.Length);
+                Debug.Assert(parameterDeclaredScopes.IsDefault || parameterTypes.Length == parameterDeclaredScopes.Length);


parameterDeclaredScopes.IsDefault

What is the scenario when this condition is true? #Closed

Is there a spec how this is supposed to work?

I'll add spec change today.

When all parameters are unscoped, we use a default array.

Created corresponding spec update: dotnet/csharplang#6551

AlekseyTs · 2022-10-14T18:24:18Z

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

+
+            static DeclarationScope getByValueParameterDeclaredScope(ParameterSymbol parameter)
+            {
+                // For implicitly-typed lambda parameters, we only care about declaration scopes on by-value parameters.


For implicitly-typed lambda parameters, we only care about declaration scopes on by-value parameters.

It is not obvious why is that. #Closed

If a parameter is ref or out then the parameters must have been explicitly-typed.

If a parameter is ref or out then the parameters must have been explicitly-typed.

I am not sure I am following. We are looking at a method symbol. All parameters are explicitly typed for it.

Still looking at other changes, I might get it at the end

jaredpar · 2022-10-17T15:31:41Z

@roslyn-compiler please ignore this message. Testing out a GitHub rule change.

cston · 2022-10-17T15:52:02Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs

@@ -1502,7 +1502,7 @@ internal sealed class SourceComplexParameterSymbolWithCustomModifiersPrecedingRe
            SyntaxReference syntaxRef,
            bool isParams,
            bool isExtensionMethodThis,
-            DeclarationScope scope)
+            DeclarationScope? scope)


DeclarationScope? scope

Is this change needed? Can we get here from a LambdaParameterSymbol? #Resolved

cston · 2022-10-17T15:53:28Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs

@@ -1479,7 +1479,7 @@ internal sealed class SourceComplexParameterSymbol : SourceComplexParameterSymbo
            SyntaxReference syntaxRef,
            bool isParams,
            bool isExtensionMethodThis,
-            DeclarationScope scope)
+            DeclarationScope? scope)


DeclarationScope? scope

Is this change needed? Can we get here from a LambdaParameterSymbol? #Resolved

WithCustomModifiersAndParamsCore copies this.DeclaredSymbol (which is Nullable<DeclarationScope>) using this constructor.
It similarly calls the SourceComplexParameterSymbolWithCustomModifiersPrecedingRef constructor.

WithCustomModifiersAndParamsCore() should not be used for LambdaParameterSymbol though. If it is, we'd presumably need to copy EffectiveScope as well.

WithCustomModifiersAndParamsCore isn't used for lambdas. It's used for OHI scenarios.

AlekseyTs · 2022-10-17T21:13:40Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceParameterSymbol.cs

-            var declaredScope = this.DeclaredScope;
+            // DeclaredScope is only null in LambdaParameterSymbol where EffectiveScope can be provided rather than computed
+            Debug.Assert(this.DeclaredScope is not null);
+            var declaredScope = this.DeclaredScope.Value;


Value

ValueOrDefault? Or is it your intent to crash on null value here? If so, perhaps the assert is redundant.

I'd rather leave as-is. I thought this was nullable-enabled code.

AlekseyTs

LGTM (commit 9)

This reverts commit 5bf1a43.

Scoped implicitly-typed lambda parameters

4b76f8a

jcouv added the New Language Feature - Ref Fields label Oct 12, 2022

jcouv self-assigned this Oct 12, 2022

dotnet-issue-labeler bot added the Area-Compilers label Oct 12, 2022

Add declaration scopes to lambda cache key

fd3c176

jcouv force-pushed the scoped-lambda branch from 5c7ae2f to fd3c176 Compare October 12, 2022 21:21

jcouv marked this pull request as ready for review October 12, 2022 23:49

jcouv requested a review from a team as a code owner October 12, 2022 23:49

cston reviewed Oct 13, 2022

View reviewed changes

jcouv added 2 commits October 13, 2022 19:31

Address feedback

838b4be

One more rename

32954a5

jcouv requested a review from cston October 14, 2022 02:33

cston reviewed Oct 14, 2022

View reviewed changes

cston approved these changes Oct 14, 2022

View reviewed changes

simplify

a14ae68

AlekseyTs reviewed Oct 14, 2022

View reviewed changes

jcouv added 2 commits October 14, 2022 19:53

Flow effective scope through

bd27664

tweaks

c4a6441

jcouv marked this pull request as draft October 15, 2022 16:56

Address TODO

9ca7eeb

jcouv marked this pull request as ready for review October 15, 2022 18:54

cston reviewed Oct 17, 2022

View reviewed changes

cston approved these changes Oct 17, 2022

View reviewed changes

jcouv requested a review from AlekseyTs October 17, 2022 16:29

Split test

db9e1ee

AlekseyTs reviewed Oct 17, 2022

View reviewed changes

AlekseyTs approved these changes Oct 17, 2022

View reviewed changes

jcouv merged commit 5bf1a43 into dotnet:main Oct 17, 2022

jcouv deleted the scoped-lambda branch October 17, 2022 22:24

ghost added this to the Next milestone Oct 17, 2022

allisonchou mentioned this pull request Oct 22, 2022

[Automated] PRs inserted in VS build main-33021.259 #64913

Closed

RikkiGibson modified the milestones: Next, 17.5 P1 Oct 24, 2022

jcouv added a commit to jcouv/roslyn that referenced this pull request Oct 25, 2022

Revert "Scoped implicitly-typed lambda parameters (dotnet#64680)"

7ecf390

This reverts commit 5bf1a43.

jcouv mentioned this pull request Oct 26, 2022

Revert "Scoped implicitly-typed lambda parameters (#64680)" #64986

Merged

jcouv added a commit that referenced this pull request Oct 27, 2022

Revert "Scoped implicitly-typed lambda parameters (#64680)" (#64986)

c50d5e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scoped implicitly-typed lambda parameters #64680

Scoped implicitly-typed lambda parameters #64680

jcouv commented Oct 12, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 14, 2022 •

edited by jcouv

Loading

AlekseyTs commented Oct 14, 2022

AlekseyTs Oct 14, 2022 •

edited

Loading

jcouv Oct 14, 2022 •

edited

Loading

jcouv Oct 14, 2022

jcouv Oct 14, 2022

AlekseyTs Oct 14, 2022 •

edited

Loading

jcouv Oct 14, 2022

AlekseyTs Oct 14, 2022

AlekseyTs Oct 14, 2022

jaredpar commented Oct 17, 2022

cston Oct 17, 2022 •

edited by jcouv

Loading

cston Oct 17, 2022 •

edited by jcouv

Loading

jcouv Oct 17, 2022

cston Oct 17, 2022 •

edited

Loading

jcouv Oct 17, 2022

AlekseyTs Oct 17, 2022

jcouv Oct 17, 2022

AlekseyTs left a comment

Scoped implicitly-typed lambda parameters #64680

Scoped implicitly-typed lambda parameters #64680

Conversation

jcouv commented Oct 12, 2022 • edited Loading

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

cston Oct 14, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs commented Oct 14, 2022

AlekseyTs Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Oct 17, 2022

cston Oct 17, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Oct 17, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Oct 12, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 13, 2022 •

edited

Loading

cston Oct 14, 2022 •

edited by jcouv

Loading

AlekseyTs Oct 14, 2022 •

edited

Loading

jcouv Oct 14, 2022 •

edited

Loading

AlekseyTs Oct 14, 2022 •

edited

Loading

cston Oct 17, 2022 •

edited by jcouv

Loading

cston Oct 17, 2022 •

edited by jcouv

Loading

cston Oct 17, 2022 •

edited

Loading