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-staging] Prefer most derived member in Configuration Binder source generator #101686

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Apr 29, 2024

Backport of #101316 to release/8.0-staging

/cc @ericstj

Customer Impact

  • Customer reported
  • Found internally

When binding a type that uses new to hide a member and changes the type of that member, Configuration Binder will fail to bind. This is inconsistent with the reflection binder, which binds all members.

Regression

  • Yes
  • No

Not a regression, but a blocker when using the source-generator.

Testing

New automated tests added, sharing bits with partner team to validate scenario is unblocked.

Risk

Low. Previously the source generator would traverse the type hierarchy down to System.Object and use the lowest/base member definition. Now we’ll choose the highest/most derived.

It could mean that there were cases that produced compiler failures before that now become warnings. For example - a type hides a member to make it read-only. Previously the source generator would try to set that member and result in an error. Now the member will be ignored, just like any other read-only member. This new behavior is more desirable and consistent with the behavior of the source generator.

There's still a gap between the source-generator and the reflection binder. The reflection binder would set all members in the type hierarchy with the same configuration data - even if the member was hidden. We're not trying to replicate this behavior and have not heard customer feedback asking for that behavior in the source generator.

Copy link
Contributor

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

@tarekgh tarekgh added this to the 8.0.x milestone Apr 29, 2024
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.

@ericstj I assume you'll fill the template and add the package authoring if needed. Thanks!

@ericstj ericstj added the Servicing-consider Issue for next servicing release review label Apr 29, 2024
@eerhardt
Copy link
Member

I took a private build of this and used it in the project I was hitting #101267 and #101273. This change allows the generated code to build successfully. It should unblock my scenario.

@ericstj ericstj added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@ericstj
Copy link
Member

ericstj commented Apr 30, 2024

Approved in mail.

@ericstj ericstj merged commit 47f25fb into release/8.0-staging Apr 30, 2024
109 of 113 checks passed
@danmoseley danmoseley deleted the backport/pr-101316-to-release/8.0-staging branch May 1, 2024 18:14
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2024
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