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 3 #119

Merged
merged 15 commits into from
Nov 15, 2024
Merged

Merge 5.2.0minus 3 #119

merged 15 commits into from
Nov 15, 2024

Conversation

liam923
Copy link
Contributor

@liam923 liam923 commented Nov 13, 2024

This PR merges compiler changes to bring Merlin up to date with 5.2.0minus-3. This was a complicated merge for a number of reasons:

  • It is the first merge since the compiler changed its directory layout
  • There were a lot of conflicts due to the removal of jane syntax
  • There was a compiler change that caused a dependency cycle in Merlin. More on that below.
  • The addition of default modalities in signatures broke a lot of Merlin-specific code.

Regarding the dependency cycle: A compiler change made utils/language_extension.ml(i) depend on parsing/location.ml(i). There are other files in parsing/ that depend on files in utils/, so there is a mutual dependency between files in parsing/ and utils/. While this compiles fine in the compiler, this dependency structure is problematic in Merlin because Merlin compiles each directory as an individual library. Rather than performing dune magic to resolve this, I opened a PR on the compiler that moves utils/language_extension.ml(i) to parsing/language_extension.ml(i). The compiler version used for this merge is the tip of that PR rather than the tagged version.

Here's my review instructions:

  • Review the first commit, which updates the import scripts to be able to handle the new directory structure. Actually, it's a bit worse than that. It needs to be able to handle two different directory structures, since for this merge, the old version uses the old structure and the new version uses the new structure.
  • Gloss over the next three commits (3052ea2, bd98c3c, 2aa9ac6), which import compiler changes and commit conflicts. I would usually say to skip them, except that this is the first run of the import script since the changes. Thus, it's worth doing some sort of spot-check.
  • Review the remaining commits.

Here's some notes about some of the commits:

  • 50305c2 (Remove files that were deleted in flambda): The import script doesn't handle file removals or moves well (I've created tickets internally to fix this). This manually deals with some such files.
  • 49edf67 (Apply old diff to language_extension): The Language_extension module moved locations as a result of this merge (see above note about dependency cycle). This manually ports over some Merlin-specific changes to that file.
  • e28fc52 (Update magic number script): The compiler changed how it handles magic numbers since the last merge. This commit updates the script that bumps magic numbers to reflect this.

@liam923 liam923 requested a review from goldfirere November 13, 2024 21:51
@goldfirere
Copy link
Contributor

I’m very much on the back foot right now. I nominate @dkalinichenko-js to review. I know she does not have (much?) experience in merlin, but I think the nature of the changes here does not require that much experience. Diana, if there are parts that want further review, please highlight and I can take a look. Thanks!

@dkalinichenko-js
Copy link

The subdirectory bits in import scripts will be unnecessary after this is merged, right? Should we leave a CR to clean them up?

@liam923
Copy link
Contributor Author

liam923 commented Nov 14, 2024

@dkalinichenko-js yes, but imo it's best to keep it around incase we ever do a change in the future. I'll leave a comment saying something to this effect.

@dkalinichenko-js
Copy link

@goldfirere could you look at 2d99ac1, especially the combine_sigs function? Everything else look good to me.

Copy link
Contributor

@goldfirere goldfirere left a comment

Choose a reason for hiding this comment

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

Quick review, focusing on combine_sigs. Some style suggestions, but all looks correct.

| Tvar { name = None; _ } | Tunivar { name = None; _ } -> Typ.any ()
| Tvar { name = Some s; _ } | Tunivar { name = Some s; _ } -> Typ.var s
| Tvar { name = None; jkind = _ } | Tunivar { name = None; jkind = _ } ->
(* CR modes: do something better here with the jkind *)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems easy enough to do better -- but maybe the point is that we're not sure what we want to display to users? (This doesn't need to hold up the merge.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what we want to display to the user, and I'd rather print the jkind to little than too often (this is only used for construct), so I think it's best to just not include the kind for now. I also suspect this might be easier to do once ocaml-flambda/flambda-backend#3224 is merged.

src/analysis/ptyp_of_type.ml Outdated Show resolved Hide resolved
src/kernel/extension.ml Show resolved Hide resolved
@liam923 liam923 merged commit 23a8ce8 into main Nov 15, 2024
2 checks passed
@liam923 liam923 deleted the merge-5.2.0minus-3 branch November 15, 2024 19:50
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.

3 participants