-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
There was a problem hiding this 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.
524e1c7
to
8c1d017
Compare
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. |
3b0a802
to
7b58880
Compare
There was a problem hiding this 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.
c6bba38
to
f7d2871
Compare
c4ad48d
to
33df433
Compare
This is ready for another look now - commits "correct semantics" and "address comments". |
There was a problem hiding this 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.
14e82df
to
655a6eb
Compare
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.