-
Notifications
You must be signed in to change notification settings - Fork 516
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
[Generator] Ensure that selectors fields do not have overlapping names. Fixes #18645 #18646
Conversation
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
|
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.
Looks like we need some test fixes
|
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
❗ API diff vs stable (Breaking changes)Legacy Xamarin (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
ℹ️ Generator diffGenerator 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 |
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
📚 [PR Build] Artifacts 📚Packages generatedView packagesPipeline on Agent |
🚀 [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 Pipeline on Agent |
/sudo backport net8.0-xcode15 |
Backport Job to branch net8.0-xcode15 Created! The magic is happening here |
Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=8218688 for more details. |
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