Support relative paths in interceptors by RikkiGibson · Pull Request #71879 · dotnet/roslyn

RikkiGibson · 2024-01-30T20:45:21Z

When no /generatedfilesout argument is specified, use the OutputDirectory (directory name of /out argument) as the base path for generated files. (Generated files are still only written to disk when /generatedfilesout is explicitly specified.)
Include the base path for generated files in the SyntaxTree.FilePath
Resolve interceptors file paths relative to the containing file
Removed diagnostic ERR_InterceptorPathNotInCompilationWithUnmappedCandidate. We don't want to suggest using a mapped path over a relative path, and in scenarios where files have absolute paths 99.9% of the time, we end up mapping the path after it is resolved relative to containing file, thus using an unmapped path ends up having the same effect as using a mapped path.

After this change we expect generators to start primarily using relative paths, but for compat, we want to continue accepting the same paths we have already been accepting in the preview, at least until everybody has a chance to move to relative paths.

RikkiGibson · 2024-02-13T00:52:51Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverOptions.cs


        public readonly bool TrackIncrementalGeneratorSteps;

+        internal string? BaseDirectory { get; init; }


It's possible that eventually we will want to make this API public to allow hosts outside the compiler to specify a value for it. However, I wanted to separate this decision from the initial work to get the "interceptors relative paths" scenarios working end-to-end.

Think it should be internal until a generator can articulate specific scearios which justify us making it public

RikkiGibson · 2024-02-13T23:46:48Z

@cston @jcouv Please take a look when you have the chance, thanks.

docs/features/interceptors.md

jaredpar · 2024-02-14T00:27:33Z

docs/features/interceptors.md

+1. A *mapped path* of each syntax tree is determined by applying [`/pathmap`](https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.commandlinearguments.pathmap?view=roslyn-dotnet-4.7.0) substitution to `SyntaxTree.FilePath`.
+2. For a given `[InterceptsLocation]` usage, the `filePath` argument value is compared to the *mapped path* of each syntax tree using ordinal string comparison. If exactly one syntax tree matches under this comparison, that is the *referenced syntax tree*.
+3. If no syntax tree matched by the above comparison, then the `filePath` argument value is resolved relative to path of the containing syntax tree of the `[InterceptsLocation]` usage. Pathmap substitution is applied to the result of this relative resolution. If exactly one syntax tree matches this resolved and mapped path, that is the *referenced syntax tree*.
+4. If neither of the above methods of comparison matched exactly one syntax tree, an error occurs.


Couldn't this be summarized as

The filePath argument is interepreted the same as a path for a #line directive in the same location?

If not in what ways are they different?

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

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

cston · 2024-02-14T01:22:08Z

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

