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

[release/8.0] Make config binding gen incremental (#89587) #92730

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Sep 27, 2023

Backport of #89587 to release/8.0.
Fixes #83534.

Customer Impact

This change ensures that during IDE development, after emitting source to honor an initial trigger, the generator won’t re-emit code given subsequent edits (i.e every keystroke) that do not change the effective input to the generator.

This is important for large solutions (numerous projects and/or source files) that have code that would trigger the generator to participate in compilation.

See sample code that showcase scenarios where customers would observe this impact.

Testing

New tests theoretically prove that given the same effective input, the generator produces the same parsed model which prevents regeneration.

A variety of sanity check and core scenario validation tests have been added to verify that we regenerate source only when needed.

There are no regressions to existing unit and functional tests.

Risk

Overall .NET 8 product

Low. Mostly a contained fix for an off-by-default component.

There are minimal changes to the System.Text.Json source generator; only to share generator helper components: diagnostic handling helpers and a utility collection type to facilitate creating incremental models.

Source generator

Medium

To facilitate incremental generation, parsing logic has been non-trivially modified to produce equatable models. No change to codified/already tested functionality but there might be subtle differences that might regress unknown scenarios.

The changes introduce intentional but minimal diffs in a small number of emitted-source regression baselines. We’ve determined that the changes are valid, but any diff indicates that generated binding logic might change for untested scenarios, which might cause functionality regressions.

Now that this final v1 feature-size change is complete, we’ll switch gears to extensively testing more scenarios and ensure upstream product integration (e.g. ASP.NET; regression projects for final products, working with .NET fundamentals team). As always, we’ll address any reported scenarios quickly.

* Make config binding gen incremental

* Iterate on implementation

* Add incremental tests & driver

* Make incremental tests pass and revert functional regression

* Address failing tests

* Make tests pass

* Suppress diagnostic

* Address feedback on diag info creation

* Refactor member access expr parsing to indicate assumptions

* Address feedback & do misc clean up

* Adjust model to minimize baseline diff / misc clean up
@layomia layomia added Servicing-consider Issue for next servicing release review area-Extensions-Configuration labels Sep 27, 2023
@layomia layomia added this to the 8.0.0 milestone Sep 27, 2023
@layomia layomia self-assigned this Sep 27, 2023
@ghost
Copy link

ghost commented Sep 27, 2023

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #89587 to release/8.0.
Fixes #83534.

Customer Impact

This change ensures that during IDE development, after emitting source to honor an initial trigger, the generator won’t re-emit code given subsequent edits (i.e every keystroke) that do not change the effective input to the generator.

This is important for large solutions (numerous projects and/or source files) that have code that would trigger the generator to participate in compilation.

See sample code that showcase scenarios where customers would observe this impact.

Testing

New tests theoretically prove that given the same effective input, the generator produces the same parsed model which prevents regeneration.

A variety of sanity check and core scenario validation tests have been added to verify that we regenerate source only when needed.

There are no regressions to existing unit and functional tests.

Planned, but considered a “goodness” effort

We could do with real world simulation tests for the “large solution” scenario mentioned above. This is in progress and we’ll backport a test-only PR when complete. We shouldn’t block on this given other 8.0 work and the confidence we have given other tests. Any gaps we find would only require a minimal, incremental code fix.

Risk

Overall .NET 8 product

Low. Mostly a contained fix for an off-by-default component.

There are minimal changes to the System.Text.Json source generator; only to share generator helper components: diagnostic handling helpers and a utility collection type to facilitate creating incremental models.

Source generator

Medium

To facilitate incremental generation, parsing logic has been non-trivially modified to produce equatable models. No change to codified/already tested functionality but there might be subtle differences that might regress unknown scenarios.

The changes introduce intentional but minimal diffs in a small number of emitted-source regression baselines. We’ve determined that the changes are valid, but any diff indicates that generated binding logic might change for untested scenarios, which might cause functionality regressions.

Now that this final v1 feature-size change is complete, we’ll switch gears to extensively testing more scenarios and ensure upstream product integration (e.g. ASP.NET; regression projects for final products, working with .NET fundamentals team). As always, we’ll address any reported scenarios quickly.

Author: layomia
Assignees: layomia
Labels:

Servicing-consider, area-Extensions-Configuration

Milestone: 8.0.0

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 3, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email.

@carlossanlop
Copy link
Member

@eiriktsarpalis @tarekgh @ericstj we only need a code review sign-off from one of you and I can merge.

@ericstj
Copy link
Member

ericstj commented Oct 3, 2023

Single failing test is #92944

@ericstj ericstj merged commit bc0f1b0 into dotnet:release/8.0 Oct 3, 2023
105 of 108 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants