Skip to content

packed modtype lookup can fail due to missing cmi#9409

Merged
Octachron merged 1 commit intoocaml:trunkfrom
Octachron:yet_another_implicit_deps_error
Apr 17, 2020
Merged

packed modtype lookup can fail due to missing cmi#9409
Octachron merged 1 commit intoocaml:trunkfrom
Octachron:yet_another_implicit_deps_error

Conversation

@Octachron
Copy link
Copy Markdown
Member

@Octachron Octachron commented Mar 31, 2020 •
edited
Loading

Closes #9406:

This PR makes module types from missing cmis behaves the same as abstract module type everywhere.

This was already the case in nearly all cases, except for a forgotten case in the function Typemod.modtype_of_package.

After this change, there is no call site of Env.find_modtype that distinguish between the case None and exception Not_found. It would probably make sense to fold the two cases into one.

@Octachron Octachron mentioned this pull request Mar 31, 2020
Closed
@Drup
Copy link
Copy Markdown
Contributor

Drup commented Mar 31, 2020

Sounds good. This fixes the bug and make the behavior consistent again. The patch and tests look fine.

I would prefer if missing cmis were actually an error, but that's a contentious topic for another time. :)

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 31, 2020

It would probably make sense to fold the two cases into one.

You are proposing that Env.find_modtype would never raise Not_found and only return None? (Are you planning to submit a PR?)

@Octachron
Copy link
Copy Markdown
Member Author

Octachron commented Mar 31, 2020

Rereading the code, it would rather be a version of find_modtype_expansion that returns an option rather than raise a Not_found exception.

Maybe, we could split find_modtype into find_modtype_decl for finding the declaration of the module type and find_modtype for looking up just the type with the Not_found -> abstract translation.
I am willing to submit a PR if we agree on a design.

@Octachron
Copy link
Copy Markdown
Member Author

Octachron commented Apr 16, 2020

Reviewing the fix, I would prefer to start with integrating the current bugfix PR (to integrate the test) before potentially moving on to a refactoring PR.

gasche
gasche approved these changes Apr 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

The fix looks obviously correct. Approving. This is good to go if the CI passes and the patchset is rebased appropriately, please feel free to merge. (I will probably forget to do it myself.)

@Octachron Octachron force-pushed the yet_another_implicit_deps_error branch 2 times, most recently from f55ba91 to e724b37 Compare April 16, 2020 16:49
@Octachron
packed modtype lookup can fail due to missing cmi
35dbd85 
This commit removes an assert false and makes the missing
cmi case behaves like the abstract case.
@Octachron Octachron force-pushed the yet_another_implicit_deps_error branch from e724b37 to 35dbd85 Compare April 17, 2020 09:24
@Octachron Octachron merged commit 9108648 into ocaml:trunk Apr 17, 2020
@lpw25 lpw25 mentioned this pull request Jun 25, 2020
Merged
@kit-ty-kate kit-ty-kate mentioned this pull request Nov 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gasche gasche gasche approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

File "typing/typemod.ml", line 1817, characters 27-33: Assertion failed

3 participants

@Octachron @Drup @gasche