Skip to content

Support incremental code generation in Visual Studio 2022#355

Closed
sharwell wants to merge 10 commits into
microsoft:mainfrom
sharwell:roslyn4
Closed

Support incremental code generation in Visual Studio 2022#355
sharwell wants to merge 10 commits into
microsoft:mainfrom
sharwell:roslyn4

Conversation

@sharwell

@sharwell sharwell commented Aug 9, 2021

Copy link
Copy Markdown
Contributor
  • Split Microsoft.Windows.CsWin32 into separate assemblies targeting Roslyn 3.8 and Roslyn 4.0
  • Modify MSBuild integration to use the Roslyn 4.0 assembly for design time builds inside Visual Studio 2022
  • Modify the Roslyn 4.0 source generator to implement IIncrementalGenerator instead of ISourceGenerator

@sharwell sharwell changed the title Support incremental code generation in Visual Studio 2022 🚧 Support incremental code generation in Visual Studio 2022 Aug 9, 2021
@sharwell sharwell force-pushed the roslyn4 branch 2 times, most recently from 56628fa to 16f0ba3 Compare August 9, 2021 17:42
Comment on lines +21 to +22
<ProjectReference Include="..\Microsoft.Windows.CsWin32\Roslyn38\Microsoft.Windows.CsWin32.Roslyn38.csproj" Private="true" ReferenceOutputAssembly="false" />
<ProjectReference Include="..\Microsoft.Windows.CsWin32\Roslyn40\Microsoft.Windows.CsWin32.Roslyn40.csproj" Private="true" ReferenceOutputAssembly="false" />

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 The Roslyn38 and Roslyn40 subfolders seemed the easiest way to achieve all of:

  1. Share version.json
  2. Not move settings.schema.json
  3. Not have 3 top-level folders for one multi-targeted assembly

/// <param name="parseOptions">The parse options that will be used for the generated code.</param>
public Generator(string metadataLibraryPath, string? apiDocsPath, GeneratorOptions? options = null, CSharpCompilation? compilation = null, CSharpParseOptions? parseOptions = null)
/// <param name="languageVersion">The language version that will be used for the generated code.</param>
public Generator(string metadataLibraryPath, string? apiDocsPath, GeneratorOptions? options = null, CSharpCompilation? compilation = null, LanguageVersion? languageVersion = null)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

📝 This pull request contains a few distinct examples of taking a value that frequently changes and extracting an intermediate incremental value which doesn't change as frequently. Breaking up compilation in the same manner could be a high-value win, but it's also significantly more work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The general approach for breaking up compilation would be:

  • Keep a map of source document → types of interest declared in that source document
  • Keep a map of metadata reference → types of interest declared in that metadata reference
  • Reference the maps in the code generation step instead of directly using Compilation

This approach means a change to one source file only requires a change to one value in the maps, and then a quick lookup during the incremental output step.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the idea. I'm puzzled by your removal of ParseOptions though as that should change infrequently. And isn't that where I'd find things like whether NRT is enabled? I don't use that yet but I may someday.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't use that yet but I may someday.

None of this is public API. If/when you need something new, feel free to add it. Until then, omitting it means the source generator won't trigger on changes to that item.

@sharwell sharwell changed the title 🚧 Support incremental code generation in Visual Studio 2022 Support incremental code generation in Visual Studio 2022 Aug 9, 2021
@azure-pipelines

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 355 in repo microsoft/CsWin32

@sharwell sharwell changed the base branch from main to 3rdparty August 10, 2021 18:36
@sharwell sharwell changed the base branch from 3rdparty to main August 10, 2021 18:36
@AArnott

AArnott commented Aug 19, 2021

Copy link
Copy Markdown
Member

I see you split out the Microsoft.Windows.CsWin32.props file into net20 and netstandard1.0 folders. How is that better than the model of just one directly in the build folder that we had before?

@AArnott

AArnott commented Aug 19, 2021

Copy link
Copy Markdown
Member

