-
Notifications
You must be signed in to change notification settings - Fork 554
[RGen] Use the 'global::' alias for all types. #22885
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
Conversation
Upate the code generation of rgen to use the 'global::' allias based in the generator configuration. The tests classes have been updated to ensure that if we switch off the use of the alias, the tests do not need to be udpdated.
| [SupportedOSPlatform ("maccatalyst13.1")] | ||
| [BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)] | ||
| public virtual partial nfloat LineSpacing | ||
| public virtual partial global::CompareGeneratedCode.CompareGeneratedCode.netmodule..nfloat LineSpacing |
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.
This is wrong, missing the global using. Will fix 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.
Pull Request Overview
This PR updates the RGen code generator to support toggling the global:: alias for all generated type references via configuration.
- Introduces a
$GLOBAL$placeholder in test fixtures and aisGlobalflag to replace it at read time - Adds
StringExtensions.GetIdentifierNameand refactors emitters/formatters to buildTypeSyntaxdirectly - Enhances
TypeInfowith init-only properties and conversion helpers for array/non-nullable types
Reviewed Changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rgen/.../ExpectedAVAudioPcmBufferNoDefaultCtr.cs | Switched global:: placeholder for ObjCRuntime calls |
| tests/rgen/.../ExpectedAVAudioPcmBufferDefaultCtr.cs | Updated default ctor to use global:: alias for Foundation types |
| tests/rgen/.../BindingSourceGeneratorGeneratorTests.cs | Adjusted test to expect global:: in class handle |
| tests/rgen/.../BaseTestDataGenerator.cs | Added isGlobal flag and placeholder replacement logic |
| tests/rgen/.../BaseGeneratorTestClass.cs | Added Global(string) helper to prefix with global:: |
| src/rgen/.../Microsoft.Macios.Transformer.csproj | Linked Configuration.cs and ArgumentSyntaxExtensions.cs |
| src/rgen/.../TypeInfoFormatter.cs | Refactored identifier building to use GetIdentifierName |
| src/rgen/.../StringExtensions.cs | Added GetIdentifierName extension for qualified names |
| src/rgen/.../ArgumentSyntaxExtensions.cs | Removed unused using and aligned imports |
| src/rgen/.../EnumEmitter.cs | Refactored error-domain emitter to use NSString & Dlfcn |
| src/rgen/.../ClassEmitter.cs | Updated default-ctor emission to use qualified NSObjectFlag |
| src/rgen/.../BindingSyntaxFactory.cs | Changed StaticVariable and invocations to accept TypeSyntax |
| src/rgen/.../BindingSyntaxFactory.Trampoline.cs | Switched pointer/generic calls to use GetIdentifierSyntax |
| src/rgen/.../BindingSyntaxFactory.Runtime.cs | Updated various Get* methods to take TypeSyntax |
| src/rgen/.../BindingSyntaxFactory.Property.cs | Refactored property binding to use GetIdentifierName |
| src/rgen/.../BindingSyntaxFactory.ObjCRuntime.cs | Expanded and reorganized known ObjCRuntime/Foundation types |
| src/rgen/.../BindingSyntaxFactory.Dlfcn.cs | Switched Dlfcn/Libraries fields to use GetIdentifierName |
| src/rgen/.../DataModel/TypeInfo.cs | Made several props init-only; added ToArrayElementType/ToNonNullable |
| src/rgen/.../Microsoft.Macios.Bindings.Analyzer.csproj | Linked Configuration.cs and ArgumentSyntaxExtensions.cs |
| return isGlobal | ||
| ? File.ReadAllText (fullPath).Replace ("$GLOBAL$", "global::") | ||
| : File.ReadAllText (fullPath).Replace ("$GLOBAL$", ""); |
Copilot
AI
May 23, 2025
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.
The code invokes File.ReadAllText twice depending on isGlobal. Consider reading the file into a local variable once and then applying .Replace(...) to avoid duplicate I/O.
| return isGlobal | |
| ? File.ReadAllText (fullPath).Replace ("$GLOBAL$", "global::") | |
| : File.ReadAllText (fullPath).Replace ("$GLOBAL$", ""); | |
| var fileContent = File.ReadAllText(fullPath); | |
| return isGlobal | |
| ? fileContent.Replace("$GLOBAL$", "global::") | |
| : fileContent.Replace("$GLOBAL$", ""); |
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.
It's an if unary operator, only one branch executes.
| return type; | ||
| } | ||
|
|
||
| public TypeInfo ToArrayElementType () |
Copilot
AI
May 23, 2025
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.
The ToArrayElementType helper resets IsArray and SpecialType but does not update Name or Namespace to the element type. This will lead to incorrect type names downstream. It should also set Name and Namespace based on the element type.
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.
The comment is incorrect, we do the following:
if (symbol is IArrayTypeSymbol arraySymbol) {
// override the name and namespace with the array name
(Name, Namespace) = GetTypeNameAndNamespace (arraySymbol.ElementType);
FullyQualifiedName = arraySymbol.ElementType.ToDisplayString ();
IsArray = true;
ArrayElementType = arraySymbol.ElementType.SpecialType;
ArrayElementTypeIsWrapped = arraySymbol.ElementType.IsWrapped ();
ArrayElementIsINativeObject = arraySymbol.ElementType.IsINativeObject ();
}which avoids the issue. Good try.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/rgen/Microsoft.Macios.Generator/Emitters/BindingSyntaxFactory.Dlfcn.cs
Outdated
Show resolved
Hide resolved
…ppends with nfloat.
…ory.Dlfcn.cs Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #4732f46] Build passed (Build packages) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #c0160ed] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commit.NET ( No breaking changes )✅ API diff vs stable.NET ( No breaking changes )ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #0bfe739] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
💻 [CI Build #0bfe739] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #0bfe739] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #0bfe739] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
💻 [CI Build #c0160ed] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🔥 [CI Build #0bfe739] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 15 tests failed, 110 tests passed. Failures❌ monotouch tests (MacCatalyst)Details
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
|
Failures are unrelated to code changes. |
Update the code generation of rgen to use the 'global::' alias based in the generator configuration. The tests classes have been updated to ensure that if we switch off the use of the alias, the tests do not need to be updated.