-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Reuse more Microsoft.Interop.SourceGeneration code in JS generators #74949
Conversation
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
the wasm built test failures are likely from #74897 @ericstj @danmoseley can you verify? |
Yes:
That's been fixed now, we can retrigger the pipeline. |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
…SourceGeneration.GeneratedStatements and ContainingSyntaxContext types.
…cating the logic.
99a95ea
to
5c8eaa3
Compare
...aries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportStubContext.cs
Outdated
Show resolved
Hide resolved
AttributeArgument(LiteralExpression(SyntaxKind.StringLiteralExpression, Literal(GeneratorVersion))) | ||
})))))); | ||
} | ||
string stubTypeFullName = method.ContainingType.ToDisplayString(new SymbolDisplayFormat(typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces)); |
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.
NameAndContainingTypesAndNamespaces
is better than what we have in the GetFullyQualifiedMethodName
and s_typeNameFormat
, please unify 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.
If we use NameAndContainingTypesAndNamespaces
in GetFullyQualifiedMethodName
, how do we determine which "." separate class names and which namespace names?
Context: mono special naming convention requires to separate class names with /
and namespaces with '.'.
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 think to meet the Mono naming requirements with NameAndContainingTypesAndNamespaces
would require us to go back to using ToDisplayParts()
. I'll leave that for now since we just moved away from that.
...ies/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportCodeGenerator.cs
Show resolved
Hide resolved
...es/System.Runtime.InteropServices/gen/LibraryImportGenerator/DefaultMarshallingInfoParser.cs
Outdated
Show resolved
Hide resolved
...aries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportStubContext.cs
Outdated
Show resolved
Hide resolved
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.
love it!
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
Reuse more of the code in Microsoft.Interop.SourceGeneration in the JavaScript source generators instead of duplicating the logic.
Refactor the JavaScript generators to use the GeneratedStatements type instead of providing its own JSGeneratedStatements type.
Refactor the JSSignatureContext type to wrap a SignatureContext instead of providing a very similar surface area with a lot of duplicated code.
This PR also enhances the API surface of types in Microsoft.Interop.SourceGeneration to enable these sorts of scenarios.