Feature: CallerArgumentExpressionAttribute by Youssef1313 · Pull Request #51952 · dotnet/roslyn

Youssef1313 · 2021-03-18T10:25:18Z

Proposal: dotnet/csharplang#287
Test plan: #52745

TODO:

~~Add a warning if the parameter name specified in the attribute is incorrect.~~ Done.

Open questions:

Do we need to support VB?
Do we need to add a warning for pre-C# 10 using this feature?

What should happen for the following case:

public static void M(
    [CallerMemberName] string callerName = "",
    [CallerArgumentExpression("callerName")] string argumentExp = "")
{
    Console.WriteLine(callerName);
    Console.WriteLine(argumentExp);
}

What should happen for the following case, should this have a warning?

M(); M("value");

public static void M(
    [CallerArgumentExpression("p")] string p = "")
{
    Console.WriteLine(p);
}

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

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

Youssef1313 · 2021-03-18T21:58:21Z

Ready for initial review.

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

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

333fred · 2021-03-19T17:41:17Z

@Youssef1313 I've created a feature branch for this work. Feel free to put your open questions as prototype comments in the source: the syntax is // PROTOTYPE(caller-expr): comment, so something similar. Just use the same name everywhere for the feature name. It may be a while before LDM can address the questions, so lets not block work on it.

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

333fred · 2021-03-19T20:31:46Z

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

                {
                    arguments.GetOrCreateData<ParameterEarlyWellKnownAttributeData>().HasCallerMemberNameAttribute = true;
                }
+                else if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.CallerArgumentExpressionAttribute))


As the language exists today we can do this as an early attribute, but if we do implement the nameof proposal (allowing nameof(parameter) in parameters/attributes on methods) I don't believe it will end well (this will cause a loop or fail to bind entirely). Please add a prototype comment to consider moving binding of the actual attribute data into regular attribute binding.

@333fred To make sure I understand this correctly, this is about nameof binding being done after we are trying to get the parameter name?

This also affects existing code, for example:

roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

Lines 898 to 912 in 8408750

if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.ConditionalAttribute))

{

boundAttribute = arguments.Binder.GetAttribute(arguments.AttributeSyntax, arguments.AttributeType, out hasAnyDiagnostics);

if (!boundAttribute.HasErrors)

{

string name = boundAttribute.GetConstructorArgument<string>(0, SpecialType.System_String);

arguments.GetOrCreateData<CommonTypeEarlyWellKnownAttributeData>().AddConditionalSymbol(name);

if (!hasAnyDiagnostics)

{

return boundAttribute;

}

}

return null;

}

To make sure I understand this correctly, this is about nameof binding being done after we are trying to get the parameter name?

I'm not sure I understand the question. My concern here is:

We're attempting to bind the parameter

The parameter has this attribute, and uses nameof on a parameter in this method (even itself!)

Attempting to bind the nameof argument causes us to bind the method, which causes us to bind the nameof, which causes us to bind the method, which... etc.

Today, the parameter is not in scope for that nameof, so our standard early attribute handling will work correctly without a chance for a loop. I'm not certain it will work correctly in the face of an updated nameof, however, so to do the right thing we'd want to move binding of the attribute argument to the standard binding location, which isn't susceptible to this problem.

I think I got the general idea here. I think (most likely) this will require fixes in other existing areas first (e.g, the ConditionalAttribute above), so I'll probably follow the same work and add tests.

AlekseyTs · 2021-04-06T14:50:26Z

src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs

                    ErrorCode.WRN_AnalyzerReferencesFramework,
                    ErrorCode.WRN_UnreadRecordParameter,
                    ErrorCode.WRN_DoNotCompareFunctionPointers,
+                    ErrorCode.WRN_CallerArgumentExpressionParamForUnconsumedLocation,


ErrorCode.WRN_CallerArgumentExpressionParamForUnconsumedLocation, [](start = 20, length = 65)

