Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track GeneratedFilesOutputDirectory in the IDE #75311

Merged
merged 7 commits into from
Oct 18, 2024

Conversation

tmat
Copy link
Member

@tmat tmat commented Oct 1, 2024

Flows CompilerGeneratedFilesOutputPath msbuild property to compiler driver through Solution snapshot.

The compiler constructs the full path of a source generated file from the value of CompilerGeneratedFilesOutputPath msbuild property, if set, or from output path otherwise. This path is then used to generate metadata name of file-scoped types.

The IDE currently uses relative paths for source generated files. This causes compiler crashes during EnC since the metadata names generated by delta emit differ from those generated by the compiler during build. This change updates the IDE to use full paths for source generated files in the same way as the compiler does.

Fixes #51998, #72331

Public API:

TODO:

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 1, 2024
@dotnet-policy-service dotnet-policy-service bot added VSCode Needs API Review Needs to be reviewed by the API review council labels Oct 1, 2024
@tmat tmat force-pushed the GeneratedSourceDir branch 2 times, most recently from 6b0ff3f to 4e5a45d Compare October 4, 2024 02:04
@tmat tmat marked this pull request as ready for review October 4, 2024 16:16
@tmat tmat requested review from a team as code owners October 4, 2024 16:16
@tmat tmat marked this pull request as draft October 4, 2024 16:24
@tmat tmat marked this pull request as ready for review October 4, 2024 17:07
@tmat
Copy link
Member Author

tmat commented Oct 4, 2024

@dotnet/roslyn-ide @dotnet/roslyn-compiler @RikkiGibson @CyrusNajmabadi ptal

@tmat tmat force-pushed the GeneratedSourceDir branch from 1b135b1 to ac3feef Compare October 7, 2024 01:38
@jasonmalinowski
Copy link
Member

It's unclear to me why we're taking this approach in this PR, especially given this comment:

The compiler constructs the full path of a source generated file from the value of CompilerGeneratedFilesOutputPath msbuild property, if set, or from output path otherwise. This path is then used to generate metadata name of file-scoped types.

What if we just...didn't do that, and used the source-generator relative path for the metadata name being generated in either case? Would that be much simpler, and also avoid having to create several public APIs in the process?

(I recognize I'm out of the loop here, but it'd be nice if we can write down somewhere what alternate approaches were considered here.)

@tmat
Copy link
Member Author

tmat commented Oct 10, 2024

@RikkiGibson To comment on that. The change he's made in the compiler was mainly for interceptors which also use the file path: #71879

Using full paths for generated files solves other issues. See #51998. I don't see any drawback.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Oct 10, 2024

The latest design of interceptors, which we are pushing all generators to move to, doesn't actually use the file path. It uses the content checksum, and a file name for error reporting.

re: generated files having full paths in general: Behavior of code differs depending on file path in a number of scenarios. It seems like a good thing to always use the same full paths that "would be used when emitting the files to disk". It seems like it is a general preference that we have the ability to take those files emitted to disk, add them to compilation, then remove the associated generators, and get an equivalent result from compilation.

@tmat
Copy link
Member Author

tmat commented Oct 11, 2024

API review feedback incorporated.

@jaredpar
Copy link
Member

@jjonescz, @chsienki PTAL at compiler changes.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable. a s long as both APIs were approved, i'm ok with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Area-IDE Feature - File-Local Types File-local types (file types) Feature - Source Generators Source Generators Interactive-EnC Needs API Review Needs to be reviewed by the API review council untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source files produced by source generators should have absolute paths
8 participants