+            }
+            """;
+
+        // parent directory of the drive root is just the drive root


parent directory of the drive root is just the drive root

According to the documentation for Directory.GetParent(), the return value is:

[...] null if path is the root directory

I will try and make this comment more clear that this about relative path resolution.

More illustrative example might be: SharpLab

cston · 2024-02-14T01:30:00Z

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

+    [Fact]
+    public void RelativePaths_08()
+    {
+        // Unmapped file paths are not absolute. Relative path resolution is not performed.


Unmapped file paths are not absolute. Relative path resolution is not performed

I don't understand this. It sounded like the goal of the change was to allow using relative paths. From the PR description:

After this change we expect generators to start primarily using relative paths [...]

Path in the attribute is often relative, but we won't actually be able to resolve it when the containing syntax tree does not have an absolute path.

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

jcouv

Done with review pass (iteration 10)

jcouv

LGTM Thanks (iteration 11)

jaredpar · 2024-02-23T01:17:15Z

docs/features/interceptors.md

-The file path given in the attribute must be equal by ordinal comparison to the value given by the above function.
-
-The compiler does not map `#line` directives when determining if an `[InterceptsLocation]` attribute intercepts a particular call in syntax.
+Note that `#line` directives are not considered when determining the call referenced by an `[InterceptsLocation]` attribute.


The doc still says that we consider mapped path here, thought we decided it would just match #line

The term mapped path is used in reference to /pathmap, not #line mapping. This sentence is meant to say that presence or absence of #line in source code doesn't affect what an InterceptsLocationAttribute means (aside from the trailing newline affecting the line numbers of the subsequent lines).

I think this is something which will become much simpler conceptually as we phase out the "compat" lookup. Basically, there will no longer be any expectation for the user of [InterceptsLocation] to perform any kind of mapping of the file path, line, or column; rather, to just get and use a relative path from the "current file" to the "referenced file", and to use the "unmapped" line and column numbers.

Point (2) still says that path map impacts how we resolve paths in InterceptsLocation. That means the following paths do not necessarily mean the same thing.

#line /src/blah/code.cs [Intercepts("/src/blah/code.cs")]

That feels very broken. Both of these features are about mapping a file path, relative or absolute, to another file in the compilation. Why should one use /pathmap and the other not use it?

If I'm incorrect and #line does respect path map for embedding I'm happy to be corrected on this point. If it does though why do we have such a complex section on how this mapping works. Why can't we just simply say:

The path inside InterceptsLocation is resolved the same way a #line would be

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

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

jaredpar · 2024-02-23T01:22:15Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverOptions.cs


        public readonly bool TrackIncrementalGeneratorSteps;

+        internal string? BaseDirectory { get; init; }


Think it should be internal until a generator can articulate specific scearios which justify us making it public

jaredpar · 2024-02-23T01:22:52Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverState.cs


        internal readonly SyntaxStore SyntaxStore;

+        private readonly GeneratorDriverOptions DriverOptions;


private fields should have the _driverOptions syntax

jaredpar · 2024-02-23T01:24:24Z

src/Compilers/Core/Portable/FileSystem/FileUtilities.cs

            }

-            string? normalizedPath = TryNormalizeAbsolutePath(resolvedPath);
+            string? normalizedPath = PathUtilities.IsAbsolute(resolvedPath) ? TryNormalizeAbsolutePath(resolvedPath) : null;


Why did you put the check here vs. putting the check inside TryNormalizeAbsolutePath? Feels like one case it should handle is if it's not passed an absolute path.

Reason I put it here was because it reduced the need to examine the other call sites in order to decide whether those call sites should "fail fast" when the input is not an absolute path, as they currently do, versus propagating a null result.

However, if it doesn't feel useful to preserve that "fail fast" behavior for the other call sites, I'm happy to push the check inside of TryNormalizeAbsolutePath and remove the debug assertion from within that method at the same time.

jaredpar · 2024-02-23T01:25:34Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

            ImmutableArray<AdditionalText> additionalTexts,
            DiagnosticBag generatorDiagnostics)
        {
+            Debug.Assert(baseDirectory is not null);


Think that we need a more descriptive name here. In the context of the compiler baseDirectory can have a variety of interpretations. Feel like we need to disambiguate this a bit.

jaredpar

The doc still says that we treat #line and InterceptsLocation paths differently which I don't understand. They should be the same for the new version of the attribute.

RikkiGibson · 2024-02-23T03:03:33Z

The doc still says that we treat #line and InterceptsLocation paths differently which I don't understand. They should be the same for the new version of the attribute.

Are you asking for the doc to clarify that the "compat" behavior in this PR is a temporary state of affairs? This PR doesn't implement support for [InterceptsLocation(string locationSpecifier)] nor introduce a specification for it.

…roslyn into generated-file-path

jaredpar · 2024-02-24T15:41:37Z

docs/features/interceptors.md

+`#line` directives are not considered when determining the call referenced by an `[InterceptsLocation]` attribute. In other words, the file path, line and column numbers used in `[InterceptsLocation]` are expected to refer to *unmapped* source locations.

-Temporarily, for compatibility purposes, we use the following rules:
+Temporarily, for compatibility purposes, when the initial matching strategy outlined above fails to match any syntax trees, we will fall back to a "compat" matching strategy which works in the following way:


Let's file a bug to track removing this work around.

jcouv · 2024-02-26T21:18:30Z

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

        internal OneOrMany<SyntaxTree> GetSyntaxTreesByMappedPath(string mappedPath)
        {
+            // This method supports a "compat" behavior for interceptor file path resolution.
+            // It must be removed prior to stable release.


nit: consider adding link to [https://github.com/dotnet/roslyn/issues/72265](https://github.com/dotnet/roslyn/issues/72265`) here as well

jcouv

LGTM Thanks (iteration 18)

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

cause i want to ;)

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 30, 2024

runfoapp bot mentioned this pull request Feb 2, 2024

invokenavigatetoforgeneratedfile_infolder failures #69686

Open

RikkiGibson changed the base branch from main to features/interceptors February 6, 2024 18:30

RikkiGibson force-pushed the generated-file-path branch from 916c290 to 7f9cce9 Compare February 12, 2024 19:09

RikkiGibson changed the title ~~Use /generatedfilesout in generated SyntaxTree.FilePath~~ Support relative paths in interceptors Feb 12, 2024

RikkiGibson changed the base branch from features/interceptors to main February 12, 2024 19:10

Resolve interceptor file path relative to containing file

26fe473

RikkiGibson force-pushed the generated-file-path branch from 7f9cce9 to 26fe473 Compare February 12, 2024 19:59

RikkiGibson added 2 commits February 12, 2024 13:13

unix adjustments

92927a8

unix adjustments

81a7a60

RikkiGibson commented Feb 13, 2024

View reviewed changes

RikkiGibson added 2 commits February 12, 2024 17:19

minor adjustments and add end to end tests

462ff45

Add end to end tests with pathmap

805bb13

RikkiGibson marked this pull request as ready for review February 13, 2024 01:32

RikkiGibson requested a review from a team as a code owner February 13, 2024 01:32

RikkiGibson requested review from cston and jcouv February 13, 2024 03:10

jaredpar reviewed Feb 14, 2024

View reviewed changes

cston reviewed Feb 14, 2024

View reviewed changes

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs Outdated Show resolved Hide resolved

cston reviewed Feb 14, 2024

View reviewed changes

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs Outdated Show resolved Hide resolved

cston reviewed Feb 14, 2024

View reviewed changes

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

cston reviewed Feb 14, 2024

View reviewed changes

jjonescz reviewed Feb 14, 2024

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated Show resolved Hide resolved

RikkiGibson added 2 commits February 14, 2024 10:54

remove unneeded usings

b7eafa7

Address feedback

7d11582

RikkiGibson requested review from cston and jaredpar February 14, 2024 19:37

jcouv reviewed Feb 20, 2024

View reviewed changes

Address feedback

826168f

cston approved these changes Feb 22, 2024

View reviewed changes

jcouv approved these changes Feb 22, 2024

View reviewed changes

fix unix test

cd0d5a7

jaredpar reviewed Feb 23, 2024

View reviewed changes

jaredpar previously requested changes Feb 23, 2024

View reviewed changes

RikkiGibson added 4 commits February 23, 2024 10:46

Clarify plan for file paths in doc

3681998

Merge branch 'generated-file-path' of https://github.com/RikkiGibson/…

f59f20a

…roslyn into generated-file-path

Do not perform pathmap substitution when normalizing relative paths

aa52b66

unix test baselines

b18f5a2

jaredpar reviewed Feb 24, 2024

View reviewed changes

RikkiGibson mentioned this pull request Feb 24, 2024

Remove compat workaround for interceptors file path lookup. #72265

Closed

RikkiGibson added 2 commits February 24, 2024 10:53

Add tracking issue

9dab4fa

Address feedback

82dc78a

RikkiGibson requested a review from jaredpar February 26, 2024 20:18

jcouv reviewed Feb 26, 2024

View reviewed changes

jcouv approved these changes Feb 26, 2024

View reviewed changes

cston reviewed Feb 26, 2024

View reviewed changes

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

RikkiGibson commented Feb 26, 2024

View reviewed changes

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

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

551cfc1

cston approved these changes Feb 26, 2024

View reviewed changes

RikkiGibson merged commit 1fae4e6 into dotnet:main Feb 27, 2024

RikkiGibson deleted the generated-file-path branch February 27, 2024 00:41

jjonescz added this to the 17.10 P2 milestone Feb 27, 2024

dotnet-bot mentioned this pull request Feb 29, 2024

[Automated] PRs inserted in VS build main-34628.19 #72320

Closed

tmat mentioned this pull request Oct 1, 2024

Source files produced by source generators should have absolute paths #51998

Closed

tmat mentioned this pull request Oct 10, 2024

Track GeneratedFilesOutputDirectory in the IDE #75311

Merged

1 task


		public readonly bool TrackIncrementalGeneratorSteps;

		internal string? BaseDirectory { get; init; }


		internal readonly SyntaxStore SyntaxStore;

		private readonly GeneratorDriverOptions DriverOptions;

Conversation

RikkiGibson commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Feb 13, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Feb 23, 2024 • 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!

jaredpar Feb 23, 2024 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RikkiGibson commented Jan 30, 2024 •

edited

Loading

RikkiGibson Feb 23, 2024 •

edited

Loading

jaredpar Feb 23, 2024 •

edited

Loading

RikkiGibson commented Feb 23, 2024 •

edited

Loading