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

Merge 5.2.0minus-5 #124

Merged
merged 26 commits into from
Jan 20, 2025
Merged

Merge 5.2.0minus-5 #124

merged 26 commits into from
Jan 20, 2025

Conversation

liam923
Copy link
Contributor

@liam923 liam923 commented Jan 16, 2025

Merge flambda changes for 5.2.0minus-5. The first two commits run the import script and commit merge conflicts - I recommend skipping them when reviewing.

This PR was difficult for two reasons:

  1. Unboxed types caused a lot of conflicts, and broke some Merlin code. For now, the Merlin support for unboxed types is not very good. I added a test file that demonstrates what works and what doesn't. I've internally made a ticket to improve this state of affairs.
  2. Due to overwriting, the compiler added "holes". The compiler's notion of holes is for statements like overwrite (3,4) with (_, 5). New nodes are added to the parsetree and typedtree: Pexp_hole and Texp_hole, respectively. This partially conflicts with Merlin's idea of typed holes. Before, Merlin used an extension node to represent holes in the parsetree, and in the typedtree, it also added a Texp_hole constructor. I resolve this by using the new Pexp_hole in place of an extension node (so Pexp_hole represents either type of hole) and renaming Merlin's Texp_hole to Texp_typed_hole (thus, there is now both Texp_hole and Texp_typed_hole in Merlin). I've submitted a PR to upstream Merlin to rename Texp_hole to Texp_typed_hole to mitigate conflicts in the future. (I also renamed Tmod_hole to Tmod_typed_hole for consistency - I included this in the upstream PR as well.)

@liam923 liam923 force-pushed the merge-5.2.0minus-5 branch from 14d1873 to 2678281 Compare January 16, 2025 19:36
@liam923 liam923 marked this pull request as ready for review January 16, 2025 22:26
@liam923 liam923 requested a review from goldfirere January 16, 2025 22:26
@liam923 liam923 changed the title Merge 5.2.0minus 5 Merge 5.2.0minus-5 Jan 16, 2025
@liam923
Copy link
Contributor Author

liam923 commented Jan 17, 2025

I'm a bit confused by the CI failure. The same test passes when I run locally, and I haven't yet been able to reproduce. This test has been known to be flaky in the past, so I re-ran it twice, but there's two test cases that have consistently failed across all three runs. I'm tempted to comment out those tests for now so that they don't block rolling Merlin (the justification being that they were already flaky before).

@goldfirere
Copy link
Contributor

Those tests shouldn't be flaky -- they're an output of type-checking, which should be deterministic. That said, I'm not shocked that this has changed as we've evolved the type checked. The tests are looking to see where we allocate a cell that never gets used, which is a bit unusual. I think it's fine to accept the new output. But I wouldn't comment out the tests!

tests/test-dirs/no-escape.t/run.t Show resolved Hide resolved
src/frontend/query_commands.ml Show resolved Hide resolved
src/analysis/env_lookup.ml Outdated Show resolved Hide resolved
src/analysis/completion.ml Outdated Show resolved Hide resolved
@liam923
Copy link
Contributor Author

liam923 commented Jan 20, 2025

I'm hitting merge without waiting for replies to my replies in order to roll this Merlin version. If more changes are needed, please lmk and I'll make them in a separate PR. (I don't foresee any requested changes being ones that should block rolling)

@liam923 liam923 merged commit 9f0a6b5 into main Jan 20, 2025
1 of 2 checks passed
@liam923 liam923 deleted the merge-5.2.0minus-5 branch January 20, 2025 14:43
@goldfirere
Copy link
Contributor

Yes all good. Thanks!

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.

2 participants