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

bugfix: merge involving constructor deletion #5110

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Conversation

mitchellwrosen
Copy link
Member

@mitchellwrosen mitchellwrosen commented Jun 20, 2024

Overview

This PR attempts to fix a bug in merge reported at #5092.

The syntactic hashing of constructor referents was wrong, at least in the context of how the rest of the algorithm used the diff computed by syntactic hashes.

Prior to this PR, we computed the syntactic hash of a constructor as equivalent to the term that represents calling that constructor.

For example, if the namespace contains the name

Just = #Maybe#1

then the syntactic hash of that referent was computed as the syntactic hash of the term whose body is just #Maybe#1.

Thus, when updating

type Foo = Bar | Baz

to

type Foo = Bar

the diff would show an update on the type Foo but not the constructor Bar.

There may be a design in which that works out ok, but not in ours: we take the adds, updates, and deletes from the diff and lay them over the LCA. In this example, we'd end up with an updated Foo without a name for its new constructor Bar.

So, the fix I implemented is to change how we compute syntactic hashes of constructors. Now, it's simply equivalent to the syntactic hash of its type declaration (which itself would change if any constructors change in shape or name). Therefore, the constructors of a type that undergoes an update would appear in the diff as updates as well.

Additionally, I noticed while reading the syntactic hashing code that we computed the hash tokens of a constructor reference differently than a normal term reference. That seemed like a mistake, unrelated to this bug, which could cause unnecessary conflicts. So, I fixed that too.

Test coverage

I've added a regression test to the main merge.md transcript. You can see its results flip from incorrect to correct right here: prior to this PR, merge erroneously dropped the name for a constructor, and now it doesn't.

Loose ends

Link to related issues that address things you didn't get to. Stuff you encountered on the way and decided not to include in this PR.

@mitchellwrosen mitchellwrosen marked this pull request as ready for review June 20, 2024 19:35
@aryairani aryairani merged commit 25c4e6e into trunk Jun 20, 2024
20 checks passed
@aryairani aryairani deleted the fix-merge-bug branch June 20, 2024 19:49
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.

None yet

2 participants