-
Notifications
You must be signed in to change notification settings - Fork 219
C#: Use fully qualified type names #1275
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
base: main
Are you sure you want to change the base?
C#: Use fully qualified type names #1275
Conversation
3c9f118
to
8b7231f
Compare
87c3fb2
to
e8daded
Compare
e8daded
to
d0be31c
Compare
@@ -948,7 +935,7 @@ impl Bindgen for FunctionBindgen<'_, '_> { | |||
uwrite!( | |||
self.src, | |||
" | |||
var {array} = new List<{ty}>({length}); | |||
var {array} = new global::System.Collections.Generic.List<{ty}>({length}); |
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.
Does type_name_with_qualifier
give ty
the global name too ?
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 do not believe so. Please lead me in the right direction to make this happen. It should be unnecessary as long as ty
is either a primitive or a type already contained within the same namespace as this emitted code.
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 know much about the generator codebase, @jsturtevant please advise.
I just noticed that different flavors ty
is sprinkled thru the code.
I'm thinking that we could have built-in hash table (or switch) translating known system types from short name to long name. All of them for consistency sake.
For int
-> global::System.Int32
I guess later we will also generate marshaling for global::System.Threading.Tasks.Task
which is not in System
namespace. Also global::System.Threading.Tasks.Task<global::System.Int32>
And I think WIT can do list of list ?
global::System.Collections.Generic.List<global::System.Collections.Generic.List<global::System.Int32>>
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 personally believe that primitives (int
) should be fully specified, but that's up to you.
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.
Not big deal for me, I mentioned it for consistency. This generated code should not be something that people need to read often and compiler sees that as equivalent.
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.
Does type_name_with_qualifier give ty the global name too ?
No type_name_with_qualifier
will not add global::
in-front of the type name.
And I think WIT can do list of list ?
Yes it can.
I like that this makes the code generation part a lot easier. On a personal opinion thought I don't particularly like the fact that we will be generating code that you would normally never write yourself (I know this isn't true for everything already, but I would prefer if we can take it in that direction). Would adjusting the current code to address the scenario of usage of types that haven't been imported (by adding the using statement in the generated code, for those missing places) be sufficient or do you think there is a broader problem with that approach @just-ero ?
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.
Since wit-bindgen
is a standalone tool (as in, not directly connected to componentize-dotnet
), it seems reasonable to me to generate code that a human might write.
However, a couple things to dissuade;
- Since the generated header explicitly discourages editing of the generated code, users would interact with it very rarely. The appearance of the generated code would not matter in most cases, as it is rarely even observed.
- Fully qualified type names are safer, as they leave no ambiguity. Please read
string
vs.String
is not a style debate by Jared Parsons.
To be clear: in our case, the fully qualified type name would be closer to thestring
variant, including namespaces and using just the type name is closer to theString
variant.
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.
In general, one requirement of generated code (at least for .NET) is to fulfill the lowest common denominator. In C# that might mean not using interpolated strings, file-scoped namespaces, or other more modern features, as the user may target a language version which does not support those features.
componentize-dotnet
is basically hard locked to projects which target .NET 10 and up, so this would only be a problem if the user explicitly downgraded using the LangVersion
MSBuild property. It's not a scenario I would worry too much about, but I thought I'd mention it.
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.
Let's not worry about below Net10
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.
Fully qualified type names are safer, as they leave no ambiguity.
This is compelling to me for generated code. So I am +1 to this change.
It may make sense to introduce constants for some namespace or type names; |
Closes #1265.