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.

Sign up for GitHub

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

Jump to bottom

Consider modalities when adding baggage to with-kinds #3401

Open
wants to merge 11 commits into
base: rae/with-kinds
Choose a base branch
from
Open

Consider modalities when adding baggage to with-kinds #3401

wants to merge 11 commits into from

Conversation

glittershark
Copy link
Member

@glittershark glittershark commented Dec 20, 2024 •
edited
Loading

If a field has a constant modality, or a type is annotated with a constant
modality, then skip that type when adding it as baggage to the kind, both when
converting user-written kind annotations and inferring kinds on types.

The guts of this is a big, ugly pattern match on Jkind_axis and
Modes.Modality.Const.axis, which ideally can go away or get much simpler with a
refactor to unify these two types, but I decided to do it this way for now to
get something working that's known-correct, and refactor under green

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 20, 2024 •
edited by glittershark
Loading

Parser Change Checklist

This PR modifies the parser. Please check that the following tests are updated:

  • parsetree/source_jane_street.ml

This test should have examples of every new bit of syntax you are adding. Feel free to just check the box if your PR does not actually change the syntax (because it is refactoring the parser, say).

@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch from d0f9538 to 7f8173c Compare December 20, 2024 19:44
@glittershark glittershark changed the title consider modalities when adding baggage to with-kinds Consider modalities when adding baggage to with-kinds Dec 20, 2024
@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch from 7f8173c to a45f29f Compare December 20, 2024 20:43
@glittershark glittershark marked this pull request as ready for review December 20, 2024 20:47
@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch 3 times, most recently from 1e1efeb to 16d4feb Compare December 20, 2024 21:49
@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch 2 times, most recently from 6a29bd9 to 65e7ba6 Compare December 24, 2024 05:59
@goldfirere goldfirere force-pushed the aspsmith/with-kind-modalities branch from 65e7ba6 to 74cc04a Compare December 24, 2024 17:08
@goldfirere
Copy link
Collaborator

I've just force-pushed to accommodate the force-push to #3284.

goldfirere
goldfirere reviewed Dec 24, 2024
View reviewed changes
testsuite/tests/parsetree/source_jane_street.ml Outdated Show resolved Hide resolved
testsuite/tests/typing-jkind-bounds/basics.ml Outdated Show resolved Hide resolved
testsuite/tests/typing-jkind-bounds/basics.ml Outdated Show resolved Hide resolved
testsuite/tests/typing-jkind-bounds/modalities.ml Outdated Show resolved Hide resolved
testsuite/tests/typing-jkind-bounds/modalities.ml Outdated Show resolved Hide resolved
typing/jkind_axis.mli Show resolved Hide resolved
typing/jkind_axis.ml Outdated Show resolved Hide resolved
typing/jkind_axis.ml Outdated Show resolved Hide resolved
typing/jkind.mli Outdated Show resolved Hide resolved
typing/jkind.ml Outdated Show resolved Hide resolved
@goldfirere
Copy link
Collaborator

goldfirere commented Dec 24, 2024 •
edited
Loading

(This should not be merged until #3284 is reviewed, so there's no rush here.)

@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch 3 times, most recently from 737a2a8 to c01dc5d Compare December 27, 2024 15:28
@glittershark glittershark mentioned this pull request Dec 28, 2024
Open
42 tasks
@glittershark
Copy link
Member Author

glittershark commented Dec 28, 2024 •
edited by goldfirere
Loading

LGTM, I think

@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch 2 times, most recently from 9354afe to 7da1766 Compare December 28, 2024 20:53
@riaqn
Copy link
Contributor

riaqn commented Jan 6, 2025

Thanks for the PR - I think in the long run, in type checker, the baggage will contain a list of types, each with a modality (could be id or constant modalities). And I think the simplification about identity modalities and constant modalities only needs to happen at printing. Does that sounds about right?

@glittershark
Copy link
Member Author

Thanks for the PR - I think in the long run, in type checker, the baggage will contain a list of types, each with a modality (could be id or constant modalities). And I think the simplification about identity modalities and constant modalities only needs to happen at printing. Does that sounds about right?

yep, that sounds right to me

@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch from 7df828b to 66bd480 Compare January 13, 2025 22:42
@mshinwell mshinwell added the modes Work on modes. There's some overlap with the `multicore` label, but not strictly so. label Jan 15, 2025
glittershark and others added 11 commits January 23, 2025 11:38
@glittershark
Add modalities to With in the parsetree
e95d732 
Parse and represent optional modalities on `with` in jkind annotations. This
supports jkind annotations like:

    value mod portable many uncontended with 'a @@ uncontended with int
@glittershark
Consider modalities when adding baggage to with-kinds
64d2295 
If a field has a constant modality, or a type is annotated with a constant
modality, then skip that type when adding it as baggage to the kind, both when
converting user-written kind annotations and inferring kinds on types.

The guts of this is a big, ugly pattern match on Jkind_axis and
Modes.Modality.Const.axis, which ideally can go away or get much simpler with a
refactor to unify these two types, but I decided to do it this way for now to
get something working that's known-correct, and refactor under green
@goldfirere @glittershark
Accept new test output
066706e
@goldfirere @glittershark
Modalities in unboxed records (+ a comment)
a489fa1
@glittershark
Make modality arg on add_baggage non-optional
144a3c5 
This isn't nearly as ugly (in predef) as I expected.
@glittershark
promote tests
b3dd858
@glittershark @goldfirere
Update typing/jkind_axis.mli
fadde60 
Co-authored-by: Richard Eisenberg <[email protected]>
@glittershark
split iarray test in two
a9cb53d
@glittershark
Delete Jkind_axis.Modal in favor of Mode.Alloc.axis (#3409)
93bcb54
@glittershark
use the right modality
e513c32
@glittershark
format
0ef90f9
@glittershark glittershark force-pushed the aspsmith/with-kind-modalities branch from 22fbe6a to 0ef90f9 Compare January 23, 2025 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goldfirere goldfirere goldfirere approved these changes

Assignees
No one assigned
Labels
modes Work on modes. There's some overlap with the `multicore` label, but not strictly so.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@glittershark @goldfirere @riaqn @mshinwell