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

Support more stack_ syntax #3475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support more stack_ syntax #3475

wants to merge 1 commit into from

Conversation

riaqn
Copy link
Contributor

@riaqn riaqn commented Jan 15, 2025

This PR allows stack_ at more places:

let stack_ x = ...
let stack_ x : _ = ...
let stack_ f a b c = ...

Copy link

github-actions bot commented Jan 15, 2025

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).

@mshinwell mshinwell added the locals Local allocation label Jan 15, 2025
@riaqn
Copy link
Contributor Author

riaqn commented Jan 15, 2025

Don't review yet - we need to support syntax currying...

let stack_ f : int -> int -> int = ... in

the stack_ keyword should influence the interpretation of int -> int -> int.

@goldfirere
Copy link
Collaborator

I haven’t looked at the code here, just the PR initial post… but is there a design doc for this? I’m worried we’re diluting the nice, simple meaning of stack_, which I thought was meant to apply only at an allocation site. (I could see allowing let stack_ f x y z = …, where the stack_ is meant to apply to the implicit fun. But I don’t know what it would mean for the other forms.)

@riaqn
Copy link
Contributor Author

riaqn commented Jan 16, 2025

but is there a design doc for this?

TBF there is no documentation for stack_ in the first place, which is why it's not announced. I found this usability issue when adding stack_ documentation, which is at #3479

let stack_ x = .. is parsed as let x = stack_ (..), so all the typing rules stay the same, and everything is still nice and simple.

@lpw25
Copy link
Collaborator

lpw25 commented Jan 16, 2025

I agree with @goldfirere: let stack_ f a b c = ... is good and worth including, the others are probably not a good idea.

@riaqn
Copy link
Contributor Author

riaqn commented Jan 16, 2025

Discussed with Richard and agreed that let stack_ a = .. is not a good idea (because stack_ work shallowly, which is not obvious in this syntax). Will consult others for opinion on let stack_ f a b = ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
locals Local allocation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants