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] Use fully qualified names when referencing user types. (#94267) #94307

Merged

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Nov 2, 2023

Backport of #94267 to release/8.0-staging

Customer Impact

As implemented, the ConfigurationBinder source generator employs using statements to import namespaces of user-defined types and references these types by name only. This can cause compiler errors in cases where there is ambiguity -- i.e. referencing types with equal names but in different namespaces. This is blocking partner teams when working with common scenaria, see #93498.

This PR updates the source generator to always use fully qualified names when referencing user declared types.

Testing

Added regression testing covering the type name conflict scenario. Manually tested eshopOnContainers sample, and Azure.Data.Tables package as well.

Risk

Low to moderate. Makes substantial changes to product code, replacing all type formatting from minimal to fully qualified names.

@ghost
Copy link

ghost commented Nov 2, 2023

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

Customer Impact

As implemented, the ConfigurationBinder source generator employs using statements to import namespaces of user-defined types and references these types by name only. This can cause compiler errors in cases where there is ambiguity -- i.e. referencing types with equal names but in different namespaces. This is blocking partner teams when working with common scenaria, see #93498.

This PR updates the source generator to always use fully qualified names when referencing user declared types.

Testing

Added regression testing covering the type name conflict scenario.

Risk

Low to moderate. Makes substantial changes to product code, replacing all type formatting from minimal to fully qualified names.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-Extensions-Configuration

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.x milestone Nov 2, 2023
@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Nov 2, 2023
@teo-tsirpanis
Copy link
Contributor

Should this target release/8.0-staged?

@tarekgh
Copy link
Member

tarekgh commented Nov 2, 2023

oops, @teo-tsirpanis is correct. This should be ported to release/8.0-staged instead. good catch.

@ericstj ericstj changed the base branch from release/8.0 to release/8.0-staging November 2, 2023 19:46
@ericstj
Copy link
Member

ericstj commented Nov 2, 2023

I went ahead and changed the target branch of the PR. It should port cleanly as we haven't had churn there.

edit: looks like we need to remove a couple commits.

* Use fully qualified names when referencing user types.

* Use ToUpperInvariant

* Address feedback
@ericstj ericstj force-pushed the cb-fully-qualified-names branch from 757f066 to a204e81 Compare November 2, 2023 19:55
@eiriktsarpalis eiriktsarpalis changed the title Use fully qualified names when referencing user types. (#94267) [release/8.0-staging] Use fully qualified names when referencing user types. (#94267) Nov 2, 2023
@danmoseley
Copy link
Member

is it possible to check our various other source generators in the box for the same issue?

@eiriktsarpalis
Copy link
Member Author

is it possible to check our various other source generators in the box for the same issue?

STJ has been using fully qualified names since it shipped. Not sure what the case is with options validator or the logger cc @tarekgh

@danmoseley
Copy link
Member

Yeah and the Regex one and the RequestDelegateGenerator do as well, but how to figure whether they're using global:: in all necessary places, not sure. I assume so :) @captainsafia

@danmoseley
Copy link
Member

@BrennanConroy what about the SignalR generators?

@BrennanConroy
Copy link
Member

@BrennanConroy what about the SignalR generators?

It's not shipping. But if we have validation that can be added we'd be interested so we can add it when we do want to ship it.

@captainsafia
Copy link
Member

Yeah and the Regex one and the RequestDelegateGenerator do as well, but how to figure whether they're using global:: in all necessary places, not sure. I assume so :) @captainsafia

RDG uses FQN + global:: for all user provided types. The display format we use is defined here.

@ericstj ericstj added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 7, 2023
@ericstj
Copy link
Member

ericstj commented Nov 7, 2023

Approved over mail.

@ericstj ericstj merged commit 4ae54f2 into dotnet:release/8.0-staging Nov 7, 2023
@eiriktsarpalis eiriktsarpalis deleted the cb-fully-qualified-names branch November 7, 2023 15:12
@carlossanlop carlossanlop modified the milestones: 8.0.x, 8.0.1 Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
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.

8 participants