Make SourceClonedParameterSymbol abstract by Youssef1313 · Pull Request #54355 · dotnet/roslyn

Youssef1313 · 2021-06-24T04:42:30Z

~~Builds on #54132 (I'll rebase once the other PR goes in).~~ I think there is no need to wait on the other PR. Going to rebase on the current feature branch.

@AlekseyTs Let me know if you want the other PR to contain everything.

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

Youssef1313 · 2021-06-25T14:00:23Z

@AlekseyTs This is ready for review. Thanks!

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs

AlekseyTs · 2021-06-25T18:13:05Z

Done with review pass (commit 1)

Youssef1313 · 2021-06-25T21:17:07Z

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs

+
+        internal override bool IsCallerMemberName => _originalParam.IsCallerMemberName;
+
+        internal override int CallerArgumentExpressionParameterIndex => throw ExceptionUtilities.Unreachable;


@AlekseyTs The created parameter may not be optional as you stated in the other PR. However, I think this is still unreachable. We attempt to bind default arguments only if overload resolution succeeds.

If overload resolution succeeds, then an explicit value was passed to the parameter, meaning we are not going to access this property (I'm open to adding any test cases you'd like if the existing tests are not enough). #Resolved

If overload resolution succeeds, then an explicit value was passed to the parameter, meaning we are not going to access this property.

That maybe true for EndEnvoke (I assume because parameters are ref/out/in), but why this is true for BeginInvoke is not obvious to me.

I'm open to adding any test cases you'd like if the existing tests are not enough

I think we should have targeted test for Begin/EndInvoke invocations for scenarios when cloned parameters are optional and have the attribute on them.

Both BeginInvoke and EndInvoke has extra parameter(s) at the end (AsyncCallBack and object for BeginInvoke, and IAsyncResult for EndInvoke). If overload resolution succeeds, then these parameters must have been supplied, meaning all the previous parameters has explicitly been passed.

I'll add some tests.

Both BeginInvoke and EndInvoke has extra parameter(s) at the end (AsyncCallBack and object for BeginInvoke, and IAsyncResult for EndInvoke). If overload resolution succeeds, then these parameters must have been supplied, meaning all the previous parameters has explicitly been passed.

I see now why this is wrong.

Youssef1313 · 2021-06-28T05:14:28Z

@AlekseyTs This is ready for another look. Thanks!

AlekseyTs · 2021-06-28T15:26:22Z

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

    public class AttributeTests_CallerInfoAttributes : WellKnownAttributesTestBase
    {
+        [ConditionalFact(typeof(CoreClrOnly))]
+        public void TestBeginInvoke()


TestBeginInvoke

Do we have similar tests for EndInvoke? Even though they are likely going to be error scenarios, we want to make sure that binding doesn't fail in an unexpected way (a crash/assert, etc. ) #Closed

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs

AlekseyTs · 2021-06-28T15:30:59Z

Done with review pass (commit 2)

…ssion' into caller-arg-cloned-param

AlekseyTs · 2021-06-29T12:53:14Z

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

+class Program
+{
+    const string s1 = nameof(s1);
+    delegate void D(ref string s1, [CallerArgumentExpression(s1)] [Optional] [DefaultParameterValue(""default"")] string s2);


[CallerArgumentExpression(s1)] [Optional] [DefaultParameterValue(""default"")] string s2

The interesting scenario is when this parameter is getting cloned and the one that it references isn't and probably follows it. Is this the case in this unit-test. We want to test scenario where simply reusing the index from the original parameter will be observably wrong. #Closed

The interesting scenario is when this parameter is getting cloned and the one that it references isn't

For the parameter to get cloned, it must be ref if I understand correctly. ref parameters seem to have a false IsOptional. Hence, the test won't hit calling CallerArgumentExpressionIndex. This is regardless we're in error case or not.

I think the test you're asking for should hit CallerArgumentExpressionIndex. I'm unable to find a test case that fulfills all the following:

The CallerArgumentExpression parameter is cloned.

The parameter it refers to isn't cloned.

CallerArgumentExpressionIndex gets hit.

I added a test that fulfills 1 and 2.

For the parameter to get cloned, it must be ref if I understand correctly. ref parameters seem to have a false IsOptional. Hence, the test won't hit calling CallerArgumentExpressionIndex. This is regardless we're in error case or not.

We still can have a test attempting to do that. Trying all possible ways marking the parameter optional, attempting to ommit it at the call-site. It is fine if we will be unable to hit the API. We still need tests that attempt to do that.

@AlekseyTs I added a test in 25dd0ff, and added two more now in 8ee88e0. Let me know if there are any other scenarios :)

AlekseyTs · 2021-06-29T12:54:56Z

Done with review pass (commit 6)

AlekseyTs · 2021-07-01T14:03:14Z

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

+class Program
+{
+    const string s2 = nameof(s2);
+    delegate void D([CallerArgumentExpression(s2)] [Optional] [DefaultParameterValue(""default"")] ref string s1, string s2);


s2

Let's add a couple of more parameters to the end and change this arttribute to refer to the last parameter. The goal is to make the correspnding index to go out range of ordinals of parameters in EndInvoke. #Closed

AlekseyTs · 2021-07-01T14:21:31Z

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

+    {
+        D d = M;
+        string s = string.Empty;
+        d.EndInvoke(ref s, null);


d.EndInvoke(ref s, null);

Are we omitting the argument for the "interesting" parameter (parameter that was clned from the one with CallerArgumentExpression attribute)? #Resolved

AlekseyTs · 2021-07-01T14:22:35Z

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

+    {
+        D d = M;
+        string s = string.Empty;
+        d.EndInvoke(ref s, null);


d.EndInvoke(ref s, null);

Are we omitting the argument for the "interesting" parameter (parameter that was clned from the one with CallerArgumentExpression attribute)? #Resolved

AlekseyTs · 2021-07-01T14:28:34Z

Done with review pass (commit 10)

Youssef1313 · 2021-07-03T19:12:15Z

.tools\msbuild\16.8.0-preview2.1\tools\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(10,5): error : (NETCORE_ENGINEERING_TELEMETRY=Restore) Failed to download package 'Microsoft.VisualStudio.Shell.Interop.14.0.DesignTime.16.10.31225.38' from 'https://pkgs.dev.azure.com/azure-public/3ccf6661-f8ce-4e8a-bb2e-eff943ddd3c7/_packaging/2a239fd0-3e21-40b0-b9d6-bc122fec7eb2/nuget/v3/flat2/microsoft.visualstudio.shell.interop.14.0.designtime/16.10.31225.38/microsoft.visualstudio.shell.interop.14.0.designtime.16.10.31225.38.nupkg'.

Youssef1313 · 2021-07-06T19:32:04Z

Pinging @AlekseyTs and @333fred for review. Thanks!

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs

333fred · 2021-07-07T20:15:44Z

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

+";
+
+            var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
+            // Begin/EndInvoke are not currently supported.


supported

Do you mean at runtime? Supported where?

@333fred No, I meant "supported by the compiler". ie, the compiler doesn't do anything for caller argument expressions for cloned begin/end invoke parameters.

333fred · 2021-07-07T20:31:31Z

Done review pass (commit 11). Only a few small comments for clarifications.

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs

…nedParameterSymbolForBeginAndEndInvoke.cs

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs

…nedParameterSymbolForBeginAndEndInvoke.cs

333fred

LGTM (commit 13). @dotnet/roslyn-compiler for a second review.

Youssef1313 · 2021-07-11T08:27:44Z

@jcouv Can you review please? Thanks!

Youssef1313 · 2021-07-13T16:54:28Z

Ping @jcouv

jcouv · 2021-07-13T20:19:34Z

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs

+        {
+        }
+
+        internal override bool IsCallerFilePath => _originalParam.IsCallerFilePath;


I didn't understand why properties that are implemented the same way in SourceDelegateClonedParameterSymbolForBeginAndEndInvoke and SourcePropertyClonedParameterSymbolForAccessors were moved out of the base type.

@jcouv This was requested by @AlekseyTs. The reasoning is to force new types to implement it. i.e, avoid future mistakes if a new type should implement it another way.

jcouv · 2021-07-13T20:29:14Z

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

+  IL_001e:  ret
+}
+");
+        }


It looks like the added tests cover delegate scenarios. Do we already have scenarios for properties/indexers, since that's another scenario that uses SourceClonedParameterSymbol?

@jcouv There is a test for indexer:

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

Line 1685 in 63531dc

public void TestIndexers()

Let me know if you want to extend the tests with any scenarios.

Awesome, thanks

jcouv

Done with review pass (iteration 13). Just a couple of questions

jcouv · 2021-07-13T20:31:20Z

Also it occurred to me that we'll want to think of possible interactions between CallerArgumentExpression and interpolated string handlers.

jcouv

LGTM Thanks (iteration 13)

jcouv · 2021-07-13T20:38:34Z

possible interactions between CallerArgumentExpression and interpolated string handlers.

Clarified from chat with Fred. Here's possible scenario (may just fall out):
What if I put CallerArgumentExpression on a parameter of handler constructor, then use the argument attribute to copy an argument from invocation and pass it to that constructor?

ghost added the Area-Compilers label Jun 24, 2021

Youssef1313 mentioned this pull request Jun 24, 2021

VB support for CallerArgumentExpression #54132

Merged

Make SourceClonedParameterSymbol abstract

55b6ebe

Youssef1313 force-pushed the caller-arg-cloned-param branch from 5e45818 to 55b6ebe Compare June 25, 2021 13:59

Youssef1313 marked this pull request as ready for review June 25, 2021 13:59

Youssef1313 requested a review from a team as a code owner June 25, 2021 13:59

AlekseyTs reviewed Jun 25, 2021

View reviewed changes

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs Show resolved Hide resolved

Youssef1313 commented Jun 25, 2021

View reviewed changes

Youssef1313 requested a review from AlekseyTs June 26, 2021 17:37

Fix implementation and add tests

034ef63

AlekseyTs reviewed Jun 28, 2021

View reviewed changes

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 28, 2021

View reviewed changes

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 28, 2021

View reviewed changes

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs Outdated Show resolved Hide resolved

Return -1 for now

5bc7e09

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 29, 2021

Youssef1313 added 3 commits June 29, 2021 07:02

Merge remote-tracking branch 'upstream/features/caller-argument-expre…

62f83f1

…ssion' into caller-arg-cloned-param

Add test

a53e766

Delete PROTOTYPE comment

502df54

AlekseyTs reviewed Jun 29, 2021

View reviewed changes

Youssef1313 added 2 commits June 30, 2021 17:31

Add test

25dd0ff

Two more tests

8ee88e0

AlekseyTs reviewed Jul 1, 2021

View reviewed changes

Youssef1313 added 2 commits July 1, 2021 16:07

Address feedback

e14db71

Fix diagnostic comment

0701455

AlekseyTs reviewed Jul 1, 2021

View reviewed changes

Address requested test change

d0fdc95

Youssef1313 closed this Jul 3, 2021

Youssef1313 reopened this Jul 3, 2021

Youssef1313 requested a review from AlekseyTs July 3, 2021 19:12

333fred reviewed Jul 7, 2021

View reviewed changes

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs Show resolved Hide resolved

333fred reviewed Jul 7, 2021

View reviewed changes

Youssef1313 commented Jul 7, 2021

View reviewed changes

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateClo…

447c1ba

…nedParameterSymbolForBeginAndEndInvoke.cs

333fred reviewed Jul 7, 2021

View reviewed changes

...rs/CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateClo…

63531dc

…nedParameterSymbolForBeginAndEndInvoke.cs

333fred approved these changes Jul 7, 2021

View reviewed changes

jcouv self-assigned this Jul 13, 2021

jcouv reviewed Jul 13, 2021

View reviewed changes

jcouv approved these changes Jul 13, 2021

View reviewed changes

jcouv merged commit b688438 into dotnet:features/caller-argument-expression Jul 13, 2021

Youssef1313 deleted the caller-arg-cloned-param branch July 13, 2021 20:38

Youssef1313 mentioned this pull request Jul 13, 2021

Test plan for CallerArgumentExpressionAttribute #52745

Closed

46 tasks

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


		internal override bool IsCallerMemberName => _originalParam.IsCallerMemberName;

		internal override int CallerArgumentExpressionParameterIndex => throw ExceptionUtilities.Unreachable;

Conversation

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

Uh oh!

Uh oh!

Youssef1313 commented Jun 25, 2021

Uh oh!

Uh oh!

AlekseyTs commented Jun 25, 2021

Uh oh!

Youssef1313 Jun 25, 2021 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Jun 28, 2021

Uh oh!

AlekseyTs Jun 28, 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!

Uh oh!

Uh oh!

Uh oh!

AlekseyTs commented Jun 28, 2021

Uh oh!

AlekseyTs Jun 29, 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 Jun 30, 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 Jun 30, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 29, 2021

Uh oh!

AlekseyTs Jul 1, 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 Jul 1, 2021 • edited by 333fred 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 Jul 1, 2021 • edited by 333fred 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 commented Jul 1, 2021

Uh oh!

Youssef1313 commented Jul 3, 2021

Uh oh!

Youssef1313 commented Jul 6, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented Jul 7, 2021

Uh oh!

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Jul 11, 2021

Uh oh!

Youssef1313 commented Jul 13, 2021

Uh oh!

Youssef1313 commented Jun 24, 2021 •

edited

Loading

Youssef1313 Jun 25, 2021 •

edited by 333fred

Loading

AlekseyTs Jun 28, 2021 •

edited

Loading

AlekseyTs Jun 29, 2021 •

edited

Loading

Youssef1313 Jun 30, 2021 •

edited

Loading

AlekseyTs Jun 30, 2021 •

edited

Loading

AlekseyTs Jul 1, 2021 •

edited

Loading

AlekseyTs Jul 1, 2021 •

edited by 333fred

Loading

AlekseyTs Jul 1, 2021 •

edited by 333fred

Loading