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

Refactor new mode syntax #3522

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Refactor new mode syntax #3522

wants to merge 2 commits into from

Conversation

riaqn
Copy link
Contributor

@riaqn riaqn commented Jan 28, 2025

This PR refactors the new mode syntax (@ and @@).

  • With this PR, @ always means modes, and @@ always mean modalities. Previously, @@ can mean modes in some position for disambiguition. For example, x : a -> a @@ local says x is local, and x : b -> a @ local says a is local). After this PR, the first example will write x : (a -> a) @ local. The second example doesn't change. You can also write x : a @ local.
  • To ensure certain parsing-printing roundtrip, we print modes in legacy position only if the whole mode annotation contains old modes only. That means local_ a @ portable -> b will print back a @ local portable -> b, while local_ a -> b prints back the same. The idea is that "if the user already knows about and uses the new mode syntax, then we should print back new mode syntax". This is done in both pprintast.ml and printtyp.ml.
  • fun a b : ty @@ modes -> .. was introduced by Supports mode annotation on function body #3327 , and would be changed to fun a b : ty @ modes -> ... Note the ambiguity: the arrow could be arrow type. We decide to remove this for simplicity.
  • Refactoring and fixes in parser.mly and pprintast.ml, so that some missing corner cases are covered, and diff to the upstream is minimized. For example, labeled_simple_pattern is now almost identical to upstream. This allows systematic enumeration of all possible syntax, which helps with user documentation, and test coverage.
  • The mode section in source_jane_street.ml is rewritten to cover all possible syntax.

TODO: update documentation.

Suggestions to reviewers (Not ready for review)

  • First look at the mode section of source_jane_street.ml, to get a sense of the new syntax (don't look at the type errors - they are fine).
  • Then look at parser.mly.
  • Then look at pprintast.ml and printtyp.ml. The former should be functionally "most fine" as tested by source_jane_street.ml, but other aspects such as coding style should be inspected.
  • Changes in tests, except source_jane_street.ml, are all mechanical and can be skipped.

Copy link

github-actions bot commented Jan 28, 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).

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

Successfully merging this pull request may close these issues.

1 participant