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

[Generator] Ensure that selectors fields do not have overlapping names. Fixes #18645 #18646

Merged

Conversation

mandel-macaque
Copy link
Member

The way the generator names private fields for selectors results on not compilable code when there are two selectoors A and B such that selector B == AHandle.

That is, if selector A is "user", selector B will be "userHandle". This happens because Handle is quite a common postfix in security when working with user accounts (userHandler and others) and we need to ensure that the Handle postfix added by the generator is unique.

As stated in the bug we could try to fix this by keeping track of the variables in the context in wich the generator is creating code. The problem with such approach is that it will make very hard to predict the name of the fields making the manual code harder to write. The PR contains examples of such manual code.

To fix the situation we have moved from using Handle to XHandle as a postfix in the field names whihc reduces the chances of finding a similar corner case in the future.

A test has been added to show the case in which we found the bug.

Fixes #18645

Fixes xamarin#18645

The way the generator names private fields for selectors results on not
compilable code when there are two selectoors A and B such that selector
B == AHandle.

That is, if selector A is "user", selector B will be
"userHandle". This happens because Handle is quite a common postfix in
security when working with user accounts (userHandler and others) and
we need to ensure that the Handle postfix added by the generator is unique.

As stated in the bug we could try to fix this by keeping track of the variables in the context
in wich the generator is creating code. The problem with such approach
is that it will make very hard to predict the name of the fields making
the manual code harder to write. The PR contains examples of such manual
code.

To fix the situation we have moved from using Handle to XHandle as a
postfix in the field names whihc reduces the chances of finding a
similar corner case in the future.

A test has been added to show the case in which we found the bug.

Fixes xamarin#18645
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@dalexsoto
Copy link
Member

Looks like we need some test fixes

Executing link-frameworks-1...
[PASS] As expected found field NSObject.fl for framework 'Foundation'.
[PASS] As expected found field NSObject.al for framework 'AppKit'.
[FAIL] Unexpectedly found field NSObject.selDescriptionXHandle for framework 'unknown'.
[FAIL] Unexpectedly found field NSObject.selHashXHandle for framework 'unknown'.
[FAIL] Unexpectedly found field NSObject.selIsEqual_XHandle for framework 'unknown'.
Executed link-frameworks-1

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS (no change detected)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS (no change detected)
NET (empty diffs)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: (empty diff detected)

❗ API diff vs stable (Breaking changes)

Legacy Xamarin (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • iOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • tvOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • watchOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • macOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • iOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • tvOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • MacCatalyst: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • macOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) Unable to create gist: Response status code does not indicate success: 502 (Bad Gateway). (raw diff) - Please review changes)

Pipeline on Agent
Hash: 53f265714efbbac7a9c3fd7a52c499baf79250fd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Windows Integration Tests passed 💻

All Windows Integration Tests passed.

Pipeline on Agent
Hash: 53f265714efbbac7a9c3fd7a52c499baf79250fd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 235 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 35 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 53f265714efbbac7a9c3fd7a52c499baf79250fd [PR build]

@mandel-macaque mandel-macaque merged commit 716e8ba into xamarin:main Aug 8, 2023
46 of 48 checks passed
@mandel-macaque mandel-macaque deleted the fix-generator-duplicated-var branch August 8, 2023 13:25
@mandel-macaque
Copy link
Member Author

/sudo backport net8.0-xcode15

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch net8.0-xcode15 Created! The magic is happening here

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=8218688 for more details.

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.

[Generator] The generator creates not compilable code when a selector has the word Handle in it.
5 participants