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

Fully Qualified Type names for the Source Generator (to avoid namespace clashes) #160

Merged

Conversation

thomhurst
Copy link
Contributor

I've got a solution where I'm getting compilation errors because of namespace naming conflicts. Fully qualifying the types in generated code (so they begin with global::) should fix this.

@@ -89,8 +89,8 @@ partial class {classNameWithGenericTypes}");
foreach (var (param, arg) in paramArgPairs)
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 we also update similar usage in line 80?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope namespaces don't support global:: - Only types do

Copy link
Contributor

Choose a reason for hiding this comment

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

right, sorry I thought it can also be used for namespaces

@romfir
Copy link
Contributor

romfir commented Sep 1, 2023

@thomhurst Please run OneOf.SourceGenerator.AnalyzerTests and fix them (I think they should now fail) and add a test for your specific case (for the error that you encountered), could you also add "global::" prefix to AttributeNamespace and remove "global::" from filtering inside GetSemanticTargetForGeneration?
@mcintyre321 could we possibly run these tests in CI?

@thomhurst
Copy link
Contributor Author

thomhurst commented Sep 1, 2023

@thomhurst Please run OneOf.SourceGenerator.AnalyzerTests and fix them (I think they should now fail) and add a test for your specific case (for the error that you encountered), could you also add "global::" prefix to AttributeNamespace and remove "global::" from filtering inside GetSemanticTargetForGeneration? @mcintyre321 could we possibly run these tests in CI?

The comparison inside GetSemanticTargetForGeneration is just with strings, so I have to prepend global:: there. But we're comparing it anyway against a fully qualified type, so we know we're getting the right one.

Also all tests are still passing.

I can look at adding a test though.

@thomhurst
Copy link
Contributor Author

@romfir They have also run on CI here:
https://ci.appveyor.com/project/mcintyre321/oneof/builds/47938445

@romfir
Copy link
Contributor

romfir commented Sep 1, 2023

Interesting I thought that this test would fails because of the missing namespace in implicit operators

@romfir
Copy link
Contributor

romfir commented Sep 1, 2023

GeneratesPublicClass fails on my machine with (4,11): error CS7000: Unexpected use of an aliased name

@thomhurst
Copy link
Contributor Author

Interesting I thought that this test would fails because of the missing namespace in implicit operators

Looks like the assertions aren't doing full equivalency. It seems to be doing things like checking no diagnostics, or that certain elements match?

@thomhurst
Copy link
Contributor Author

GeneratesPublicClass fails on my machine with (4,11): error CS7000: Unexpected use of an aliased name

You haven't changed the namespace to fully qualified have you?

@romfir
Copy link
Contributor

romfir commented Sep 1, 2023

Interesting I thought that this test would fails because of the missing namespace in implicit operators

Looks like the assertions aren't doing full equivalency. It seems to be doing things like checking no diagnostics, or that certain elements match?

They are using SyntaxTree.IsEquivalentTo, perhabs adding "global::" is a trivia difference mentioned in the method description 😄 also the comments seems to be ignored

You haven't changed the namespace to fully qualified have you?

I did 🥲

Could you move this new test to AnalyzerTests? All OneOf.SourceGenerator.Tests should be moved there but I did not have time to do it, perhaps the name is a bit misleading but its a better way to test source generators.

@thomhurst
Copy link
Contributor Author

@romfir Have added a test to the analyzer project GeneratesClassWithConflictingNamespaces

@thomhurst
Copy link
Contributor Author

@mcintyre321 Any change of getting this in please? I'm blocked from using the source generator at the moment because of this

Copy link
Contributor

@romfir romfir left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcintyre321 mcintyre321 merged commit cfe1c4d into mcintyre321:master Sep 7, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants