-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix binder gen compile issues due to inaccessible members and identifier name clashes #91657
Conversation
Tagging subscribers to this area: @dotnet/area-extensions-configuration |
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Show resolved
Hide resolved
9d8783f
to
832c9bb
Compare
CI failures unrelated; referenced above. |
@@ -78,6 +80,27 @@ private static bool IsValidRootConfigType(ITypeSymbol? type) | |||
} | |||
|
|||
return true; | |||
|
|||
static bool IsAccessibleFromGenBinders(ITypeSymbol type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you use IsSymbolAccessibleWithin
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a "within" symbol. The symbol would be the generated binder extension class which doesn't exist yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you use another type symbol in the same assembly as a proxy for the one that will be generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to use the current IAssemblySymbol as the 'within' parameter although I haven't tested that myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I considered the proxy approach. It didn't feel deterministic on first thought, but yes it would be a correct/better check. I'll also try using the assembly symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a proxy would work in the common case, but we'd need to account for the assembly possibly having just one (e.g. a singular, simple target-config POCO with primitive fields). Any handwritten fallback would have a vastly different impl.
Comparing with assembly doesn't work - nested privates aren't visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing with assembly doesn't work - nested privates aren't visible.
Are any of the types being generated nested within user-defined types? If not, I would expect its visibility to be equivalent to that of the assembly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing with assembly doesn't work - nested privates aren't visible.
I tried it again; the assembly check works. Must have not used the !
operator when I tried. Thanks.
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Show resolved
Hide resolved
Will address @eiriktsarpalis's feedback in a new PR before proposing an RC-2 backport with the two merged commits. |
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165475776 |
Fixes #90909 and #90976. RC-2 candidate.
cc @ericstj.