Convert legacy local_/global_ to new mode/modality syntax#143
Open
riaqn wants to merge 8 commits into
Open
Conversation
riaqn
commented
May 22, 2026
Comment on lines
+1726
to
+1727
| ( fmt_pattern ~parens:false c xpat | ||
| $ fmt_if islocal "@ @@ local" ) ) ) |
Author
There was a problem hiding this comment.
merge_islocal_into_existing_modes tries to push the local into the existing list of modes, and if that fails we still print @ local here.
It's dirty, but if we consistently push the local inward I encounter some "unable to stablize" error due to print-box issues.
Given that we will remove all of this in the very next roll, I think it's fine.
riaqn
commented
May 22, 2026
| (Params.Indent.exp_constraint c.conf) | ||
| (Params.parens_if (parens && has_attr) c.conf | ||
| ( wrap_fits_breaks ~space:false c.conf "(" ")" | ||
| ( fmt_expression c (sub_exp ~ctx sbody) |
Author
There was a problem hiding this comment.
we don't push modes into sbody, causing nested parens (see added tests). Pushing it into sbody would complicate the code and probably not worthwhile, given that we are removing legacy syntax very soon anyway.
Format Jane Street legacy `local_`/`global_` annotations using the new mode/modality syntax: `local_ exp` → `(exp : @ local)`, `(local_ pat)` → `(pat @ local)`, `let local_ x = ...` → `let x @ local = ...`, `global_ c : t` → `c : t @@ global`, `C of global_ T` → `C of T @@ global`, `local_ T -> U` → `T @ local -> U` (arrow params and return). Add normalizations in Normalize_extended_ast so the legacy and new representations compare equal during the AST round-trip check. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The formatter's [type_constr_and_body] transformation moves a function body's [Pexp_constraint(_, None, modes)] into the function's [ret_mode_annotations]. The std parser distinguishes these forms, so add a normalization rule that hoists body modes to ret_mode_annotations when the function has no other constraints, mirroring the existing rule for [Pexp_constraint(_, Some ty, [])]. With this in place, the extended AST normalizations for [Pexp_apply(local_, e)], [pvb_local], record/constructor [global_], and arrow [local_] are no longer needed -- the std parser already canonicalizes those legacy/new representations to the same shape. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sugar.of_let_binding now always converts pvb_local=true into a [Mode "local"] entry in pvb_modes, so the lb_local field is always false at the formatter level. Drop the field and the now-unreachable fmt_if lb_local " local_" emission. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When a function parameter mixes legacy [local_] with new [@ mode] annotations, e.g., [(local_ x @ mode)], the previous code would output [((x @ mode) @ local)] which is not valid syntax (a single mode group can't be re-extended with another [@]). Fold the [local] mode into the existing modes list when the pattern is already a [Ppat_constraint], so the printer emits [(x @ mode local)] through the existing mode-list printing path. For patterns without an existing constraint we keep the simpler prefix-style code, which avoids changes to comment placement (which would otherwise increase the iteration count needed to reach a stable formatting). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop the comment-relocation step (a pre-existing workaround for the old legacy-printing path that had no Cmts.fmt call for the global_ attr's loc) and instead thread the attr's loc directly into the synthetic [Modality "global"]. [fmt_modal]'s [Cmts.fmt c loc] then picks up any comments naturally. No test refs change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Consistent with fmt_label_declaration: when converting a legacy [global_]-attributed constructor argument to a [Modality "global"], pass the original attribute's loc through to the synthetic modality rather than hard-coding [Location.none]. The parser-extended creates the [global_] attribute with [~loc:Location.none], so the rendered output is identical -- this is purely a consistency cleanup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
These exercise [local_ (e : t)], [local_ (e : t @ m)], and [local_ (e : @ m)]. Current output emits nested constraints (e.g., [((x : int) : @ local)]) -- since the legacy syntax will be removed soon, the slightly redundant nesting is acceptable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…format After rebasing onto oxcaml/jane (which upstreamed the [local_ T] -> [T @ local] arrow conversion in commit d9c82d3), the [localI] block in [fmt_arrow_param] is no longer needed: the parser-extended doesn't produce the legacy attribute on arrow params anymore. Also reformat as suggested by [dune build @fmt]. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
riaqn
force-pushed
the
convert-local_-to-new-syntax
branch
from
105ccab to
06dbc5b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Format Jane Street legacy
local_/global_annotations using the new mode / modality syntax. Conversions:local_ exp→(exp : @ local)(local_ pat),~(local_ pat),?(local_ pat = e)etc. →(pat @ local)(6 sites infmt_fun_args)let local_ x = e→let x @ local = e{ global_ c : t }→{ c : t @@ global }(record fields)C of global_ T→C of T @@ global(constructor args)Arrow types (
local_ T -> U→T @ local -> U) are handled by #142 (now inoxcaml/jane), so this PR's scope is just expressions, patterns, let-bindings, record fields, and constructor arguments.The standard parser canonicalizes the legacy and new forms to the same AST in most places (via
with_optional_mode_expretc.), so the round-trip AST equality check passes naturally for the conversions above. One case needed extra help: the formatter's pre-existingtype_constr_and_bodytransformation hoists modes from a function body(y : @ mode)into the function'sret_mode_annotations. The std parser distinguishes these two shapes and existing normalization only handled thePexp_constraint(_, Some ty, [])case; this PR adds the symmetric rule forPexp_constraint(_, None, modes).The
lb_localfield onSugar.Let_binding.tis removed;Sugar.of_let_bindingnow foldspvb_local=trueintopvb_modesso the regular new-syntax printing path takes over.Mixed legacy + new syntax (e.g.
(local_ x @ mode)) merges thelocalmode into the existing modes list, so the output is a single@ ...group(x @ local mode)instead of an invalid((x @ mode) @ local)re-extension.Other legacy AST shapes (
Pparam_val (true, ...)for plainPpat_var,global_attribute on record/constructor fields,Pexp_apply (extension_local, ...)) remain in the AST as the parser produced them; the formatter simply prints them in the new syntax.Tests in
local.mlexercise the six mixed-syntax pattern shapes and the threelocal_ (e : ...)expression shapes.