It looks like this test requires unnecessary maintenance. Perhap the check for the range of error codes at the beginning of the loop should be adjusted instead? Based on the comment only "Nullable-unrelated warnings in the C# 8 range should be added to this array." #Closed

@AlekseyTs I guess you meant this to be a "general" feedback on this test, rather than this PR specifically. Is that right?
Should I target main for that change in a separate PR, or file an issue for discussion?

I guess you meant this to be a "general" feedback on this test, rather than this PR specifically. Is that right?

It is specific to this PR because there are changes here.

Should I target main for that change in a separate PR, or file an issue for discussion?

I guess either way is fine as long as we either fix it in this PR, or make sure it will be fixed before merging the feature into main.

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

Sorry I initially misunderstood your comment. Fixed in 418ec88.

Youssef1313 · 2021-04-06T14:59:39Z

can you take another look?

It looks like the issue around an unnecessary bit flag in PEParameterSymbol hasn't been addressed.

Will give that a look 👀

AlekseyTs · 2021-04-06T15:00:45Z

Done with review pass (commit 39) #Closed

Youssef1313 · 2021-04-06T15:17:00Z

roslyn/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

Lines 110 to 115 in 7becc52

    
           public bool TryGetWellKnownAttribute(WellKnownAttributeFlags flag, out bool value) 
        
           { 
        
               int theBits = _bits; // Read this.bits once to ensure the consistency of the value and completion flags. 
        
               value = (theBits & ((int)flag << WellKnownAttributeDataOffset)) != 0; 
        
               return (theBits & ((int)flag << WellKnownAttributeCompletionFlagOffset)) != 0; 
        
           }

If I understand this method correctly, it looks like both flags are needed. One flag specifies whether we can get the attribute data or not, and the other specifies the actual data. Both SetWellKnownAttribute and TryGetWellKnownAttribute were built on the assumption that we have two bits for each attribute. Let me know if there is something I'm missing @AlekseyTs

AlekseyTs · 2021-04-06T15:31:17Z

If I understand this method correctly, it looks like both flags are needed. One flag specifies whether we can get the attribute data or not, and the other specifies the actual data. Both SetWellKnownAttribute and TryGetWellKnownAttribute were built on the assumption that we have two bits for each attribute. Let me know if there is something I'm missing

I believe that is not the issue I am referring to. See https://github.com/dotnet/roslyn/pull/51952/files#r600068668 #Closed

Youssef1313 · 2021-04-06T15:54:39Z

If I understand this method correctly, it looks like both flags are needed. One flag specifies whether we can get the attribute data or not, and the other specifies the actual data. Both SetWellKnownAttribute and TryGetWellKnownAttribute were built on the assumption that we have two bits for each attribute. Let me know if there is something I'm missing

I believe that is not the issue I am referring to. See https://github.com/dotnet/roslyn/pull/51952/files#r600068668

Thanks. Does b99bb7a address that?

AlekseyTs · 2021-04-06T16:20:36Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

            }
        }

+        private bool HasCallerArgumentExpressionAttribute


HasCallerArgumentExpressionAttribute [](start = 21, length = 36)

This property feels unnecessary. Also, it feels like we should be able to avoid looking for the attribute twice. First in this property and then to extract parameter name from it. #Closed

@AlekseyTs It feels much more simple if I didn't follow the existing pattern for other caller info, i.e, not even touch PackedFlags. I attempted to do that in e8023b1. Let me know if there are any concerns with the approach I took.

AlekseyTs · 2021-04-07T14:37:25Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

            }
        }

+        private int? _cachedCallerArgumentExpressionParameterIndex;


private int? _cachedCallerArgumentExpressionParameterIndex; [](start = 8, length = 59)

Please place the field declaration at the beginning of the class declaration, next to other fields. Also, we use "lazy" prefix for fields with delayed initialization. #Closed

AlekseyTs · 2021-04-07T14:40:50Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

+                bool isCallerArgumentExpression = !HasCallerLineNumberAttribute
+                    && !HasCallerFilePathAttribute
+                    && !HasCallerMemberNameAttribute
+                    && info.HasValue


&& info.HasValue [](start = 20, length = 16)

Is this check cheap? Consider doing it first. #Closed

AlekseyTs · 2021-04-07T14:42:17Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

+            {
+                if (_cachedCallerArgumentExpressionParameterIndex.HasValue)
+                {
+                    return _cachedCallerArgumentExpressionParameterIndex.Value;


Value [](start = 73, length = 5)

GetValueOrDefault? It avoids another HasValue check. #Closed

AlekseyTs · 2021-04-07T14:50:12Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

+                    {
+                        if (parameters[i].Name == parameterName)
+                        {
+                            _cachedCallerArgumentExpressionParameterIndex = i;


_cachedCallerArgumentExpressionParameterIndex = i; [](start = 28, length = 50)

This assignment is not guaranteed to be atomic, therefore, the implementation of this property is not thread safe. That is why we don't use Nullable<T> for "lazy" members. For example, we use ThreeState instead of bool?. It looks like we can use a simple int field with initial value (-2), for example, to indicate uninitialized state. #Closed

AlekseyTs · 2021-04-07T14:50:39Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

+                    }
+                }
+
+                _cachedCallerArgumentExpressionParameterIndex = -1;


_cachedCallerArgumentExpressionParameterIndex = -1; [](start = 16, length = 51)

Same comment #Closed

AlekseyTs · 2021-04-07T14:52:39Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

            private const int RefKindMask = 0x3;
-            private const int WellKnownAttributeDataMask = 0xFF;
+
+            private const int WellKnownAttributeDataMask = (0x1 << 8) - 1;


(0x1 << 8) - 1 [](start = 59, length = 14)

Consider reverting back to 0xFF literal. #Closed

AlekseyTs · 2021-04-07T15:02:59Z

Done with review pass (commit 43) #Closed

AlekseyTs

LGTM (commit 44)

AlekseyTs

LGTM (commit 44)

AlekseyTs · 2021-04-08T13:58:09Z

@333fred Please review the latest revision. Please squash commits during merging.

333fred

LGTM (commit 44)

333fred · 2021-04-10T00:38:08Z

Thanks @Youssef1313!

Youssef1313 · 2021-04-10T06:15:05Z

Thanks a lot for review @AlekseyTs and @333fred.

ghost added the Area-Compilers label Mar 18, 2021

Youssef1313 commented Mar 18, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs Outdated Show resolved Hide resolved

Youssef1313 force-pushed the features/caller-expression-info branch from 50a83c5 to cb0ab7d Compare March 18, 2021 10:29

Youssef1313 commented Mar 18, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs Outdated Show resolved Hide resolved

Youssef1313 added 4 commits March 18, 2021 20:20

Initial work for caller argument expression

15ea810

Add resources for caller argument expression diagnostics

b480dd5

Account for swapped named arguments

3ea7b29

Add few tests

28f460a

Youssef1313 force-pushed the features/caller-expression-info branch from 0cd335f to 28f460a Compare March 18, 2021 21:02

Complete implementation in PEParameterSymbol

8085998

Youssef1313 marked this pull request as ready for review March 18, 2021 21:58

Youssef1313 requested a review from a team as a code owner March 18, 2021 21:58

Youssef1313 added 3 commits March 18, 2021 22:30

Update tests

c1669d0

Produce warning for invalid parameter name

ac953bc

Run generate-compiler-code.cmd

cab0727

Youssef1313 commented Mar 19, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs Show resolved Hide resolved

Youssef1313 added 2 commits March 19, 2021 06:49

Fix out of range exception and add more tests

a2b2607

Extract DecodeCallerArgumentExpressionAttribute extension

1c49908

Youssef1313 commented Mar 19, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs Outdated Show resolved Hide resolved

Fix freeing twice

380edbc

333fred changed the base branch from main to features/caller-argument-expression March 19, 2021 17:34

333fred reviewed Mar 19, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs Outdated Show resolved Hide resolved

333fred reviewed Mar 19, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits March 19, 2021 18:35

Fix comments

6adaa54

Simplify PEParameterSymbol

c119c1b

Youssef1313 mentioned this pull request Mar 19, 2021

Simplify NotNullIfParameterNotNull implementation in PEParameterSymbol #51995

Merged

Fix NullableWarnings test

174d1ca

333fred reviewed Mar 19, 2021

View reviewed changes

AlekseyTs reviewed Apr 6, 2021

View reviewed changes

Adjust NullableWarnings test

418ec88

Address feedback

b99bb7a

AlekseyTs reviewed Apr 6, 2021

View reviewed changes

Youssef1313 added 2 commits April 6, 2021 17:11

Simplify

e8023b1

Fix formatting

3da9a8a

AlekseyTs reviewed Apr 7, 2021

View reviewed changes

Address feedback

3e68c55

Youssef1313 requested a review from AlekseyTs April 8, 2021 11:32

AlekseyTs reviewed Apr 8, 2021

View reviewed changes

AlekseyTs approved these changes Apr 8, 2021

View reviewed changes

333fred approved these changes Apr 10, 2021

View reviewed changes

333fred merged commit 52f6db1 into dotnet:features/caller-argument-expression Apr 10, 2021

Youssef1313 deleted the features/caller-expression-info branch April 10, 2021 06:14

Happypig375 mentioned this pull request May 4, 2021

Support [<CallerArgumentExpression>] fsharp/fslang-suggestions#966

Open

5 tasks

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

	if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.ConditionalAttribute))
	{
	boundAttribute = arguments.Binder.GetAttribute(arguments.AttributeSyntax, arguments.AttributeType, out hasAnyDiagnostics);
	if (!boundAttribute.HasErrors)
	{
	string name = boundAttribute.GetConstructorArgument<string>(0, SpecialType.System_String);
	arguments.GetOrCreateData<CommonTypeEarlyWellKnownAttributeData>().AddConditionalSymbol(name);
	if (!hasAnyDiagnostics)
	{
	return boundAttribute;
	}
	}

	return null;
	}

Conversation

Youssef1313 commented Mar 18, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Youssef1313 commented Mar 18, 2021

Uh oh!

Uh oh!

Uh oh!

333fred commented Mar 19, 2021

Uh oh!

Uh oh!

Uh oh!

333fred Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 20, 2021

Choose a reason for hiding this comment

Uh oh!

333fred Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2021 • 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!

Youssef1313 Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Apr 6, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs Apr 6, 2021 • 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!

Youssef1313 Apr 6, 2021 • 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 7, 2021 • 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 7, 2021 • 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 7, 2021 • 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 7, 2021 • 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 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Youssef1313 commented Mar 18, 2021 •

edited by jcouv

Loading

AlekseyTs Apr 6, 2021 •

edited

Loading

AlekseyTs commented Apr 6, 2021 •

edited

Loading

Youssef1313 commented Apr 6, 2021 •

edited

Loading

AlekseyTs commented Apr 6, 2021 •

edited

Loading

Youssef1313 commented Apr 6, 2021 •

edited

Loading

AlekseyTs Apr 6, 2021 •

edited

Loading

Youssef1313 Apr 6, 2021 •

edited

Loading

AlekseyTs Apr 7, 2021 •

edited

Loading

AlekseyTs Apr 7, 2021 •

edited

Loading

AlekseyTs Apr 7, 2021 •

edited

Loading

AlekseyTs Apr 7, 2021 •

edited

Loading

AlekseyTs Apr 7, 2021 •

edited

Loading

AlekseyTs Apr 7, 2021 •

edited

Loading

AlekseyTs commented Apr 7, 2021 •

edited

Loading