Get CallerArgumentExpression ready #54839

Youssef1313 · 2021-07-15T09:57:51Z

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

@333fred @jcouv for review.

src/Compilers/VisualBasic/Portable/Parser/ParserFeature.vb

jcouv

LGTM Thanks (iteration 3)

Youssef1313 · 2021-07-18T10:24:19Z

Ping @333fred for review.

333fred · 2021-07-19T21:31:17Z

We went through the compiler test plan review today. Overall, it looks good, but we'd like to see some tests around the "Compiler generated calls with user-written code" bullet before we merge to main. @Youssef1313 do you think you can add them to this PR? Test plan is here: #52745

333fred · 2021-07-19T21:32:53Z

src/Compilers/VisualBasic/Portable/Parser/ParserFeature.vb

@@ -108,7 +108,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
                    Return LanguageVersion.VisualBasic16_9

                Case Feature.CallerArgumentExpression


Is this actually checked anywhere?

@333fred If you mean tests, there is TestGoodCallerArgumentExpressionAttribute_Version16_9 that tests old version. If you mean in product code, it's checked here:

roslyn/src/Compilers/VisualBasic/Portable/Binding/Binder_Invocation.vb

Lines 3180 to 3183 in db43364

InternalSyntax.Parser.CheckFeatureAvailability(diagnostics,

argumentSyntax.Location,

DirectCast(argumentSyntax.SyntaxTree.Options, VisualBasicParseOptions).LanguageVersion,

InternalSyntax.Feature.CallerArgumentExpression)

Youssef1313 · 2021-07-20T18:24:45Z

We went through the compiler test plan review today. Overall, it looks good, but we'd like to see some tests around the "Compiler generated calls with user-written code" bullet before we merge to main. @Youssef1313 do you think you can add them to this PR? Test plan is here: #52745

Yes I'll add them in this PR.

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

Youssef1313 · 2021-07-22T09:21:17Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceParameterSymbol.vb

@@ -266,6 +267,14 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols

        Friend Overrides Iterator Function GetCustomAttributesToEmit(compilationState As ModuleCompilationState) As IEnumerable(Of VisualBasicAttributeData)
            Dim attributes = MyBase.GetCustomAttributesToEmit(compilationState)
+
+            If Not Location.SourceTree.Options.Features.ContainsKey(InternalSyntax.GetFeatureFlag(InternalSyntax.Feature.CallerArgumentExpression)) Then


@333fred It looks like SourceTree can be null. I'm not sure what to do for the null case, allow the feature to work or emit the attributes as is?

We should emit the attribute as is in those cases. I imagine most those will be synthesized parameters, and thus not likely to have this attribute in the first place?

333fred · 2021-07-22T17:10:28Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_CallerArgumentExpression.vb

-        Log(123)
-            ~~~
-</expected>)
+            Dim compilation = CreateCompilation(source, targetFramework:=TargetFramework.NetCoreApp, references:={Net451.MicrosoftVisualBasic}, options:=TestOptions.ReleaseExe, parseOptions:=TestOptions.Regular16_9.WithFeature("CallerArgumentExpression"))


Rather than modifying this test to succeed, let's leave the feature flag off and verify the behavior when it's not present.

333fred

Only a couple of small things left. We're really close!

333fred

LGTM (commit 10). Just one thing to remove the unshipped api txt file. @jcouv, please take another look.

jcouv · 2021-07-22T22:21:38Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_CallerArgumentExpression.vb

@@ -233,12 +233,12 @@ End Module
        </file>
    </compilation>

-            Dim compilation = CreateCompilationWithCustomILSource(source, il, options:=TestOptions.ReleaseExe, includeVbRuntime:=True, parseOptions:=TestOptions.RegularLatest)
+            Dim compilation = CreateCompilationWithCustomILSource(source, il, options:=TestOptions.ReleaseExe, includeVbRuntime:=True, parseOptions:=TestOptions.RegularLatest.WithFeature("CallerArgumentExpression"))


nit: it may have been good to have a test with feature flag and an earlier version, to illustrate the the feature works on old language versions

jcouv

LGTM Thanks (iteration 10)

jcouv · 2021-07-22T22:25:37Z

@333fred How do you want to track the removal of feature flag? We can either use the test plan (keep it open for now) or open a separate issue.

333fred · 2021-07-22T22:27:16Z

@333fred How do you want to track the removal of feature flag? We can either use the test plan (keep it open for now) or open a separate issue.

I'd say let's use the test plan.

Edit: I've updated the test plan with a bullet.

jcouv · 2021-07-22T22:35:58Z

Correctness leg fails with:

src\Compilers\VisualBasic\Portable\PublicAPI.Unshipped.txt(2,1): error RS0017: (NETCORE_ENGINEERING_TELEMETRY=Build) Symbol 'Microsoft.CodeAnalysis.VisualBasic.LanguageVersion.VisualBasic17 = 1700 -> Microsoft.CodeAnalysis.VisualBasic.LanguageVersion' is part of the declared API, but is either not public or could not be found

jcouv · 2021-07-23T03:53:18Z

Merged/squashed. Thanks @Youssef1313

Youssef1313 · 2021-07-23T08:35:29Z

