-
Notifications
You must be signed in to change notification settings - Fork 789
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
Bugfix: Ref assemblies contained .property definitions at the wrong type in generated IL #14093
Conversation
@T-Gro please retarget the fix to 17.4 branch. |
ba37e87
to
8d3d52b
Compare
Wondering if we can rely on existence of properties when generating getter and setter methods? In other words if existing order matters and if we can theoretically break codegen by changing it? Edit: probably shouldn't matter in this stage, since those only generate tables and indices in them |
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.
Yeah so I guess the right way to go would be to get rid of a shared state, but since it's a quickfix - thanks for adding tests :)
It's not just "getting rid" of mutable state unfortunately, due to how IL emitting works. It's big and complex, and will likely involve a nearly full rewrite. Perhaps reflection.emit can be used for it instead. I know @TIHan was looking into it. It would be nice, but it'll likely be a quite low priority compared to all other issues we have now. |
I have tested the generated IL and it was the same, the order does not matter for the layout of the output. |
I never said "just" :) Sure. |
Thank you @T-Gro @vzarytovskii and team |
* Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 1997730 (#13925) (#14041) Co-authored-by: Vlad Zarytovskii <[email protected]> * Merge main to release/dev17.5 (#14043) Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Don Syme <[email protected]> Co-authored-by: Tomas Grosup <[email protected]> Co-authored-by: Edgar Gonzalez <[email protected]> Co-authored-by: Eugene Auduchinok <[email protected]> Co-authored-by: Chet Husk <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Florian Verdonck <[email protected]> Co-authored-by: Petr <[email protected]> Co-authored-by: Petr Pokorny <[email protected]> Co-authored-by: Theodore Tsirpanis <[email protected]> * Update FSharp.Editor.fsproj I believe a bad merge happened, this line is not in main. And the file does not exist in either branch. * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2014480 (#14049) * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2016907 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2016985 * XLF * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2017073 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2017391 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2017391 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2018131 (#14089) * Bugfix: Ref assemblies contained .property definitions at the wrong type in generated IL (#14093) * Bugfix: Ref assemblies contained property definitions at the wrong type * Better comment * Update versions for dev17.4 (#14102) * Update versions for dev17.5 (#14100) * Merge main to release/dev17.5 (#14098) * Add SynType.Or. (#14058) * Add SynType.Or for generic constrains in the form (^A or ^B):... * Change ty1.Equals(ty2) to call static op_Equality (#13028) Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Don Syme <[email protected]> Co-authored-by: Florian Verdonck <[email protected]> Co-authored-by: Rustam <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Don Syme <[email protected]> * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2023680 (#14133) * Disable ref assemblies in e2e tests (#14135) Disable reference assemblies for e2e test of type providers Co-authored-by: Adam Boniecki <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2024009 (#14139) * Downgrade SDK, rc2 is failing signing (#14146) * Generate SBOM for Fsharp (#14029) (#14169) * Generate Sbom * pass ci flag * update * Sbom generation * Fix for trimming tests: Added nuget.org source + explicit source mapping for FSharp.Core * Update Build.ps1 Tweaks to handle useGlobalNugetCache Co-authored-by: Kevin Ransom (msft) <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Kevin Ransom (msft) <[email protected]> Co-authored-by: Epsitha Ananth <[email protected]> Co-authored-by: Kevin Ransom (msft) <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Kevin Ransom (msft) <[email protected]> * Update Arcade & Put automated pool provider usage functionality into Dev17.4 branch (similar to PR in main now but will not be backported here) (#14177) * [release/dev17.4] fix metadata failure due to double integration of signature (#14190) * fix metadata failure due to double duplication * fix metadata failure due to double duplication Co-authored-by: Don Syme <[email protected]> * Global.json * [release/dev17.4] Don't emit IsReadOnlyAttribute if not available. (#14281) Co-authored-by: nojaf <[email protected]> * Fixed package versions to publicly available (#14291) * Fixed package versions to publicly available * Update Versions.props Microsoft.Build.* to 17.4.0 Co-authored-by: Kevin Ransom (msft) <[email protected]> * Update Versions.props * Prefer nullable over other conversions, fixes #14302 * Replace ROSpan for DateTimeOffset as op_Implicit target, ROSpan is not defined on all test TFMs * [release/dev17.4] F# 7 fixes (#14322) * WIP: Fix for calling init-only setter via srtp call + allow calling special-named functions via srtp * Fix 14097 Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Tomas Grosup <[email protected]> Co-authored-by: Don Syme <[email protected]> * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2051937 (#14381) * Deploy System.Diagnostics.DiagnosticSource to Tools folder (#14417) * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2059214 (#14427) * Revert "IL: optimize attribute cluster reading (#13821)" This reverts commit 179db4e. * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2067933 (#14472) * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2067933 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2067933 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2068561 (#14474) * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2068115 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2068115 Co-authored-by: Tomas Grosup <[email protected]> * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2077478 (#14533) * Revert "Merge branch 'release/dev17.5' of https://github.com/dotnet/fsharp into release/dev17.5" This reverts commit 4d222de, reversing changes made to 2e92791. * Update azure-pipelines.yml * Merge main to release/dev17.5 (#14562) Co-authored-by: Petr <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: Don Syme <[email protected]> Co-authored-by: Tomas Grosup <[email protected]> Co-authored-by: Edgar Gonzalez <[email protected]> Co-authored-by: Eugene Auduchinok <[email protected]> Co-authored-by: Chet Husk <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Florian Verdonck <[email protected]> Co-authored-by: Petr <[email protected]> Co-authored-by: Petr Pokorny <[email protected]> Co-authored-by: Theodore Tsirpanis <[email protected]> Co-authored-by: Kevin Ransom (msft) <[email protected]> Co-authored-by: Rustam <[email protected]> Co-authored-by: Adam Boniecki <[email protected]> Co-authored-by: Adam Boniecki <[email protected]> Co-authored-by: Epsitha Ananth <[email protected]> Co-authored-by: Kevin Ransom (msft) <[email protected]> Co-authored-by: Matt Galbraith <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Nino Floris <[email protected]>
A inner check for generating properties in reference assemblies was checking existence of generated methods.
However, before the fix, methods were only generated AFTER the property itself.
This lead to strange shift of property definitions, putting them to the very last type of a file.
Besides adding tests dedicated for this scenario, some existing tests had assertions for the (broken) IL - after knowing that the issue is in assignment of property into the wrong class, it was very visible.