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

Address feedback from config binding gen PR to improve enum parsing #89952

Merged
merged 22 commits into from
Oct 11, 2023
Merged

Address feedback from config binding gen PR to improve enum parsing #89952

merged 22 commits into from
Oct 11, 2023

Conversation

pedrobsaila
Copy link
Contributor

@pedrobsaila pedrobsaila commented Aug 3, 2023 •
edited
Loading

Fixes #89936
Fixes #89879

@layomia I addressed the follow-up fixes.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 3, 2023
@ghost
Copy link

ghost commented Aug 3, 2023

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

Issue Details

Fixes #89936

@layomia I addressed 2 of the follow-up fixes. I'll wait for the final decision (polyfill/checking whether Enum.Parse is available) before proceeding to fix it

Author: pedrobsaila
Assignees: -
Labels:

area-Extensions-Configuration
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Aug 3, 2023
Closed
layomia
layomia reviewed Aug 9, 2023
View reviewed changes
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated
@@ -128,7 +137,7 @@ private static bool IsValidRootConfigType(ITypeSymbol? type)
{
ParsableFromStringSpec stringParsableSpec = new(type) { StringParsableTypeKind = specialTypeKind };

if (stringParsableSpec.StringParsableTypeKind is not StringParsableTypeKind.AssignFromSectionValue)
if (stringParsableSpec.StringParsableTypeKind != StringParsableTypeKind.AssignFromSectionValue && stringParsableSpec.StringParsableTypeKind != StringParsableTypeKind.Enum)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd re-write this as

Suggested change
if (stringParsableSpec.StringParsableTypeKind != StringParsableTypeKind.AssignFromSectionValue && stringParsableSpec.StringParsableTypeKind != StringParsableTypeKind.Enum)
if (specialTypeKind is StringParsableTypeKind.Enum)
{
_sourceGenSpec.EmitEnumParseMethod = true;
}
else if (specialTypeKind is not StringParsableTypeKind.AssignFromSectionValue)
{
_sourceGenSpec.PrimitivesForHelperGen.Add(stringParsableSpec);
}

layomia
layomia reviewed Aug 9, 2023
View reviewed changes
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated
Comment on lines 68 to 67
foreach (var typeSymbol in _createdSpecs.Keys)
{
if (IsEnum(typeSymbol))
{
_sourceGenSpec.EmitEnumParseMethod = true;
break;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need this given the suggestion below.

@layomia
Copy link
Contributor

layomia commented Aug 9, 2023 •
edited
Loading

@pedrobsaila, let's go with checking for the right method in the parser - #89823 (comment). We could also address #89879 in this PR.

@build-analysis build-analysis bot mentioned this pull request Aug 12, 2023
Open
3 tasks
@layomia layomia self-assigned this Aug 25, 2023
@layomia
Copy link
Contributor

layomia commented Aug 25, 2023

@pedrobsaila apologies for my delayed response. Could you pls resolve merge conflicts here?

@build-analysis build-analysis bot mentioned this pull request Aug 27, 2023
Closed
layomia
layomia reviewed Aug 28, 2023
View reviewed changes
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs Outdated
@@ -108,6 +110,10 @@ public KnownTypeSymbols(CSharpCompilation compilation)
IReadOnlyList_Unbound = compilation.GetBestTypeByMetadataName(typeof(IReadOnlyList<>))?.ConstructUnboundGenericType();
IReadOnlySet_Unbound = compilation.GetBestTypeByMetadataName("System.Collections.Generic.IReadOnlySet`1")?.ConstructUnboundGenericType();
ISet_Unbound = ISet?.ConstructUnboundGenericType();

// needed to be able to know if a member exist inside the compilation unit
Enum = compilation.GetBestTypeByMetadataName(typeof(Enum));
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the overload taking a SpecialType (i.e. SpecialType.System_Enum). All the special types are guaranteed to be in the input compilation.

layomia
layomia reviewed Aug 28, 2023
View reviewed changes
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/KnownTypeSymbols.cs Outdated
@@ -54,6 +54,8 @@ internal sealed record KnownTypeSymbols
public INamedTypeSymbol? ISet_Unbound { get; }
public INamedTypeSymbol? ISet { get; }
public INamedTypeSymbol? List { get; }
public INamedTypeSymbol? Enum { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public INamedTypeSymbol? Enum { get; }
public INamedTypeSymbol Enum { get; }

Per https://github.com/dotnet/runtime/pull/89952/files#r1307693295 the fetched value would not be null.

layomia
layomia reviewed Aug 28, 2023
View reviewed changes
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated
@@ -63,6 +63,8 @@ public Parser(SourceProductionContext context, KnownTypeSymbols typeSymbols, Imm
}
}

_sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;

The symbol could be null; avoid a null-ref exception.

layomia
layomia reviewed Aug 28, 2023
View reviewed changes
...der/tests/SourceGenerationTests/Baselines/ServiceCollection/Configure_T.net462.generated.txt Outdated
@@ -0,0 +1,243 @@
// <auto-generated/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the net462 variants. Could we refactor to the following structure?

> baselines
    > netcoreapp
        > foo.generated.txt
    > net462
        > foo.generated.txt

I'd actually suggest making a preliminary PR doing this. We'd need to remove the condition on [this line] so that the net462 tests would be included in automated runs. Doing this will reveal a bit of additional, tangential work needed to complete the effort.

@layomia
Copy link
Contributor

layomia commented Aug 28, 2023

Oof looks like merging #91180 reset your rebase effort. If you take another stab, I'll be sure to check if this PR is merge-ready & queue it up first if so.

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
...ries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs Outdated
{
string path = extType is ExtensionClassType.None
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a a preprocessor directive to determine the path

string path = 
#if NETCOREAPP
    ...
#else 
    ...
    ;

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
...soft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs Outdated
@@ -851,7 +852,7 @@ public async Task CollectionsFwk()
{
string source = GetCollectionsSource();

await VerifyAgainstBaselineUsingFile("Collections.net462.generated.txt", source, assessDiagnostics: (d) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Per f5a2abc#r1321885499, please revert the changes to these invocations.

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj Outdated
@@ -44,10 +93,8 @@
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Content Include="Baselines\*.generated.txt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert back to partitioning the baselines based on the binder ext. type. This is helpful when working issues specific to a particular binder.

layomia
layomia reviewed Sep 11, 2023
View reviewed changes
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated
@@ -63,7 +63,7 @@ public Parser(SourceProductionContext context, KnownTypeSymbols typeSymbols, Imm
}
}

_sourceGenSpec.EmitThrowIfNullMethod = !_typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty;
_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException is not null && _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull").IsEmpty is false;
Copy link
Contributor

Choose a reason for hiding this comment

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

_sourceGenSpec.EmitThrowIfNullMethod = _typeSymbols.ArgumentNullException?.GetMembers("ThrowIfNull").IsEmpty is false;

Also -- to be doubly sure, check that the method has the expected parameters.

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Sep 18, 2023 •
edited
Loading

@layomia can you check it please before the PR become stale again

@eiriktsarpalis
Copy link
Member

@pedrobsaila apologies for the delayed response, would it be possible to rebase your changes? Thanks!

@tarekgh
Copy link
Member

tarekgh commented Oct 5, 2023 •
edited
Loading

@pedrobsaila do you have a chance to resolve the merging conflicts?

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Oct 6, 2023 •
edited
Loading

I'm on it. The code in main branch changed deeply, resolving conflict is not easy and will take some time

@pedrobsaila
Copy link
Contributor Author

@eiriktsarpalis @tarekgh ready for a new round of reviews

tarekgh
tarekgh reviewed Oct 10, 2023
View reviewed changes
...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj Outdated
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Content Include="Baselines\*.generated.txt;Baselines\ConfigurationBinder\*.generated.txt;Baselines\OptionsBuilder\*.generated.txt;Baselines\ServiceCollection\*.generated.txt">
<ItemGroup>
<Content Include="Baselines\net462\*.generated.txt;
Copy link
Member

Choose a reason for hiding this comment

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

"Baselines

do we need to copy specific files? can't we include the whole Baseline folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Include just Baselines does not seem to work, the max I could do is this lightweight version :

<Content Include="Baselines\*\*\*.generated.txt;
         Baselines\*\*.generated.txt;">

Copy link
Member

Choose a reason for hiding this comment

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

You can use the syntax

runtime/src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Microsoft.Extensions.Options.SourceGeneration.Tests.csproj

Line 42 in c9b5dd4

<Content Include="Baselines\**\*">

@tarekgh
Copy link
Member

tarekgh commented Oct 10, 2023

@pedrobsaila thanks for getting this ready. I added a few questions if you can help with. otherwise LGTM.

tarekgh
tarekgh reviewed Oct 10, 2023
View reviewed changes
...ourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj Outdated
Baselines\netcoreapp\ConfigurationBinder\*.generated.txt;
Baselines\netcoreapp\OptionsBuilder\*.generated.txt;
Baselines\netcoreapp\ServiceCollection\*.generated.txt;">
<Content Include="Baselines\*\*\*.generated.txt;
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look right.

Copy link
Contributor Author

@pedrobsaila pedrobsaila Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

seems right to me. There are 2 patterns of files :

  • Baselines\netcoreapp\OptionsBuilder\*.generated.txt => would match Baselines\*\*\*.generated.txt
  • Baselines\netcoreapp\Primitives.generated.txt => would match Baselines\*.generated.txt

Copy link
Member

Choose a reason for hiding this comment

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

If we can copy the whole Baseline as I pointed out in #89952 (comment) that will be better I guess.

tarekgh
tarekgh approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Thanks @pedrobsaila, LGTM.

@tarekgh tarekgh merged commit f1b4930 into dotnet:main Oct 11, 2023
109 checks passed
@pedrobsaila pedrobsaila deleted the 89936 branch October 11, 2023 20:45
@ghost ghost locked as resolved and limited conversation to collaborators Nov 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@layomia layomia layomia left review comments

@tarekgh tarekgh tarekgh approved these changes

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

@ericstj ericstj Awaiting requested review from ericstj

Assignees
No one assigned
Labels
area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
4 participants
@pedrobsaila @layomia @eiriktsarpalis @tarekgh