Skip to content

[release/8.0] Fix binder gen compile issues due to inaccessible members and identifier name clashes - #91967

Merged
ericstj merged 5 commits into
release/8.0from
backport/pr-91657-to-release/8.0
Sep 14, 2023
Merged

[release/8.0] Fix binder gen compile issues due to inaccessible members and identifier name clashes#91967
ericstj merged 5 commits into
release/8.0from
backport/pr-91657-to-release/8.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

Backport of #91657 to release/8.0

/cc @layomia

Customer Impact

Fixes two reported bugs. We expect users to hit them:

  1. Compilation failures when private nested types are specified as config targets:
usingMicrosoft.Extensions.Configuration;internalclassProgram{privatestaticvoidMain(){}privatestaticCFoo(IConfigurationconfiguration){Cc=new();configuration.Bind(c);// Causes compile error.returnc;}privatesealedclassC{publicintValue{get;set;}}}
  1. Errors due to generator using duplicate binding helper names to handle different types:
usingMicrosoft.Extensions.Configuration;IConfigurationconfiguration=newConfigurationBuilder().Build();// Generator emits identifier `Cint` to handle both types causing a name clash.varc1=newCint();varc2=newC<int>();configuration.Bind(c1);configuration.Bind(c2);internalclassC<T>{publicintValue{get;set;}}internalclassCint{publicintValue{get;set;}}

Testing

Extensive unit and emitted source regression tests were added to verify the fix.

Risk

Low. Contained fix for off-by-default component. Minimal custom identifier mangling is done, instead we use more reliable Roslyn APIs.

@ghost

Copy link
Copy Markdown

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 #91657 to release/8.0

/cc @layomia

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author:github-actions[bot]
Assignees:-
Labels:

area-Extensions-Configuration

Milestone:-

@layomia

Copy link
Copy Markdown
Contributor

I've addressed all feedback on #91974. cc @eiriktsarpalis.

@layomia
layomiaforce-pushed the backport/pr-91657-to-release/8.0 branch from c5811d2 to 95e712fCompareSeptember 14, 2023 00:06
@layomialayomia removed the NO-REVIEW Experimental/testing PR, do NOT review it label Sep 14, 2023
@layomia
layomia marked this pull request as ready for review September 14, 2023 00:07
@layomialayomia added the Servicing-consider Issue for next servicing release review label Sep 14, 2023

@ericstjericstj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approve this change for RC2. Thank you for minimizing your diff on these fixes and reusing the code from Json. @artl93 this one is ready for your review.

@artl93artl93 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

M2 approved.

@artl93artl93 removed the Servicing-consider Issue for next servicing release review label Sep 14, 2023
@artl93artl93 added the Servicing-approved Approved for servicing release label Sep 14, 2023
@ericstj
ericstj merged commit 1d60c69 into release/8.0Sep 14, 2023
@jkotas
jkotas deleted the backport/pr-91657-to-release/8.0 branch September 15, 2023 20:58
@radicalradical mentioned this pull request Sep 26, 2023
@radicalradical mentioned this pull request Oct 3, 2023
@ghostghost locked as resolved and limited conversation to collaborators Oct 16, 2023
Sign up for freeto 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.

5 participants

@layomia@eiriktsarpalis@ericstj@artl93@carlossanlop