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

fix for net7 regression for UMX #12892

Merged
merged 2 commits into from
Mar 30, 2022
Merged

fix for net7 regression for UMX #12892

merged 2 commits into from
Mar 30, 2022

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Mar 28, 2022

Fix the core problem with #12881

As background, unit-annotated versions of primitive types are defined by:

    [<MeasureAnnotatedAbbreviation>]
    type SomeType<[<Measure>] 'm> = SomeType

This is primarily used on FSharp.Core to define float<'m>, int64<'m> and so on, but can also be used for unit-annotated versions of primitives like GUID - this is done in the UMX library.

For these types, the logic of the F# compiler must report which interfaces are implemented by the unitized versions of this type.
In general we preserve the interfaces of the underlying representation type unchanged, however there is an exception: we want the unitized types to support comparison and equality with corresponding unitized values.

So the unitized version of the type is considered to support the following interfaces. The adjustment in this PR is shown in bold

  1. IComparable<SomeType<'m>>
  2. IEquatable<SomeType<'m>>
  3. All other interfaces supported by the underlying representation SomeType except those derived from IComparable<_> or IEquatable<_>

This fixes #12881.

@dsyme dsyme changed the title [WIP] 12881: start of fix for net7 regression for UMX fix for net7 regression for UMX Mar 28, 2022
@dsyme
Copy link
Contributor Author

dsyme commented Mar 29, 2022

This is ready

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@dsyme
Copy link
Contributor Author

dsyme commented Mar 29, 2022

Note testing for MeasureAnnotatedAbbreviation is weak because it was mostly intended as a feature for FSharp.Core, and is normally tested by the bootstrap of the compiler. But the bootstrap is currently only on .NET 6.

@vzarytovskii
Copy link
Member

vzarytovskii commented Mar 29, 2022

So, in worst case scenario, for any TMeasureableRepr, we will now be traversing the whole interface hierarchy?

Edit: it's also probably worth it to add such tests to https://github.com/dotnet/performance.

//
// This rule is conservative and only applies to IComparable and IEquatable interfaces.
//
// This rule may in future be extended to rewrite the "trait" interfaces associated with .NET 7.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we'll probably want to revisit what are we looing for, once it's stabilized in .NET7+

@dsyme
Copy link
Contributor Author

dsyme commented Mar 30, 2022

So, in worst case scenario, for any TMeasureableRepr, we will now be traversing the whole interface hierarchy?

Given these growing interface lists, I think the best precautionary mitigation (not just for this issue, but in general) will be to add an InfoReader cache to memoize GetImmediateInterfacesOfType - we do that for nearly all the other operations (e.g. looking up methods, properties) that traverse the hierarchy. We could also do it for AllSuperTypesOfType, AllInterfacesOfType, ExistsHeadTypeInEntireHierarchy

As a general point, the InfoReader caches seem to have been a successful technique historically - the memoized routines very rarely appear in profiling runs and the memoization tables never seem to occupy significant resources in memory profiles (though we haven't done a deep check of that). The caches get recreated often in IDE situations (see many calls to let infoReader = InfoReader(g,amap)) to it seems good as a way for avoiding recomputation of traverses within the scope of a particular typecheck of a file or similar.

@vzarytovskii @KevinRansom Might make a good thing to pair-program

@dsyme dsyme merged commit 4ac9295 into dotnet:main Mar 30, 2022
charlesroddie pushed a commit to charlesroddie/fsharp that referenced this pull request May 2, 2023
* fix net7 regression

* fix test
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.

FSharp.UMX no longer compiles on .NET 7
3 participants