Handle CallerArgumentExpression in attributes #53535

Youssef1313 · 2021-05-20T08:26:29Z

Draft for now as it's currently not working 😕

The code I wrote seems to be getting the default value for correctly as shown in the below screenshot. So no idea what's going on.

@333fred @AlekseyTs Would you be able to help here?

Test plan: #52745.

AlekseyTs · 2021-05-20T14:00:14Z

@Youssef1313

Would you be able to help here?

Please add relevant tests

AlekseyTs · 2021-05-20T14:09:10Z

For the attempt to try behavior in VS, make sure the updated version of the compiler is used to build the scenario. If you are using deployment project from Roslyn solution, that is good for testing IDE behavior, but usually doesn't use updated compiler when builds out of process.

In reply to: 845157436

Youssef1313 · 2021-05-20T15:43:53Z

@aleksey Thanks! Indeed it looks like the Roslyn Deployment wasn't using the updated compiler. The test passed. 🎉

AlekseyTs · 2021-05-20T18:20:08Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

+    public MyAttribute(string s, [CallerArgumentExpression(""s"")] string x = """") => Console.WriteLine($""'{s}', '{x}'"");
+}
+
+[My(""Hello"")]


Consider also testing what do we get from GetAttributes API on the calss. #Closed

AlekseyTs · 2021-05-20T18:30:57Z

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

@@ -874,6 +881,19 @@ private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, Attribu
            {
                return new TypedConstant(parameterType, kind, defaultValue);
            }
+
+            static int getArgumentIndex(ParameterSymbol parameter, ImmutableArray<string> constructorArgumentNamesOpt)


getArgumentIndex

"getCallerArgumentArgumentIndex"? #Closed

AlekseyTs · 2021-05-20T18:34:44Z

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

+                Debug.Assert(parameter.ContainingSymbol is MethodSymbol);
+                var methodSymbol = (MethodSymbol)parameter.ContainingSymbol;
+                var callerArgumentParameter = methodSymbol.Parameters[parameter.CallerArgumentExpressionParameterIndex];
+                return constructorArgumentNamesOpt.IndexOf(callerArgumentParameter.Name);


return constructorArgumentNamesOpt.IndexOf(callerArgumentParameter.Name);

It looks like this code path isn't tested. Especially it is interesting what the behavior is for the situation when the corresponding argument isn't actually named, but others are. #Closed

Also when there is an out of order named argument used at the position of the target parameter.

AlekseyTs · 2021-05-20T18:37:03Z

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

@@ -775,7 +775,7 @@ private static int GetMatchingNamedConstructorArgumentIndex(string parameterName
            return argIndex;
        }

-        private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, AttributeSyntax syntax, BindingDiagnosticBag diagnostics)
+        private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, AttributeSyntax syntax, ImmutableArray<string> constructorArgumentNamesOpt, int argumentsCount, BindingDiagnosticBag diagnostics)


GetDefaultValueArgument

We have two call sites and only one test scenario. It feels like one call site doesn't have test coverage #Closed

@AlekseyTs I added few more tests. But seems I'm still hitting one call site only.

I added few more tests. But seems I'm still hitting one call site only.

Is this still the case or were you able to cover both code paths?

@AlekseyTs It's not the case now.

AlekseyTs · 2021-05-20T18:43:30Z

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

+                Debug.Assert(parameter.ContainingSymbol is MethodSymbol);
+                var methodSymbol = (MethodSymbol)parameter.ContainingSymbol;
+                var callerArgumentParameter = methodSymbol.Parameters[parameter.CallerArgumentExpressionParameterIndex];
+                return constructorArgumentNamesOpt.IndexOf(callerArgumentParameter.Name);


constructorArgumentNamesOpt.IndexOf(callerArgumentParameter.Name)

We probably should take advantage of GetMatchingNamedConstructorArgumentIndex helper rather than inventing a new way of mapping parameters to named arguments. #Closed

@AlekseyTs GetMatchingNamedConstructorArgumentIndex needs to take the parameter name, which is mostly what the helper I wrote does. Also GetMatchingNamedConstructorArgumentIndex doesn't handle the case when no named arguments exist.

GetMatchingNamedConstructorArgumentIndex needs to take the parameter name, which is mostly what the helper I wrote does

I didn't mean to say the helper is all what we need, but for the same portion of the task I prefer to reuse it.

I found that my initial approach is problematic and hard to fix it to pass the newly added test cases. So I used a totally new approach.

I found that my initial approach is problematic and hard to fix it to pass the newly added test cases. So I used a totally new approach.

I still think we should unify what we do here with what GetMatchingNamedConstructorArgumentIndex, unless there is a good reason to diverge.

@AlekseyTs I'm confused by this comment vs. #53535 (comment)

The other comment suggests to completely get rid of GetMatchingNamedConstructorArgumentIndex, which I did locally and all compiler tests passed.

I went with the other comment as GetMatchingNamedConstructorArgumentIndex didn't seem necessary.

AlekseyTs · 2021-05-20T18:46:55Z

Done with review pass (commit 2)

Youssef1313 · 2021-05-23T16:14:35Z

@AlekseyTs This is ready for another look.

AlekseyTs · 2021-05-24T17:08:57Z

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

@@ -727,6 +728,7 @@ private static int[] CreateSourceIndicesArray(int paramIndex, int parameterCount
            int argumentsCount,
            ref int argsConsumedCount,
            AttributeSyntax syntax,
+            ImmutableArray<int> argumentsToParams,
            BindingDiagnosticBag diagnostics)
        {
            int index = GetMatchingNamedConstructorArgumentIndex(parameter.Name, constructorArgumentNamesOpt, startIndex, argumentsCount);


GetMatchingNamedConstructorArgumentIndex

Do we still need this helper and the constructorArgumentNamesOpt if we now have argumentsToParams map? #Closed

AlekseyTs · 2021-05-24T17:18:24Z

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

@@ -819,6 +821,14 @@ private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, Attribu
                kind = TypedConstantKind.Primitive;
                defaultValue = ((ContextualAttributeBinder)this).AttributedMember.GetMemberCallerName();
            }
+            else if (!IsEarlyAttributeBinder && syntax.ArgumentList is not null &&
+                getCallerArgumentArgumentIndex(parameter, argumentsToParams) is int argumentIndex && argumentIndex > -1 && argumentIndex < syntax.ArgumentList.Arguments.Count)


syntax.ArgumentList.Arguments.Count

This looks suspicious. Isn't this going to include property/field initializers? Do we have tests for scenarios like that? #Closed

this going to include property/field initializers

Indeed, but it's probably that getCallerArgumentArgumentIndex won't return the index of a property/field initializer. But I'll give a deeper look and add more tests.

@AlekseyTs Fixed. Thanks!

AlekseyTs · 2021-05-27T21:08:06Z

@Youssef1313 It looks like there are legitimate test failures.

Youssef1313 · 2021-05-27T21:13:02Z

@AlekseyTs ~~When I quickly looked at it, I couldn't figure out what's going wrong. The line that sets defaultValue is setting it to the expected value correctly. I'll try to debug more deeply.~~ Fixed

AlekseyTs · 2021-05-27T21:54:23Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

+";
+
+            var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe);
+            // PROTOTYPE(caller-expr): This is inconsistent with the behavior of invocations, which doesn't show a lang version error.


PROTOTYPE(caller-expr): This is inconsistent with the behavior of invocations, which doesn't show a lang version error.

It feels like this isn't just a language version error. Are we certain the right default value is supplied for the parameter when the language version is "Preview"? #Closed

@AlekseyTs Since this is an error, nothing should be supplied. But since the error is extra, and only the warning should remain, the default value should be whatever CallerMemberName says.

Since this is an error, nothing should be supplied.

It is an error only when the target language version is C# 9. The error is supposed to go away when the target language version is Preview

@AlekseyTs That is how it is currently. But it shouldn't be an error in any version.

Per your recommendation on the first PR, we check language version when there is "actual binding" for it, which is not the case here.

Let me know if I'm misunderstanding something.

Let me know if I'm misunderstanding something.

I agree with you, the error shouldn't be there. I just find the PROTOTYPE comment somewhat incomplete. I believe the problem can be observed through a value used for the parameter in a success scenario. That raises severity of the issue in my opinion.

@AlekseyTs This brings another question. Is the existing behavior for CallerMemberName is correct?

It doesn't seem to work when the attribute is placed on classes. Is that by design, or a bug?

Assuming it's by design, I've fixed the CallerArgumentExpression case in a8ba7d1.

@AlekseyTs Note that Mono compiler have a different behavior for this CallerMemberName case.

https://onlinegdb.com/vponLAJ2g

It doesn't seem to work when the attribute is placed on classes. Is that by design, or a bug?

Hard to tell, I can see this both ways. It would be interesting to know what is the behavior of the native compiler (the last version of the compiler not based on Roslyn codebase). In any case, this feels like an ortogonal issue to what we are trying to do here, feel free to open a separate issue. I think the behavior around default value shouldn't change for this scenario.

It doesn't seem to work when the attribute is placed on classes. Is that by design, or a bug?

Hard to tell, I can see this both ways. It would be interesting to know what is the behavior of the native compiler (the last version of the compiler not based on Roslyn codebase). In any case, this feels like an ortogonal issue to what we are trying to do here, feel free to open a separate issue. I think the behavior around default value shouldn't change for this scenario.

➡️ #53757

AlekseyTs · 2021-05-27T22:05:23Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

+}
+";
+
+            var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe);


CreateCompilation

Default language version is a "moving target". If the intent to use C# 9, the test should be explicit about that. #Closed

AlekseyTs · 2021-05-27T22:09:33Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

+
+class MyAttribute : Attribute
+{
+    public MyAttribute(int a = 1, [CallerArgumentExpression(""a"")] int expr_a = 0)


[CallerArgumentExpression(""a"")]

Should we cover a scenario whan an attribute like that is applied in metadata? #Closed

@AlekseyTs The attribute definition is an error in this case, so how would I emit it as a metadata reference?

The attribute definition is an error in this case, so how would I emit it as a metadata reference?

The simplest way would be to test with IL source. See CreateCompilationWithIL helper for example.

@AlekseyTs Added the test, not sure if the current behavior is what you'd expect or not. Can you please take a look? Thanks!

AlekseyTs · 2021-05-27T22:13:06Z

Done with review pass (commit 14)

AlekseyTs · 2021-05-28T15:46:12Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

+";
+
+            var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular9);
+            // PROTOTYPE(caller-expr): This is inconsistent with the behavior of invocations, which doesn't show a lang version error.


// PROTOTYPE(caller-expr): This is inconsistent with the behavior of invocations, which doesn't show a lang version error.

Is the PROTOTYPE comment still relevant? #Closed

Deleted this outdated comment.

AlekseyTs · 2021-05-28T15:50:43Z

Done with review pass (commit 16)

AlekseyTs · 2021-05-28T21:26:55Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

+[My(1+2)]
+class Program
+{
+}";


We probably should use this code, execute it and observe behavior at runtime:

[My(1+2)] class Program { static void Main() { typeof(Program).GetCustomAttribute(typeof(MyAttribute)); } }

#Closed

AlekseyTs

LGTM (commit 17), with a test adjustment suggestion.

AlekseyTs · 2021-05-28T21:31:12Z

@dotnet/roslyn-compiler For the second review.

AlekseyTs · 2021-06-01T16:41:27Z

@dotnet/roslyn-compiler For the second review.

333fred

LGTM (commit 18)

333fred · 2021-06-01T17:49:30Z

I'll trigger a new pipeline when #53776 is merged into the branch, which should hopefully correct the integration failures.

333fred · 2021-06-01T18:11:38Z

/azp run

azure-pipelines · 2021-06-01T18:12:00Z

Azure Pipelines successfully started running 3 pipeline(s).

Handle CallerArgumentExpression in attributes

2b111de

Youssef1313 requested a review from a team as a code owner May 20, 2021 08:26

Youssef1313 marked this pull request as draft May 20, 2021 08:26

dotnet-issue-labeler bot added the Area-Compilers label May 20, 2021

Youssef1313 closed this May 20, 2021

Youssef1313 reopened this May 20, 2021

AlekseyTs added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 20, 2021

Add test

91d65f2

Youssef1313 mentioned this pull request May 20, 2021

Update Building, Debugging, and Testing on Windows.md #53543

Merged

Youssef1313 marked this pull request as ready for review May 20, 2021 16:12

AlekseyTs reviewed May 20, 2021

View reviewed changes

Feedback

c5c8b12

Youssef1313 requested a review from AlekseyTs May 20, 2021 19:18

Youssef1313 added 4 commits May 21, 2021 05:28

Address feedback

2a6ceb7

Check lang version, add failing test :/

a434aa5

Add test and fix using new approach

446bfd1

Fix test

d945e3c

AlekseyTs reviewed May 24, 2021

View reviewed changes

Youssef1313 added 2 commits May 24, 2021 22:55

Simplify code using argumentsToParams

0e19e35

Fix bug

0bce268

Youssef1313 added 2 commits May 25, 2021 23:21

Use Binder.GetSpecialType instead of Compilation.GetSpecialType

68c8078

More tests

146b609

Youssef1313 added 2 commits May 27, 2021 23:32

Fix :)

6131eb4

Feedback

c05caed

AlekseyTs reviewed May 27, 2021

View reviewed changes

Youssef1313 added 2 commits May 28, 2021 09:04

Explicitly set language version

4b3d333

Fix

a8ba7d1

AlekseyTs reviewed May 28, 2021

View reviewed changes

Youssef1313 mentioned this pull request May 28, 2021

CallerMemberName doesn't pass the class name to attribute constructor #53757

Closed

Feedback

d103a43

AlekseyTs reviewed May 28, 2021

View reviewed changes

AlekseyTs approved these changes May 28, 2021

View reviewed changes

Address feedback

2194bc9

333fred self-assigned this Jun 1, 2021

333fred approved these changes Jun 1, 2021

View reviewed changes

333fred enabled auto-merge (squash) June 1, 2021 18:14

333fred merged commit 3ff9727 into dotnet:features/caller-argument-expression Jun 1, 2021

Youssef1313 deleted the attribute-caller-arg-expr branch June 1, 2021 20:24

333fred added the Feature - CallerArgumentExpressionAttribute label Jul 19, 2021

Handle CallerArgumentExpression in attributes #53535

Handle CallerArgumentExpression in attributes #53535

Conversation

Youssef1313 commented May 20, 2021 • edited Loading

AlekseyTs commented May 20, 2021

AlekseyTs commented May 20, 2021 • edited Loading

Youssef1313 commented May 20, 2021

AlekseyTs May 20, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 20, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 20, 2021 • 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 May 20, 2021 • 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

Choose a reason for hiding this comment

Youssef1313 May 24, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 20, 2021

Youssef1313 commented May 23, 2021

AlekseyTs May 24, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 27, 2021

Youssef1313 commented May 27, 2021 • edited Loading

AlekseyTs May 27, 2021 • 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

Choose a reason for hiding this comment

Youssef1313 May 28, 2021 • 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 May 27, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 27, 2021 • 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 commented May 27, 2021

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 28, 2021

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 28, 2021

AlekseyTs commented Jun 1, 2021

333fred left a comment

Choose a reason for hiding this comment

333fred commented Jun 1, 2021

333fred commented Jun 1, 2021

azure-pipelines bot commented Jun 1, 2021

Youssef1313 commented May 20, 2021 •

edited

Loading

AlekseyTs commented May 20, 2021 •

edited

Loading

AlekseyTs May 20, 2021 •

edited

Loading

AlekseyTs May 20, 2021 •

edited

Loading

AlekseyTs May 20, 2021 •

edited

Loading

AlekseyTs May 20, 2021 •

edited

Loading

AlekseyTs May 20, 2021 •

edited

Loading

Youssef1313 May 24, 2021 •

edited

Loading

AlekseyTs May 24, 2021 •

edited

Loading

AlekseyTs May 24, 2021 •

edited

Loading

Youssef1313 commented May 27, 2021 •

edited

Loading

AlekseyTs May 27, 2021 •

edited

Loading

Youssef1313 May 28, 2021 •

edited

Loading

AlekseyTs May 27, 2021 •

edited

Loading

AlekseyTs May 27, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading