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

Create source generator for configuration binding #82179

Merged
merged 7 commits into from
Mar 16, 2023
Merged

Create source generator for configuration binding #82179

merged 7 commits into from
Mar 16, 2023

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Feb 15, 2023 •
edited
Loading

@layomia layomia added this to the 8.0.0 milestone Feb 15, 2023
@layomia layomia self-assigned this Feb 15, 2023
@ghost
Copy link

ghost commented Feb 15, 2023

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

Issue Details
Author: layomia
Assignees: layomia
Labels:

area-Extensions-Configuration
Milestone: 8.0.0

@layomia layomia requested review from jeffhandley and removed request for jeffhandley February 15, 2023 18:30
@layomia layomia added the NO-REVIEW Experimental/testing PR, do NOT review it label Feb 15, 2023
@layomia layomia force-pushed the add-configbinder-src-generator branch 5 times, most recently from d27439a to 6c9f2ed Compare February 16, 2023 20:19
@build-analysis build-analysis bot mentioned this pull request Feb 16, 2023
Closed
@layomia layomia force-pushed the add-configbinder-src-generator branch from 6c9f2ed to 2020a35 Compare February 21, 2023 04:14
@layomia layomia mentioned this pull request Feb 21, 2023
Closed
@layomia
Copy link
Contributor Author

layomia commented Feb 21, 2023

We have the green light from API review to proceed with this implementation. Still need to resolve CI failures but it's ready for a first look.

@layomia layomia removed the NO-REVIEW Experimental/testing PR, do NOT review it label Feb 21, 2023
@layomia layomia changed the title Create source generator for ConfigurationBinder Create source generator for configuration binding Feb 22, 2023
eiriktsarpalis
eiriktsarpalis reviewed Mar 14, 2023
View reviewed changes
...braries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingSourceGenerator.cs
(node, _) => node is InvocationExpressionSyntax invocation,
(context, cancellationToken) => new BinderInvocationOperation(context, cancellationToken));

IncrementalValueProvider<(KnownTypeData, ImmutableArray<BinderInvocationOperation>)> inputData = compilationData.Combine(inputCalls.Collect());
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to call Combine here? Assuming you have an application with 500 BinderInvocationOperation locations, then changing just one would result in all 500 of them being aggregated and recalculated.

Assuming there is no strong performance reason behind consolidating one parser for all operations, I would recommend keeping these independent as an IncrementalValueSProvider<(KnownTypeData, BinderInvocationOperation)>. IncrementalValuesProvider also has a RegisterSourceOutput which can be triggered independently for each of its components without forcing recalculation of the others.

sharwell
sharwell reviewed Mar 15, 2023
View reviewed changes
...Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingSourceGenerator.Helpers.cs Show resolved Hide resolved
...Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingSourceGenerator.Helpers.cs Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Resources/Strings.resx Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/SourceGenerationSpec.cs
namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
{
internal sealed record SourceGenerationSpec(
HashSet<TypeSpec> TypesForBindMethodGen,
Copy link
Member

Choose a reason for hiding this comment

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

💡 This was my solution:

DotNetAnalyzers/StyleCopAnalyzers@253d408

If there is a better solution I'm interested to know.

This was referenced Mar 16, 2023
Closed
Closed
Closed
Closed
Closed
Closed
@layomia
Copy link
Contributor Author

layomia commented Mar 16, 2023 •
edited
Loading

Thanks for reviewing this PR folks. I've opened issues to address feedback not resolved in this PR. They are linked above and also tracked by #79527. I'll merge this PR now to make preview 3 & because additional changes will require more review & churn. I believe the current implementation is viable for users to try as-is. W.r.t risk to the product caused by issues/bugs in the feature:

  • It is only available via referencing the NuGet package
  • It is opt-in via setting property <EnableMicrosoftExtensionsConfigurationBinderGenerator>true</EnableMicrosoftExtensionsConfigurationBinderGenerator>

Here are the follow up items I'll try to address in preview 3 (snaps on Monday 3/20, cc @carlossanlop):

@layomia layomia mentioned this pull request Mar 16, 2023
Closed
@layomia layomia merged commit be8d451 into dotnet:main Mar 16, 2023
@layomia layomia mentioned this pull request Mar 17, 2023
Merged
@davidfowl
Copy link
Member

How do I enable the source generator?

@ViktorHofer
Copy link
Member

ViktorHofer commented Mar 20, 2023 •
edited
Loading

How do I enable the source generator?

@davidfowl see layomi's comment above:

It is opt-in via setting property <EnableMicrosoftExtensionsConfigurationBinderGenerator>true</EnableMicrosoftExtensionsConfigurationBinderGenerator>

Make sure that the latest available Microsoft.Extensions.Configuration.Binder.nupkg is referenced (from the dotnet8 feed).

@layomia layomia mentioned this pull request Mar 27, 2023
Open
3 tasks
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2023
@layomia layomia added the source-generator Indicates an issue with a source generator feature label May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tarekgh tarekgh tarekgh left review comments

@ericstj ericstj ericstj left review comments

@eerhardt eerhardt eerhardt left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@am11 am11 am11 left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@stephentoub stephentoub stephentoub left review comments

@bollhals bollhals bollhals left review comments

@sharwell sharwell sharwell left review comments

@krwq krwq krwq approved these changes

@jeffhandley jeffhandley Awaiting requested review from jeffhandley jeffhandley is a code owner

Assignees

@layomia layomia

Labels
area-Extensions-Configuration source-generator Indicates an issue with a source generator feature
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.