AnonRecdTypeInfo was concatenating all of the record's sorted labels to create a key for some post inference check. If for two records concatenating their labels gave the same result, then the key was also the same which resulted in some collisions.

abonie marked this pull request as ready for review

June 30, 2023 10:59

abonie requested a review froma team as acode owner

June 30, 2023 10:59

psfinaki approved these changes

Jun 30, 2023

View reviewed changes

Copy link

Member

psfinaki left a comment

Choose a reason for hiding this comment

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

Nice one

T-Gro reviewed

Jun 30, 2023

View reviewed changes

tests/FSharp.Compiler.ComponentTests/EmittedIL/Misc/AnonRecd.fs.il.net472.bsl OutdatedShow resolvedHide resolved

T-Gro previously requested changes

Jun 30, 2023

View reviewed changes

Copy link

Member

T-Gro left a comment

Choose a reason for hiding this comment

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

Postponing until we get a clarification/test/.. on how this affects name stability.

The RFC explicitely mentions also here:
This name must never change in future F# compilers

abonie marked this pull request as draft

June 30, 2023 15:17

abonie added5 commits

July 3, 2023 15:34

Add unit test

74369e8

Fix AnonRecdTypeInfo creation

e141749

Add a separator in between labels' bytes to prevent later hash collision

Separate stamp and key used in postinference check

3975058

Remove some whitespace

2788ed2

Replace stamp with fixed name for IL gen

4b4e365

abonie force-pushed thefix-anonrecd-nested-with-label-overlap branch from751ec73 to4b4e365Compare

July 4, 2023 15:34

abonieand others added4 commits

July 4, 2023 17:34

Merge branch 'main' into fix-anonrecd-nested-with-label-overlap

dc194a6

Fix instantiation of AnonRecdTypeInfo

47658f2

New property (IlTypeName) needs to be set on creation

Comment on not changing generated IL type name

b86e3ee

Merge branch 'main' into fix-anonrecd-nested-with-label-overlap

9c88985

abonie marked this pull request as ready for review

July 11, 2023 14:56

abonie requested review fromT-Gro andpsfinaki

July 11, 2023 14:56

Copy link

MemberAuthor

abonie commentedJul 11, 2023

Postponing until we get a clarification/test/.. on how this affects name stability.
The RFC explicitely mentions also here:This name must never change in future F# compilers

I have addressed this now. It's a bit clunky - we use new modified stamps everywhere except for generating type names, where we use the old recipe - just to make sure it does not affect backward compatibility.