Expect mapped paths in InterceptsLocationAttribute #68242

RikkiGibson · 2023-05-18T00:28:48Z

Test plan: #67421

PR primarily implements the following:

Add handling requirement for pathmap including feature doc change.
Add pathmap tests

Incidentally have also done:

Add tests for GetEnumerator/Dispose/Deconstruct
Add EndToEndTest (large number of interceptors)

The goal of this change is to eliminate machine-specific paths from source code, which is bad for portability and determinism.

Further discussion of the #line question can be found in dotnet/aspnetcore#47918 (comment)

RikkiGibson · 2023-05-22T22:46:25Z

src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs

@@ -318,5 +318,73 @@ static void runTest(int n)
                });
            }
        }
+
+        [Fact]
+        public void Interceptors()


This test runs in ~6 sec on my machine in Release mode (10000 interceptors)

6 secs seems like a significant amount of time, even for the large number of interceptors in the test. Is it possible there is some N^2 algorithm? If the number of interceptors is 1000 (1/10th), [update] how long does the test take?

With 1000 interceptors, the test takes about 1.3s, which I think indicates we don't have any dominant N^2 algorithm in the implementation.

When interceptors are not used (commenting out InterceptsLocationAttribute in tests) we get:

~1.1s for 1000 non-interceptors
~3.8s for 10000 non-interceptors

Compare (again) with the interceptors case:

~1.3s for 1000 interceptors
~6s for 10000 interceptors

I think there's a noticeable cost incurred for very large numbers of interceptors here, but on an order that I think we can accept for an initial release.

Thanks for the additional testing. I also ran this test with 3x interceptors and the elapsed time was ~2.5x.

Not related to this PR: It would be good to look at a PerfView trace to get a better sense of where the additional cost comes from and whether it could be improved.

RikkiGibson · 2023-05-23T21:35:25Z

docs/features/interceptors.md

+
+string GetInterceptorFilePath(SyntaxTree tree, Compilation compilation)
+{
+    return compilation.Options.SourceReferenceResolver?.NormalizePath(tree.FilePath, baseFilePath: null) ?? tree.FilePath;


Note that if this feature makes it past the experimental phase, we might revisit providing "convenience public APIs". For the current release, it feels more appropriate to indicate what helpers the generator author themselves needs to include.

cston · 2023-05-26T23:34:02Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -1037,6 +1037,34 @@ internal override int GetSyntaxTreeOrdinal(SyntaxTree tree)
            }
        }

+        private ImmutableSegmentedDictionary<string, OneOrMany<SyntaxTree>> _mappedPathToSyntaxTree;


_mappedPathToSyntaxTree

Consider moving field declaration ahead of the method declarations. #Resolved

cston · 2023-05-26T23:35:16Z

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

@@ -2201,7 +2201,7 @@ internal enum ErrorCode
        ERR_InterceptorLineOutOfRange = 27005,
        ERR_InterceptorCharacterOutOfRange = 27006,
        ERR_InterceptorSignatureMismatch = 27007,
-        ERR_InterceptableMethodMustBeOrdinary = 27008,


ERR_InterceptableMethodMustBeOrdinary

Is this error code no longer used? Can we remove the resource string as well? #Resolved

cston · 2023-05-26T23:36:49Z

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

-            // At that time, we should look at caching the resolved paths for the trees in a set (or maybe a Map<string, OneOrMany<SyntaxTree>>).
-            // so we can reduce the cost of these checks.
-            foreach (var tree in syntaxTrees)
+            var referenceResolver = DeclaringCompilation.Options.SourceReferenceResolver;


referenceResolver

Can this declaration be moved inside if (matchingTrees...) { ... }?

cston · 2023-05-27T00:06:40Z

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

+        comp.VerifyEmitDiagnostics(PlatformInformation.IsWindows
+                // C:\My\Machine\Specific\Path\Program.cs(11,25): error CS27008: Cannot intercept: Path 'C:\My\Machine\Specific\Path\Program.cs' is unmapped. Expected mapped path '/_/Program.cs'.
+                //     [InterceptsLocation(@"C:\My\Machine\Specific\Path\Program.cs", 5, 3)]
+                ? Diagnostic(ErrorCode.ERR_InterceptorPathNotInCompilationWithUnmappedCandidate, @"@""C:\My\Machine\Specific\Path\Program.cs""").WithArguments(@"C:\My\Machine\Specific\Path\Program.cs", "/_/Program.cs").WithLocation(11, 25)


@"@""C:\My\Machine\Specific\Path\Program.cs""").WithArguments(@"C:\My\Machine\Specific\Path\Program.cs"

Could we avoid checking IsWindows with $"\"{path}\"").WithArguments(path)?

RikkiGibson · 2023-06-08T17:55:14Z

I wrote up a description of why we are going with respecting /pathmap but not #line and where that puts us for .NET 8. Notably, we don't reach a point of hand-authorability just by respecting /pathmap here.

https://gist.github.com/RikkiGibson/36f6650f4897a1d7cf372343647dcbb4

jcouv · 2023-06-08T19:32:09Z

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

            {
-                if (tree.FilePath == filePath)
+                var referenceResolver = DeclaringCompilation.Options.SourceReferenceResolver;


nit: consider moving closer to usage #Closed

Because it's used in each subsequent branch in this block, I felt more comfortable declaring it where it is currently, instead of after the statement declaring unmappedMatch, for example.

I'm line-breaking some of the diagnostics.Add calls to make the usages easier to spot.

jcouv · 2023-06-08T19:36:55Z

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

+            """;
+
+        var pathPrefix1 = PlatformInformation.IsWindows ? """C:\My\Machine\Specific\Path1\""" : "/My/Machine/Specific/Path1/";
+        var pathPrefix2 = PlatformInformation.IsWindows ? """C:\My\Machine\Specific\Path1\""" : "/My/Machine/Specific/Path2/";


Should this be "Path2"? #Closed

jcouv

Done with review pass (iteration 5)

jcouv

LGTM Thanks (iteration 8)

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 18, 2023

RikkiGibson force-pushed the ic-6 branch 2 times, most recently from a076045 to 8b4caff Compare May 18, 2023 20:07

build-analysis bot mentioned this pull request May 18, 2023

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

RikkiGibson force-pushed the ic-6 branch from 71913a6 to c3c24fd Compare May 22, 2023 19:40

RikkiGibson commented May 22, 2023

View reviewed changes

RikkiGibson added 3 commits May 23, 2023 14:02

Expect mapped paths in InterceptableAttribute

8eb5ff6

Add end to end test

6803108

Test GetEnumerator/Dispose/Deconstruct

ec5bd43

RikkiGibson force-pushed the ic-6 branch from 490e51e to ec5bd43 Compare May 23, 2023 21:03

RikkiGibson marked this pull request as ready for review May 23, 2023 21:32

RikkiGibson requested a review from a team as a code owner May 23, 2023 21:32

RikkiGibson commented May 23, 2023

View reviewed changes

RikkiGibson requested review from cston and jcouv May 23, 2023 22:34

jcouv self-assigned this May 24, 2023

Fix spelling of prototype comment

43ee0bb

cston reviewed May 26, 2023

View reviewed changes

cston reviewed May 27, 2023

View reviewed changes

cston approved these changes May 27, 2023

View reviewed changes

Address feedback

21fc324

cston approved these changes May 30, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/features/interceptors' into ic-6

13b6964

jcouv reviewed Jun 8, 2023

View reviewed changes

jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 8, 2023

jcouv added this to the C# 12.0 milestone Jun 8, 2023

RikkiGibson added 2 commits June 8, 2023 20:06

Address feedback

88fc265

Fix feature flag name

7c64b8a

RikkiGibson requested a review from jcouv June 9, 2023 16:21

jcouv approved these changes Jun 9, 2023

View reviewed changes

RikkiGibson merged commit d9a4149 into dotnet:features/interceptors Jun 9, 2023
26 checks passed

RikkiGibson deleted the ic-6 branch June 9, 2023 19:55

allisonchou mentioned this pull request Jun 16, 2023

[Automated] PRs inserted in VS build main-33816.152 #68663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expect mapped paths in InterceptsLocationAttribute #68242

Expect mapped paths in InterceptsLocationAttribute #68242

RikkiGibson commented May 18, 2023 •

edited

RikkiGibson May 22, 2023 •

edited

cston May 27, 2023 •

edited

RikkiGibson May 30, 2023

RikkiGibson May 30, 2023 •

edited

cston May 31, 2023

jcouv Jun 8, 2023

RikkiGibson May 23, 2023

cston May 26, 2023 •

edited by jcouv

cston May 26, 2023 •

edited by jcouv

cston May 26, 2023

cston May 27, 2023

RikkiGibson commented Jun 8, 2023

jcouv Jun 8, 2023 •

edited

RikkiGibson Jun 9, 2023

jcouv Jun 8, 2023 •

edited

jcouv left a comment

jcouv left a comment

Expect mapped paths in InterceptsLocationAttribute #68242

Expect mapped paths in InterceptsLocationAttribute #68242

Conversation

RikkiGibson commented May 18, 2023 • edited

RikkiGibson May 22, 2023 • edited

Choose a reason for hiding this comment

cston May 27, 2023 • edited

Choose a reason for hiding this comment

RikkiGibson May 30, 2023

Choose a reason for hiding this comment

RikkiGibson May 30, 2023 • edited

Choose a reason for hiding this comment

cston May 31, 2023

Choose a reason for hiding this comment

jcouv Jun 8, 2023

Choose a reason for hiding this comment

RikkiGibson May 23, 2023

Choose a reason for hiding this comment

cston May 26, 2023 • edited by jcouv

Choose a reason for hiding this comment

cston May 26, 2023 • edited by jcouv

Choose a reason for hiding this comment

cston May 26, 2023

Choose a reason for hiding this comment

cston May 27, 2023

Choose a reason for hiding this comment

RikkiGibson commented Jun 8, 2023

jcouv Jun 8, 2023 • edited

Choose a reason for hiding this comment

RikkiGibson Jun 9, 2023

Choose a reason for hiding this comment

jcouv Jun 8, 2023 • edited

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson commented May 18, 2023 •

edited

RikkiGibson May 22, 2023 •

edited

cston May 27, 2023 •

edited

RikkiGibson May 30, 2023 •

edited

cston May 26, 2023 •

edited by jcouv

cston May 26, 2023 •

edited by jcouv

jcouv Jun 8, 2023 •

edited

jcouv Jun 8, 2023 •

edited