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

Module inclusion check looks at module mode #3328

Merged
merged 6 commits into from
Dec 13, 2024
Merged

Conversation

riaqn
Copy link
Contributor

@riaqn riaqn commented Nov 29, 2024

Value description inclusion check now considers the mode of the modules, as opposed to looking at the value modalities only.

This PR is surprisingly simple, because modules are currently fixed to be legacy. When we support non-legacy modules in the future, we will have to pipe those data through.

Based on this PR, I will shortly make another PR that simplifies inferred modalities.

@riaqn riaqn added typing modes Work on modes. There's some overlap with the `multicore` label, but not strictly so. labels Nov 29, 2024
typing/includecore.ml Outdated Show resolved Hide resolved
@riaqn riaqn requested review from lpw25 and removed request for lpw25 November 29, 2024 17:24
Copy link
Collaborator

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

I think the current behaviour isn't quite right for module type definitions.

typing/includecore.ml Outdated Show resolved Hide resolved
typing/includecore.ml Outdated Show resolved Hide resolved
typing/includecore.ml Show resolved Hide resolved
@riaqn riaqn force-pushed the modal-inclusion-check branch from 524e1c7 to 8c1d017 Compare December 6, 2024 15:13
@riaqn
Copy link
Contributor Author

riaqn commented Dec 6, 2024

Had an offline discussion - we now distinguish between module type declaration vs module/value declaration, when performing inclusion check. Only the latter is affected by this PR.

@riaqn riaqn force-pushed the modal-inclusion-check branch 2 times, most recently from 3b0a802 to 7b58880 Compare December 9, 2024 15:36
@riaqn riaqn requested a review from lpw25 December 9, 2024 16:14
Copy link
Collaborator

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

I think it would be worth tidying up a few things, and I think the behaviour should be different for functors within module types.

typing/includemod.ml Outdated Show resolved Hide resolved
typing/includecore.mli Outdated Show resolved Hide resolved
typing/includecore.ml Outdated Show resolved Hide resolved
typing/includecore.ml Outdated Show resolved Hide resolved
typing/includemod.ml Outdated Show resolved Hide resolved
typing/includemod.mli Outdated Show resolved Hide resolved
@riaqn riaqn force-pushed the modal-inclusion-check branch from c6bba38 to f7d2871 Compare December 13, 2024 14:35
@riaqn riaqn force-pushed the modal-inclusion-check branch from c4ad48d to 33df433 Compare December 13, 2024 14:50
@riaqn
Copy link
Contributor Author

riaqn commented Dec 13, 2024

This is ready for another look now - commits "correct semantics" and "address comments".
The whole-PR diff is also minimized (since some code are added and then removed).

Copy link
Collaborator

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

A couple of small suggestions, but this is good to go.

testsuite/tests/typing-modes/val_modalities.ml Outdated Show resolved Hide resolved
typing/includemod.ml Outdated Show resolved Hide resolved
@riaqn riaqn force-pushed the modal-inclusion-check branch from 14e82df to 655a6eb Compare December 13, 2024 17:06
@riaqn riaqn changed the title Value description inclusion check looks at module mode Module inclusion check looks at module mode Dec 13, 2024
@riaqn riaqn merged commit 83b13be into main Dec 13, 2024
20 checks passed
@riaqn riaqn deleted the modal-inclusion-check branch December 13, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modes Work on modes. There's some overlap with the `multicore` label, but not strictly so. typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants