Skip to content

Conversation

@abonie
Copy link
Member

@abonie abonie commented Jun 28, 2023

This will fix secondary issue reported here #6411

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 abonie marked this pull request as ready for review June 30, 2023 10:59
@abonie abonie requested a review from a team as a code owner June 30, 2023 10:59
Copy link
Contributor

@psfinaki 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
T-Gro previously requested changes Jun 30, 2023
Copy link
Member

@T-Gro 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 abonie marked this pull request as draft June 30, 2023 15:17
@abonie abonie force-pushed the fix-anonrecd-nested-with-label-overlap branch from 751ec73 to 4b4e365 Compare July 4, 2023 15:34
@abonie abonie marked this pull request as ready for review July 11, 2023 14:56
@abonie abonie requested review from T-Gro and psfinaki July 11, 2023 14:56
@abonie
Copy link
Member Author

abonie commented Jul 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.

Copy link
Contributor

@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.

Looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants