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.

Sign up for GitHub

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

Jump to bottom

Ensure an empty run result doesn't throw when generators are present. #74034

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Ensure an empty run result doesn't throw when generators are present. #74034

merged 4 commits into from
Jun 26, 2024

Conversation

chsienki
Copy link
Contributor

Fixes #74033

@chsienki chsienki requested a review from a team as a code owner June 17, 2024 19:06
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 17, 2024
@chsienki
Copy link
Contributor Author

@dotnet/roslyn-compiler for reviews please :)

RikkiGibson
RikkiGibson reviewed Jun 17, 2024
View reviewed changes
src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs Outdated
@@ -39,7 +39,7 @@ internal GeneratorDriver(GeneratorDriverState state)
internal GeneratorDriver(ParseOptions parseOptions, ImmutableArray<ISourceGenerator> generators, AnalyzerConfigOptionsProvider optionsProvider, ImmutableArray<AdditionalText> additionalTexts, GeneratorDriverOptions driverOptions)
{
var incrementalGenerators = GetIncrementalGenerators(generators, SourceExtension);
_state = new GeneratorDriverState(parseOptions, optionsProvider, generators, incrementalGenerators, additionalTexts, ImmutableArray.Create(new GeneratorState[generators.Length]), DriverStateTable.Empty, SyntaxStore.Empty, driverOptions, runtime: TimeSpan.Zero, parseOptionsChanged: true);
_state = new GeneratorDriverState(parseOptions, optionsProvider, generators, incrementalGenerators, additionalTexts, generators.SelectAsArray(g => GeneratorState.Empty), DriverStateTable.Empty, SyntaxStore.Empty, driverOptions, runtime: TimeSpan.Zero, parseOptionsChanged: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain how this change prevents an NRE which was occurring in the original code?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing it's in getGeneratorSources access to generatorState.PostInitTrees.Length. The value for PostInitTrees is a default immutable array for default(GeneratorState), as opposed to an empty immutable array for GeneratorState.Empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When calling .GetRunResult() we call into a local function getGeneratorSources that creates an array to handle the generated sources of the given generator

roslyn/src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

Line 174 in b8320e1

ArrayBuilder<GeneratedSourceResult> sources = ArrayBuilder<GeneratedSourceResult>.GetInstance(generatorState.PostInitTrees.Length + generatorState.GeneratedTrees.Length);

That calls into the collections of the individual states to get their sizes. Prior to the change, we created default GeneratorStates, so the PostInitTrees and GeneratedTrees were uninitialized immutable arrays which throw when attempting to get their length.

Instead we now store GeneratorState.Empty instances, which in turn have empty result arrays rather than uninitialized ones.

We had a test that checked you could call GetRunResult before calling RunGenerators but that test didn't add any generators, so the generatorStates array was empty. Adding a generator ensures that array has content, and we see the issue in the added test.

@333fred
Copy link
Member

333fred commented Jun 17, 2024

So, the change itself looks fine to me, but there are a lot of failures in CI here. Is something not merging correctly?

@chsienki
Copy link
Contributor Author

Test failures seem weird and unrelated, kicked off a re-run and will investigate further if we still see it.

@333fred
Copy link
Member

333fred commented Jun 18, 2024

Test failures seem weird and unrelated, kicked off a re-run and will investigate further if we still see it.

Seems like some investigation may be required.

@jaredpar
Copy link
Member

src\Compilers\Core\Portable\CommandLine\AnalyzerConfig.cs(30,38): error CS8795: (NETCORE_ENGINEERING_TELEMETRY=Build) Partial method 'AnalyzerConfig.GetSectionMatcherRegex()' must have an implementation part because it has accessibility modifiers.

All the bootstrap legs broke with this message. Guessing something fairly fundamental about generators broke with this change.

@sharwell
Copy link
Member

Yeah, GeneratorState.Initialized is false for default(GeneratorState) but true for GeneratorState.Empty.

@chsienki
Split into empty and unitialized
2bf0845 
- Fix replace generators not running init for new generators and add a test
@chsienki
Copy link
Contributor Author

Turns out a bunch of generator tests were failing; they were just hiding amongst the mass of other issues that I didn't see them.

@dotnet/roslyn-compiler for reviews please :)

@jaredpar jaredpar added this to the 17.11 milestone Jun 21, 2024
@chsienki
Copy link
Contributor Author

@dotnet/roslyn-compiler for review please :)

@jaredpar
Copy link
Member

@333fred, @RikkiGibson PTAL

@chsienki chsienki merged commit 1c83ce1 into dotnet:main Jun 26, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot modified the milestones: 17.11, Next Jun 26, 2024
chsienki added a commit to chsienki/roslyn that referenced this pull request Jun 26, 2024
@chsienki
Ensure an empty run result doesn't throw when generators are present. (…
dce053d 
…dotnet#74034)

* Ensure an empty run result doesn't throw when generators are present.
* Fix replace generators not running init for new generators and add a test
@chsienki chsienki mentioned this pull request Jun 26, 2024
Merged
jjonescz pushed a commit that referenced this pull request Jun 28, 2024
@chsienki
Ensure an empty run result doesn't throw when generators are present. (…
af40227 
…#74034) (#74174)

* Ensure an empty run result doesn't throw when generators are present.
* Fix replace generators not running init for new generators and add a test
This was referenced Jul 4, 2024
Closed
Closed
@chsienki chsienki mentioned this pull request Jul 23, 2024
Merged
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharwell sharwell sharwell left review comments

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@333fred 333fred 333fred approved these changes

@jaredpar jaredpar jaredpar approved these changes

Assignees

@333fred 333fred

@RikkiGibson RikkiGibson

Labels
Area-Compilers Bug New Feature - Source Generators Source Generators untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.12 P1
Development

Successfully merging this pull request may close these issues.

GeneratorDriver.GetRunResult() should not throw
5 participants
@chsienki @333fred @jaredpar @sharwell @RikkiGibson