Support intercepting invocation within conditional access #72998

RikkiGibson · 2024-04-12T19:56:33Z

RikkiGibson · 2024-04-15T19:08:38Z

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

@@ -7199,4 +7199,173 @@ static void Main()
        Assert.Equal("(7,9)", locationSpecifier.GetDisplayLocation());
        AssertEx.Equal("""[global::System.Runtime.CompilerServices.InterceptsLocationAttribute(1, "jB4qgCy292LkEGCwmD+R6FIAAAA=")]""", locationSpecifier.GetInterceptsLocationAttributeSyntax());
    }
+
+    [Fact]
+    public void ConditionalAccess_01()


Should probably also test cond-access on a nullable value type.

Have to punt out the extension method scenario for this. It's another "receiver is not a variable" case (where calling the method directly in source would be an error). #71657 (comment)

RikkiGibson · 2024-04-15T21:24:20Z

@cston @jcouv for review of a small fix

cston · 2024-04-15T22:17:55Z

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

@@ -1039,11 +1039,13 @@ private void DecodeInterceptsLocationChecksumBased(DecodeWellKnownAttributeArgum
            switch (referencedToken)
            {
                case { Parent: SimpleNameSyntax { Parent: MemberAccessExpressionSyntax { Parent: InvocationExpressionSyntax } memberAccess } rhs } when memberAccess.Name == rhs:
+                case { Parent: SimpleNameSyntax { Parent: MemberBindingExpressionSyntax { Parent: InvocationExpressionSyntax } memberBinding } rhs1 } when memberBinding.Name == rhs1:


case

Is this case redundant given the { Parent: SimpleNameSyntax { Parent: MemberBindingExpressionSyntax memberBinding } rhs1 } when memberBinding.Name == rhs1 case below? #Resolved

No, the case below is for an error reporting path (the MemberBinding's parent is not an Invocation).

docs/features/interceptors.md

cston · 2024-04-15T22:27:46Z

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

@@ -1039,11 +1039,13 @@ private void DecodeInterceptsLocationChecksumBased(DecodeWellKnownAttributeArgum
            switch (referencedToken)
            {
                case { Parent: SimpleNameSyntax { Parent: MemberAccessExpressionSyntax { Parent: InvocationExpressionSyntax } memberAccess } rhs } when memberAccess.Name == rhs:


case { Parent: SimpleNameSyntax { Parent: MemberAccessExpressionSyntax

Is this case redundant given the { Parent: SimpleNameSyntax { Parent: MemberAccessExpressionSyntax memberAccess } rhs } when memberAccess.Name == rhs case below?

No, the case below is for an error reporting path (the MemberAccess's parent is not an Invocation).

cston · 2024-04-15T22:38:59Z

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

+            }
+            """, "Interceptors.cs", RegularWithInterceptors);
+
+        var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "1");


expectedOutput: "1"

Consider testing intercepting with c == null as well.

static void Main() { F(new C()); F(null); } static void F(C c) { c?.M(); }

jcouv · 2024-04-15T22:47:50Z

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

+                static void Main()
+                {
+                    var c = new C();
+                    c?.M();


It would be good for tests on new interceptable locations to also verify the GetInterceptorMethod API #Closed

jcouv · 2024-04-15T22:56:43Z

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

+            static class Interceptors
+            {
+                {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}}
+                public static void M1(this C? c) => Console.Write(1);


C?

Do we need the ? annotation on C here? That gives M1 a different signature than M.
nit: consider also verifying that M receives the proper values and is not invoked in the null case. #Closed

jcouv · 2024-04-15T22:58:39Z

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

+        comp = CreateCompilation([source, interceptors, s_attributesTree]);
+        comp.VerifyEmitDiagnostics(
+            // Interceptors.cs(6,6): error CS9151: Possible method name 'P' cannot be intercepted because it is not being invoked.
+            //     [global::System.Runtime.CompilerServices.InterceptsLocationAttribute(1, "q2jDXUSFcU71GJHh7313cHEAAABQcm9ncmFtLmNz")]


[global::System.Runtime.CompilerServices.InterceptsLocationAttribute(1, "q2jDXUSFcU71GJHh7313cHEAAABQcm9ncmFtLmNz")]

Not related to this PR: Have we considered making GetInterceptsLocationAttributeSyntax output a comment with a human-readable location as you suggested source generators should do when writing an [InterceptsLocation(...)] attribute?

It's possible, but for output which is most appropriate over multiple lines, it seems better to avoid having to handle indentation and so on. (even if generator author doesn't care about unnecessary name qualifiers, they probably care about reasonable indentation and whitespace.)

To avoid indentation problems, the comment could be at the end of the line: [global::System.Runtime.CompilerServices.InterceptsLocationAttribute(1, "abc")] // file.cs(1, 2) or even using /* file.cs(1, 2) */ comment

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

jcouv

Done with review pass (iteration 3)

jcouv · 2024-04-16T18:39:29Z

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

+            verify: Verification.Fails,
+            expectedOutput: "1");
+        verifier.VerifyDiagnostics();
+    }


Consider checking GetInterceptorMethod on pointer accesses too

jcouv

LGTM Thanks (iteration 5) with a small test suggestion to consider

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 12, 2024

RikkiGibson changed the title ~~Interceptors cond access~~ Support intercepting invocation within conditional access Apr 12, 2024

Support intercepting invocation within conditional access

d77473e

RikkiGibson force-pushed the interceptors-cond-access branch from 9e7a1e9 to d77473e Compare April 15, 2024 19:08

RikkiGibson commented Apr 15, 2024

View reviewed changes

RikkiGibson marked this pull request as ready for review April 15, 2024 20:56

RikkiGibson requested a review from a team as a code owner April 15, 2024 20:56

Add nullable value type test

e87391e

update doc

c860e8e

cston reviewed Apr 15, 2024

View reviewed changes

docs/features/interceptors.md Outdated Show resolved Hide resolved

cston reviewed Apr 15, 2024

View reviewed changes

Update docs/features/interceptors.md

d82f784

cston reviewed Apr 15, 2024

View reviewed changes

cston approved these changes Apr 15, 2024

View reviewed changes

jcouv reviewed Apr 15, 2024

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs Outdated Show resolved Hide resolved

jcouv reviewed Apr 15, 2024

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs Show resolved Hide resolved

jcouv reviewed Apr 15, 2024

View reviewed changes

jcouv self-assigned this Apr 15, 2024

Address feedback

21fc194

RikkiGibson requested a review from jcouv April 15, 2024 23:34

cston approved these changes Apr 15, 2024

View reviewed changes

jcouv reviewed Apr 16, 2024

View reviewed changes

jcouv approved these changes Apr 16, 2024

View reviewed changes

Test GetInterceptorMethod

249520a

RikkiGibson enabled auto-merge (squash) April 16, 2024 19:35

RikkiGibson merged commit e8bb458 into dotnet:main Apr 16, 2024
24 checks passed

dotnet-policy-service bot added this to the Next milestone Apr 16, 2024

RikkiGibson deleted the interceptors-cond-access branch April 16, 2024 21:42

This was referenced Apr 20, 2024

[Automated] PRs inserted in VS build main-34819.162 #73114

Closed

[Automated] PRs inserted in VS build feature.debugger.main-34823.102 #73204

Closed

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support intercepting invocation within conditional access #72998

Support intercepting invocation within conditional access #72998

RikkiGibson commented Apr 12, 2024 •

edited

RikkiGibson Apr 15, 2024

RikkiGibson Apr 15, 2024 •

edited

RikkiGibson commented Apr 15, 2024

cston Apr 15, 2024 •

edited by jcouv

RikkiGibson Apr 15, 2024 •

edited

cston Apr 15, 2024

RikkiGibson Apr 15, 2024 •

edited

cston Apr 15, 2024

jcouv Apr 15, 2024 •

edited

jcouv Apr 15, 2024 •

edited

jcouv Apr 15, 2024

RikkiGibson Apr 15, 2024

jcouv Apr 16, 2024

jcouv left a comment

jcouv Apr 16, 2024

jcouv left a comment

Support intercepting invocation within conditional access #72998

Support intercepting invocation within conditional access #72998

Conversation

RikkiGibson commented Apr 12, 2024 • edited

RikkiGibson Apr 15, 2024

Choose a reason for hiding this comment

RikkiGibson Apr 15, 2024 • edited

Choose a reason for hiding this comment

RikkiGibson commented Apr 15, 2024

cston Apr 15, 2024 • edited by jcouv

Choose a reason for hiding this comment

RikkiGibson Apr 15, 2024 • edited

Choose a reason for hiding this comment

cston Apr 15, 2024

Choose a reason for hiding this comment

RikkiGibson Apr 15, 2024 • edited

Choose a reason for hiding this comment

cston Apr 15, 2024

Choose a reason for hiding this comment

jcouv Apr 15, 2024 • edited

Choose a reason for hiding this comment

jcouv Apr 15, 2024 • edited

Choose a reason for hiding this comment

jcouv Apr 15, 2024

Choose a reason for hiding this comment

RikkiGibson Apr 15, 2024

Choose a reason for hiding this comment

jcouv Apr 16, 2024

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Apr 16, 2024

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson commented Apr 12, 2024 •

edited

RikkiGibson Apr 15, 2024 •

edited

cston Apr 15, 2024 •

edited by jcouv

RikkiGibson Apr 15, 2024 •

edited

RikkiGibson Apr 15, 2024 •

edited

jcouv Apr 15, 2024 •

edited

jcouv Apr 15, 2024 •

edited