Handle capturing of primary constructor parameters within queries and in type-or-value scenarios #66254

AlekseyTs · 2023-01-05T00:17:47Z

This PR matches the version of the spec from dotnet/csharplang#6855.

It might be easier to review commit by commit.

…expressions

…r Color) scenarios.

jjonescz · 2023-01-05T12:29:22Z

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

+            }
+
+            this.LookupExtensionMethodsInSingleBinder(scope, lookupResult, rightName, arity, options, ref useSiteInfo);
+            return useSiteInfo;


return useSiteInfo;

The return value of this method is never used? #Closed

The return value of this method is never used?

Yes. This is an artifact of an automatic refactoring. Will fix.

jjonescz · 2023-01-05T12:29:54Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordConstructor.cs

-            captured.Free();
-
-            return _capturedParameters;
+            throw ExceptionUtilities.Unreachable();


throw ExceptionUtilities.Unreachable();

This doesn't seem to be needed. Consider also lifting code out of the else block above - it's not needed since the if returns. #Closed

jjonescz · 2023-01-05T12:48:38Z

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

+        }
+
+        [Fact]
+        public void ParameterCapturing_022_ColorColor_MemberAccess_Instance_Method()


ParameterCapturing_022_ColorColor_MemberAccess_Instance_Method

Consider also testing when both extension method and instance method are available. #Closed

jjonescz · 2023-01-05T13:11:46Z

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

+                Diagnostic(ErrorCode.WRN_UnreadRecordParameter, "Color").WithArguments("Color").WithLocation(2, 17),
+                // (4,33): error CS9500: Cannot use primary constructor parameter 'Color Color' in this context.
+                //     public static int Test() => Color.M1(new S1());
+                Diagnostic(ErrorCode.ERR_InvalidPrimaryConstructorParameterReference, "Color").WithArguments("Color Color").WithLocation(4, 33)


Diagnostic(ErrorCode.ERR_InvalidPrimaryConstructorParameterReference, "Color").WithArguments("Color Color").WithLocation(4, 33)

This error feels unexpected. The parameter is not used here. I would expect this code to compile and call the static method. #Closed

This error feels unexpected. The parameter is not used here. I would expect this code to compile and call the static method.

Language rules do not agree with these expectations
https://sharplab.io/#v2:C4LglgNgPgzsBOBXAxsABAZQIwFgBQA3vmiWgAIDM5WAbGmAHboAqApnABQDCA9hD/DS9+8AJRoAvAD4hfAQDoAslg4NWAd0wrRogNz4AvvnxkATLJH4ieUuSqN0yjtjQAPcdLQAGfTdLFSSmo6BzRlAB5mKQ5mN3F1AAtWeFY0WJA0RAYAWwBDBlyAc1YAE0kZH0N8IA===

struct S1 { public static int Test(Color Color) => Color.M1(new S1()); } class Color { public int M1(S1 x) => 0; public static int M1<T>(T x) where T : unmanaged => 0; }

Observed:

.method public hidebysig static int32 Test ( class Color Color ) cil managed { .custom instance void System.Runtime.CompilerServices.NullableContextAttribute::.ctor(uint8) = ( 01 00 01 00 00 ) // Method begins at RVA 0x20a0 // Code size 16 (0x10) .maxstack 2 .locals init ( [0] valuetype S1 ) IL_0000: ldarg.0 IL_0001: ldloca.s 0 IL_0003: initobj S1 IL_0009: ldloc.0 IL_000a: callvirt instance int32 Color::M1(valuetype S1) IL_000f: ret } // end of method S1::Test

jjonescz · 2023-01-05T13:30:00Z

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

+            comp.VerifyEmitDiagnostics(
+                // (6,19): error CS0229: Ambiguity between 'Color.P' and 'Color.P'
+                //         _ = Color.P;
+                Diagnostic(ErrorCode.ERR_AmbigMember, "P").WithArguments("Color.P", "Color.P").WithLocation(6, 19)


ERR_AmbigMember

I'm curious why this also isn't ERR_AmbiguousPrimaryConstructorParameterAsColorColorReceiver? #Closed

I'm curious why this also isn't ERR_AmbiguousPrimaryConstructorParameterAsColorColorReceiver?

Because lookup fails with the ERR_AmbigMember, C# doesn't support overloading of regular properties. The ERR_AmbiguousPrimaryConstructorParameterAsColorColorReceiver is reported if lookup succeeds

AlekseyTs · 2023-01-06T01:44:20Z

@cston, @dotnet/roslyn-compiler For the second review

cston · 2023-01-06T18:20:01Z

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

-                             (object)contaningMember.ContainingSymbol != primaryCtor.ContainingSymbol || // The member doesn't belong to our type, i.e. from nested type
-                             ((object)contaningMember != primaryCtor && contaningMember is MethodSymbol { MethodKind: MethodKind.Constructor })) && // We are in a non-primary instance constructor
+                            (!IsInDeclaringTypeInstanceMember(primaryCtor) ||
+                             (this.ContainingMember() is MethodSymbol { MethodKind: MethodKind.Constructor } contaningMember && (object)contaningMember != primaryCtor)) && // We are in a non-primary instance constructor


contaning

Typo. #Closed

cston · 2023-01-06T18:30:47Z

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

+        {
+            return !(InParameterDefaultValue ||
+                     InAttributeArgument ||
+                     this.ContainingMember() is not { Kind: not SymbolKind.NamedType, IsStatic: false } contaningMember || // We are not in an instance member


contaning

Typo. #Closed

cston · 2023-01-06T18:59:51Z

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

@@ -6390,12 +6404,17 @@ private BoundExpression BindLeftIdentifierOfPotentialColorColorMemberAccess(Iden
                            var boundType = BindNamespaceOrType(left, typeDiagnostics);
                            if (TypeSymbol.Equals(boundType.Type, leftType, TypeCompareKind.AllIgnoreOptions))
                            {
+                                Debug.Assert(!leftType.IsDynamic());
+                                Debug.Assert(IsPotentialColorColorReceiver(left, leftType));


Debug.Assert(IsPotentialColorColorReceiver(left, leftType));

What is the assert checking? (It looks like IsPotentialColorColorReceiver() is checking the same two conditions as the two if conditions above.) #Resolved

What is the assert checking? (It looks like IsPotentialColorColorReceiver() is checking the same two conditions as the two if conditions above.)

It is verifying that we can rely on IsPotentialColorColorReceiver elsewhere to check for the same condition.

cston · 2023-01-06T19:17:51Z

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

@@ -1564,6 +1564,27 @@ private BoundExpression ReplaceTypeOrValueReceiver(BoundExpression receiver, boo
            }
        }

+        private BoundExpression GetValueExpressionIfTypeOrValueReceiver(BoundExpression receiver)


private

static #Closed

cston · 2023-01-06T22:19:37Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolutionResult.cs

-                diagnostics.Add(ErrorCode.ERR_FuncPtrMethMustBeStatic, location, symbol);
-            }
-            else
+            if (receiverOpt?.HasErrors != true)


receiverOpt?.HasErrors != true

What is the impact of this change? Does it improve errors or does it affect non-error scenarios? #Resolved

What is the impact of this change? Does it improve errors or does it affect non-error scenarios?

It reduces the noise in error scenarios. See the only test in QueryTests.cs, for example. However, the primary goal was to suppress cascading diagnostics on a BoundBadExpression that we create in ConstructBoundMemberGroupAndReportOmittedTypeArguments due to an ambiguity.

cston · 2023-01-06T23:10:35Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordConstructor.cs


-                LookupOptions options = LookupOptions.AllMethodsOnArityZero;
-                if (SyntaxFacts.IsInvoked(id))
+            bool checkQuery(QueryExpressionSyntax query, Binder enclosingBinder)


bool

Consider adding // Follows the logic of BindQuery
#Closed

cston · 2023-01-09T05:25:38Z

    public void ParameterCapturing_033_ColorColor_MemberAccess_InstanceAndStatic_Method()

Is this test identical to _032?

In reply to: 1375123522

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:9677 in 808952a. [](commit_id = 808952a, deletion_comment = False)

cston · 2023-01-09T05:45:54Z

            // (6,9): error CS9501: Identifier 'Color' is ambiguous between type 'Color' and parameter 'Color Color' in this context.

Why is Color in Color.M1(this) ambiguous here but not in _046?

In reply to: 1375131340

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10284 in 808952a. [](commit_id = 808952a, deletion_comment = False)

cston · 2023-01-09T05:49:39Z

            // (5,9): error CS9501: Identifier 'Color' is ambiguous between type 'Color' and parameter 'Color Color' in this context.

Why is Color ambiguous? Isn't the instance member M1() inapplicable because of the argument mismatch?

In reply to: 1375132933

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10315 in 808952a. [](commit_id = 808952a, deletion_comment = False)

cston · 2023-01-09T05:58:13Z

}

Consider including capture of an unmanaged parameter:

struct S3(int p3)
{
    string Test() => p3;
}

Test(new S3());

In reply to: 1375136132

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10728 in 808952a. [](commit_id = 808952a, deletion_comment = False)

cston · 2023-01-09T06:00:30Z

    public void ParameterCapturing_058_ColorColor_MemberAccess_InstanceAndStatic_Property()

It looks like both properties are instance members.

In reply to: 1375136996

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10829 in 808952a. [](commit_id = 808952a, deletion_comment = False)

cston · 2023-01-09T06:02:16Z

    Color.M1(this);

Why isn't this ambiguous?

In reply to: 1375138681

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10872 in 808952a. [](commit_id = 808952a, deletion_comment = False)

AlekseyTs · 2023-01-09T15:38:13Z

    public void ParameterCapturing_033_ColorColor_MemberAccess_InstanceAndStatic_Method()

Is this test identical to _032?

Looks like it is. I'll remove it.

In reply to: 1375123522

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:9677 in 808952a. [](commit_id = 808952a, deletion_comment = False)

AlekseyTs · 2023-01-09T15:45:45Z

            // (6,9): error CS9501: Identifier 'Color' is ambiguous between type 'Color' and parameter 'Color Color' in this context.

Why is Color in Color.M1(this) ambiguous here but not in _046?

Initializer value is not a capturing context, therefore the ambiguity restriction doesn't apply. Here is one of the conditions from the spec: "If E is treated as a simple name, rather than a type name, it would refer to a primary constructor parameter and would capture the parameter into the state of the enclosing type."

In reply to: 1375131340

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10284 in 808952a. [](commit_id = 808952a, deletion_comment = False)

AlekseyTs · 2023-01-09T15:49:41Z

            // (5,9): error CS9501: Identifier 'Color' is ambiguous between type 'Color' and parameter 'Color Color' in this context.

Why is Color ambiguous? Isn't the instance member M1() inapplicable because of the argument mismatch?

What argument mismatch? Regardless, the ambiguity rule doesn't take arguments into consideration at all. See dotnet/csharplang#6855.

In reply to: 1375132933

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10315 in 808952a. [](commit_id = 808952a, deletion_comment = False)

AlekseyTs · 2023-01-09T15:54:48Z

    Color.M1(this);

Why isn't this ambiguous?

There are no static methods. Per spec: "Extension methods applicable to the receiver type are treated as instance methods for the purpose of this check."

The "Color" definetly won't be valid as a type.

In reply to: 1375138681

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10872 in 808952a. [](commit_id = 808952a, deletion_comment = False)

cston · 2023-01-09T19:07:21Z

What argument mismatch?

The first parameter in the instance method is declared as S1 rather than Color.

In reply to: 1375132933

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PrimaryConstructorTests.cs:10315 in 808952a. [](commit_id = 808952a, deletion_comment = False)

AlekseyTs · 2023-01-09T19:58:41Z

The first parameter in the instance method is declared as S1 rather than Color.

Thanks for pointing this out. I will adjust this in one of the next PRs

In reply to: 1376143344

AlekseyTs added 2 commits January 4, 2023 16:12

Properly detect capturing of primary constructor parameters in query …

ee9a0c3

…expressions

Handle Primary Constructor parameter capturing in type-or-value (Colo…

f3042a1

…r Color) scenarios.

AlekseyTs added Area-Compilers Feature - Primary Constructors labels Jan 5, 2023

AlekseyTs requested review from jjonescz and cston January 5, 2023 00:17

AlekseyTs requested a review from a team as a code owner January 5, 2023 00:17

jjonescz reviewed Jan 5, 2023

View reviewed changes

AlekseyTs added 2 commits January 5, 2023 07:36

Fixup

68025bd

PR feedback

608c702

jjonescz approved these changes Jan 5, 2023

View reviewed changes

cston reviewed Jan 6, 2023

View reviewed changes

AlekseyTs added 2 commits January 7, 2023 16:26

PR feedback

a102b6c

Address feedback from previous PRs

808952a

PR feedback, plus more tests

4956b96

cston approved these changes Jan 9, 2023

View reviewed changes

AlekseyTs merged commit 2adfb12 into dotnet:features/PrimaryConstructors Jan 9, 2023

allisonchou mentioned this pull request Mar 3, 2023

[Automated] PRs inserted in VS build main-33503.187 #67176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle capturing of primary constructor parameters within queries and in type-or-value scenarios #66254

Handle capturing of primary constructor parameters within queries and in type-or-value scenarios #66254

AlekseyTs commented Jan 5, 2023

jjonescz Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023

jjonescz Jan 5, 2023 •

edited

Loading

jjonescz Jan 5, 2023 •

edited

Loading

jjonescz Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023

jjonescz Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023

AlekseyTs commented Jan 6, 2023

cston Jan 6, 2023 •

edited

Loading

cston Jan 6, 2023 •

edited

Loading

cston Jan 6, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jan 6, 2023

cston Jan 6, 2023 •

edited

Loading

cston Jan 6, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Jan 6, 2023

cston Jan 6, 2023 •

edited

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023 •

edited

Loading

cston commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

Handle capturing of primary constructor parameters within queries and in type-or-value scenarios #66254

Handle capturing of primary constructor parameters within queries and in type-or-value scenarios #66254

Conversation

AlekseyTs commented Jan 5, 2023

jjonescz Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 5, 2023

Choose a reason for hiding this comment

jjonescz Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jjonescz Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

jjonescz Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 5, 2023

Choose a reason for hiding this comment

jjonescz Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 5, 2023

Choose a reason for hiding this comment

AlekseyTs commented Jan 6, 2023

cston Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

cston Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

cston Jan 6, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 6, 2023

Choose a reason for hiding this comment

cston Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

cston Jan 6, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 6, 2023

Choose a reason for hiding this comment

cston Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

cston commented Jan 9, 2023 • edited by AlekseyTs Loading

cston commented Jan 9, 2023 • edited by AlekseyTs Loading

cston commented Jan 9, 2023 • edited by AlekseyTs Loading

cston commented Jan 9, 2023 • edited by AlekseyTs Loading

cston commented Jan 9, 2023 • edited by AlekseyTs Loading

cston commented Jan 9, 2023 • edited by AlekseyTs Loading

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023 • edited Loading

cston commented Jan 9, 2023

AlekseyTs commented Jan 9, 2023

jjonescz Jan 5, 2023 •

edited

Loading

jjonescz Jan 5, 2023 •

edited

Loading

jjonescz Jan 5, 2023 •

edited

Loading

jjonescz Jan 5, 2023 •

edited

Loading

jjonescz Jan 5, 2023 •

edited

Loading

cston Jan 6, 2023 •

edited

Loading

cston Jan 6, 2023 •

edited

Loading

cston Jan 6, 2023 •

edited by AlekseyTs

Loading

cston Jan 6, 2023 •

edited

Loading

cston Jan 6, 2023 •

edited by AlekseyTs

Loading

cston Jan 6, 2023 •

edited

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

cston commented Jan 9, 2023 •

edited by AlekseyTs

Loading

AlekseyTs commented Jan 9, 2023 •

edited

Loading