-
Notifications
You must be signed in to change notification settings - Fork 471
Allow skipping leading pipe in definitions of variants with attribute on leading constructor #7782
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
Conversation
… on first constructor
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
LGTM! This is even better than just fixing the error message. |
match state.Parser.token with | ||
| Dot -> false | ||
| _ -> true) | ||
| DotDotDot | Bar | DocComment _ -> true |
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.
Should this be DotDot
as above or DotDotDot
as here?
Some low priority observation. We don't really have use cases so far, but the parser is more strict than needed. Short answer: besides the “leading-constructor” case that PR unblocks, the current logic still treats any Here are the concrete places you’ll want to allow an initial
What to change (surgically)
If you make those two tweaks, you’ll cover:
All of the above are blocked today because |
Nice! This definitely sounds worth fixing. I'll see if I can get to it soon. |
Haven't checked that this code change works, but seems at least directionally OK at a glance. Awesome — here’s a minimal, surgical patch to
diff --git a/compiler/syntax/src/res_core.ml b/compiler/syntax/src/res_core.ml
--- a/compiler/syntax/src/res_core.ml
+++ b/compiler/syntax/src/res_core.ml
@@ -5044,11 +5044,33 @@ and parse_type_representation ?current_type_name_path ?inline_types_context p =
in
let kind =
match p.Parser.token with
- | Bar | Uident _ | DocComment _ | At ->
- Parsetree.Ptype_variant (parse_type_constructor_declarations p)
+ | Bar | Uident _ | DocComment _ ->
+ Parsetree.Ptype_variant (parse_type_constructor_declarations p)
+ | At ->
+ (* Attributes can prefix either a variant (constructor list) or a record.
+ Peek past attributes (and any doc comments) to decide. *)
+ let is_record_after_attrs =
+ Parser.lookahead p (fun state ->
+ ignore (parse_attributes state);
+ let rec skip_docs () =
+ match state.Parser.token with
+ | DocComment _ -> Parser.next state; skip_docs ()
+ | _ -> ()
+ in
+ skip_docs ();
+ match state.Parser.token with
+ | Lbrace -> true
+ | _ -> false)
+ in
+ if is_record_after_attrs
+ then Parsetree.Ptype_record
+ else Parsetree.Ptype_variant (parse_type_constructor_declarations p)
| Lbrace ->
Parsetree.Ptype_record
| _ ->
(* unchanged ... *)
@@ -5602,6 +5624,38 @@ and parse_type_equation_and_representation ?current_type_name_path
| Bar | DotDot | DocComment _ ->
let priv, kind = parse_type_representation p in
(None, priv, kind)
- | At -> (
- (* Attribute can start a variant constructor or a type manifest.
- Look ahead past attributes; if a constructor-like token follows (Uident not immediately
- followed by a Dot, or DotDotDot/Bar/DocComment), treat as variant; otherwise manifest *)
- let is_variant_after_attrs =
- Parser.lookahead p (fun state ->
- ignore (parse_attributes state);
- match state.Parser.token with
- | Uident _ -> (
- Parser.next state;
- match state.Parser.token with
- | Dot -> false
- | _ -> true)
- | DotDotDot | Bar | DocComment _ -> true
- | _ -> false)
- in
- if is_variant_after_attrs then
- let priv, kind = parse_type_representation p in
- (None, priv, kind)
- else
- let manifest = Some (parse_typ_expr p) in
- match p.Parser.token with
- | Equal ->
- Parser.next p;
- let priv, kind =
- parse_type_representation ?current_type_name_path
- ?inline_types_context p
- in
- (manifest, priv, kind)
- | _ -> (manifest, Public, Parsetree.Ptype_abstract))
+ | At -> (
+ (* Attributes can start a representation (variant/record/open variant) or a manifest.
+ Look ahead past attributes (and doc comments). If what follows looks like a
+ representation, parse it; otherwise parse a manifest. *)
+ let is_representation_after_attrs =
+ Parser.lookahead p (fun state ->
+ ignore (parse_attributes state);
+ (* allow doc comments between attributes and the representation token *)
+ let rec skip_docs () =
+ match state.Parser.token with
+ | DocComment _ -> Parser.next state; skip_docs ()
+ | _ -> ()
+ in
+ skip_docs ();
+ match state.Parser.token with
+ | Lbrace -> true (* record after attributes *)
+ | Bar -> true (* variant constructor list *)
+ | DotDot -> true (* extensible variant ".." *)
+ | Uident _ -> (* constructor vs module-qualified manifest *)
+ Parser.next state;
+ (match state.Parser.token with
+ | Dot -> false (* M.t => manifest *)
+ | _ -> true) (* Uident starting a constructor *)
+ | DocComment _ -> true (* treated as representation; variant path handles it *)
+ | _ -> false)
+ in
+ if is_representation_after_attrs then
+ let priv, kind = parse_type_representation p in
+ (None, priv, kind)
+ else
+ let manifest = Some (parse_typ_expr p) in
+ match p.Parser.token with
+ | Equal ->
+ Parser.next p;
+ let priv, kind =
+ parse_type_representation ?current_type_name_path ?inline_types_context p
+ in
+ (manifest, priv, kind)
+ | _ -> (manifest, Public, Parsetree.Ptype_abstract))
| _ -> (
let manifest = Some (parse_typ_expr p) in
match p.Parser.token with Notes
This patch applies cleanly on top of the code added in PR #7782 (commit |
Fixes #7578 by simply allowing it instead.
@shulhi GPT5 figured most of this out, but to me it looks reasonable and good. WDYT?