The build/netstandard1.0 folder should be renamed to build/netstandard1.1, because p/invoke was only first allowed in netstandard1.1.

@AArnott

AArnott commented Aug 19, 2021

Copy link
Copy Markdown
Member

The tools/install.ps1 script should be updated to look for the analyzer dll's in the new location, right?

@AArnott

AArnott commented Aug 19, 2021

Copy link
Copy Markdown
Member
            var inputs = context.CompilationProvider
                .Combine(languageVersion)
                .Combine(configFiles)
                .Combine(globalConfig)
                .Select((data, cancellationToken) => (compilation: data.Left.Left.Left, languageVersion: data.Left.Left.Right, additionalFiles: data.Left.Right, analyzerConfigOptions: data.Right));

Does this mean that the result will be reused so long as those inputs have not changed?
Our output can change from changes to any .cs file in the compilation. For example the user may copy and paste a generated type into their own project to modify it, and our SG immediately stops generating that type.

Comment thread src/Microsoft.Windows.CsWin32.Package/build/Microsoft.Windows.CsWin32.props Outdated
/// <param name="parseOptions">The parse options that will be used for the generated code.</param>
public Generator(string metadataLibraryPath, string? apiDocsPath, GeneratorOptions? options = null, CSharpCompilation? compilation = null, CSharpParseOptions? parseOptions = null)
/// <param name="languageVersion">The language version that will be used for the generated code.</param>
public Generator(string metadataLibraryPath, string? apiDocsPath, GeneratorOptions? options = null, CSharpCompilation? compilation = null, LanguageVersion? languageVersion = null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the idea. I'm puzzled by your removal of ParseOptions though as that should change infrequently. And isn't that where I'd find things like whether NRT is enabled? I don't use that yet but I may someday.

<AdditionalFiles Include="$(MSBuildThisFileDirectory)BannedSymbols.txt" />
</ItemGroup>
<ItemGroup>
<Compile Include="$(MSBuildThisFileDirectory)ArrayTypeHandleInfo.cs" />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really have to give up globbing for source files here?
What if instead of using Shared Projects, we simply used globbing in the two head projects to reach into a common directory?

@sharwell sharwell Dec 31, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMO this limitation is an unfortunate side effect of the only current recommendations for building projects with completely different dependency sets. It's been a while since I tried using globs in .projitems and it's possible it works now.

@AArnott

AArnott commented Aug 31, 2021

Copy link
Copy Markdown
Member

FYI I have checked out your branch and resolved merge conflicts. I can't push to your branch to share the work though.

@sharwell sharwell force-pushed the roslyn4 branch 2 times, most recently from 9c9eb1a to 7e924fe Compare December 31, 2021 02:39
@sharwell

This comment has been minimized.

@sharwell

Copy link
Copy Markdown
Contributor Author

Does this mean that the result will be reused so long as those inputs have not changed?

Yes, but one of the inputs is currently Compilation so this can be improved substantially. As a follow-up, you'll want to modify this to have some additional intermediate steps:

  1. The list of all expected types can be generated without looking at Compilation
  2. The output of (1) can be combined with Compilation to produce a map of expected type to true (needs to be generated) or false (already exists)
  3. The output of (2) can be fed into RegisterSourceOutput

This refactoring is quite complex due to the current state of the generator, but offers a significant benefit where (1) only needs to be executed if NativeMethods.txt or the input metadata changes, and (3) only needs to be executed if the inputs changed or an expected type was added/removed.

@sharwell sharwell marked this pull request as ready for review December 31, 2021 18:19
@AArnott AArnott mentioned this pull request Jul 23, 2022
6 tasks
@AArnott

AArnott commented Nov 11, 2022

Copy link
Copy Markdown
Member

There are surely some caching opportunities here, but I'll close this PR since in its present form it cannot be merged, and we aren't looking at caching across invocations right now.

@AArnott AArnott closed this Nov 11, 2022
AArnott pushed a commit that referenced this pull request Sep 5, 2025
…son-updates

Update Dockerfile and global.json updates to v9.0.201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants