Skip to content

Source generators/fix exception latching#59930

Merged
chsienki merged 4 commits intodotnet:mainfrom
chsienki:source-generators/fix-exception-latching
Mar 17, 2022
Merged

Source generators/fix exception latching#59930
chsienki merged 4 commits intodotnet:mainfrom
chsienki:source-generators/fix-exception-latching

Conversation

@chsienki
Copy link
Copy Markdown
Member

@chsienki chsienki commented Mar 3, 2022

Fixes #58625

We were incorrectly skipping generators when they had an assigned exception, meaning they would never recover once one occurred. After switching the logic it became clear that we were incorrectly losing initialization state (that wasn't exposed because we never recovered), which is fixed by altering the way we store state. I also cleaned up the unused GeneratorInfo that was left over from V1.

- Remove unsused info
- Ensure we don't drop init info between results + errors
- Don't run if we have no output nodes
@chsienki chsienki requested a review from a team as a code owner March 3, 2022 19:29
@chsienki
Copy link
Copy Markdown
Member Author

chsienki commented Mar 3, 2022

@dotnet/roslyn-compiler for review please :)

}
}

internal class CallbackHolder
Copy link
Copy Markdown
Contributor

@cston cston Mar 3, 2022

Choose a reason for hiding this comment

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

CallbackHolder

Why introduce the level of indirection rather than defining these fields on GeneratorInitializationContext directly? Are we passing around a CallbackHolder instance or copying the containing struct in some cases?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The struct get passed to ISourceGenerator.Initialize where the generator then calls the set methods. Previously we had a builder type that held onto them, but given that that's no longer used I just moved them here. Its ugly but its just for back compat.

? new GeneratorState(generatorState.Info, postInitSources, inputNodes, outputNodes)
: SetGeneratorException(MessageProvider, generatorState, sourceGenerator, ex, diagnosticsBag, isInit: true);
? new GeneratorState(postInitSources, inputNodes, outputNodes)
: SetGeneratorException(MessageProvider, GeneratorState.Empty, sourceGenerator, ex, diagnosticsBag, isInit: true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GeneratorState.Empty

It's not clear why this changed. Is it because generatorState is equivalent to GeneratorState.Empty when !Initialized?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. If the initialization fails there is no state, and we don't want to try and do anything with it in the future.

{
var generatorState = stateBuilder[i];
if (generatorState.Exception is object)
if (generatorState.OutputNodes.Length == 0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if (generatorState.OutputNodes.Length == 0)

Is this the key change in the PR?

Copy link
Copy Markdown
Member Author

@chsienki chsienki Mar 9, 2022

Choose a reason for hiding this comment

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

Yes. That then led me to discover we were throwing away initialization state when we set the exception. It didn't matter before because we never recovered, but we now need to handle it correctly.

@chsienki
Copy link
Copy Markdown
Member Author

chsienki commented Mar 9, 2022

@dotnet/roslyn-compiler for reviews please :)

@chsienki
Copy link
Copy Markdown
Member Author

ping @dotnet/roslyn-compiler for a second review.

@RikkiGibson RikkiGibson self-assigned this Mar 14, 2022
@chsienki chsienki enabled auto-merge (squash) March 15, 2022 21:34
@chsienki chsienki merged commit 385c5f1 into dotnet:main Mar 17, 2022
@ghost ghost added this to the Next milestone Mar 17, 2022
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incremental source generators do not recover from an exception

4 participants