@jcouv @333fred Is the feature ready to merge into main?

333fred · 2021-07-23T16:44:45Z

@jcouv @333fred Is the feature ready to merge into main?

Yep. I'll be working on that today/Monday.

Get CallerArgumentExpression ready

5ae48be

Youssef1313 requested a review from a team as a code owner July 15, 2021 09:57

dotnet-issue-labeler bot added the Area-Compilers label Jul 15, 2021

Youssef1313 added 2 commits July 15, 2021 12:00

Compact error messages

baebf82

Small fixes

db43364

Youssef1313 commented Jul 15, 2021

View reviewed changes

src/Compilers/VisualBasic/Portable/Parser/ParserFeature.vb Outdated Show resolved Hide resolved

Youssef1313 closed this Jul 15, 2021

Youssef1313 reopened this Jul 15, 2021

Youssef1313 closed this Jul 15, 2021

Youssef1313 reopened this Jul 15, 2021

jcouv approved these changes Jul 15, 2021

View reviewed changes

333fred reviewed Jul 19, 2021

View reviewed changes

jcouv self-assigned this Jul 20, 2021

Youssef1313 added 2 commits July 21, 2021 11:42

Add more tests. TODO: remaining tests

5ebabce

Fix test

b28c9ed

Youssef1313 commented Jul 21, 2021

View reviewed changes

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs Show resolved Hide resolved

Youssef1313 added 3 commits July 21, 2021 15:02

Add test for base ctor call

2c02869

VB feature flag

10f0998

Don't require C# 10 for caller argument expression

f22bb4c

Youssef1313 commented Jul 22, 2021

View reviewed changes

333fred reviewed Jul 22, 2021

View reviewed changes

Youssef1313 added 2 commits July 22, 2021 19:52

Small fix and address feedback

2609534

Fix NRE

5cdfcb4

333fred approved these changes Jul 22, 2021

View reviewed changes

jcouv reviewed Jul 22, 2021

View reviewed changes

jcouv approved these changes Jul 22, 2021

View reviewed changes

Address feedback

7bd4716

jcouv merged commit faecd02 into dotnet:features/caller-argument-expression Jul 23, 2021

Youssef1313 deleted the final-edits branch July 23, 2021 06:33

03l54rd1n3 mentioned this pull request Jul 27, 2021

Add support for 'CallerArgumentExpression' attribute safakgur/guard#58

Closed

runfoapp bot mentioned this pull request Aug 16, 2021

SimpleCovariantReturnEndToEndTest is stack overflowing in CI #53361

Closed

benfoster mentioned this pull request Sep 10, 2021

Add support for CallerArgumentExpression attribute benfoster/o9d-guard#55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get CallerArgumentExpression ready #54839

Get CallerArgumentExpression ready #54839

Youssef1313 commented Jul 15, 2021 •

edited

Loading

jcouv left a comment

Youssef1313 commented Jul 18, 2021

333fred commented Jul 19, 2021 •

edited

Loading

333fred Jul 19, 2021

Youssef1313 Jul 20, 2021

Youssef1313 commented Jul 20, 2021

Youssef1313 Jul 22, 2021

333fred Jul 22, 2021

333fred Jul 22, 2021

333fred left a comment

333fred left a comment

jcouv Jul 22, 2021

jcouv left a comment

jcouv commented Jul 22, 2021

333fred commented Jul 22, 2021 •

edited

Loading

jcouv commented Jul 22, 2021

jcouv commented Jul 23, 2021

Youssef1313 commented Jul 23, 2021

333fred commented Jul 23, 2021

		@@ -108,7 +108,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
		Return LanguageVersion.VisualBasic16_9

		Case Feature.CallerArgumentExpression

	InternalSyntax.Parser.CheckFeatureAvailability(diagnostics,
	argumentSyntax.Location,
	DirectCast(argumentSyntax.SyntaxTree.Options, VisualBasicParseOptions).LanguageVersion,
	InternalSyntax.Feature.CallerArgumentExpression)

Get CallerArgumentExpression ready #54839

Get CallerArgumentExpression ready #54839

Conversation

Youssef1313 commented Jul 15, 2021 • edited Loading

jcouv left a comment

Choose a reason for hiding this comment

Youssef1313 commented Jul 18, 2021

333fred commented Jul 19, 2021 • edited Loading

333fred Jul 19, 2021

Choose a reason for hiding this comment

Youssef1313 Jul 20, 2021

Choose a reason for hiding this comment

Youssef1313 commented Jul 20, 2021

Youssef1313 Jul 22, 2021

Choose a reason for hiding this comment

333fred Jul 22, 2021

Choose a reason for hiding this comment

333fred Jul 22, 2021

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

jcouv Jul 22, 2021

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Jul 22, 2021

333fred commented Jul 22, 2021 • edited Loading

jcouv commented Jul 22, 2021

jcouv commented Jul 23, 2021

Youssef1313 commented Jul 23, 2021

333fred commented Jul 23, 2021

Youssef1313 commented Jul 15, 2021 •

edited

Loading

333fred commented Jul 19, 2021 •

edited

Loading

333fred commented Jul 22, 2021 •

edited

Loading