Make @as part of the AST - #8619
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4623e05b33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (* [@as] is a field of [label_declaration] in the current parsetree and an | ||
| attribute in this frozen one, so it is put back on the way out and read off | ||
| again on the way in, the same as [pld_optional]. *) | ||
| let add_as_attr ~runtime_name attrs = |
There was a problem hiding this comment.
Adding add_as_attr modifies the frozen compatibility module even though this conversion is specific to ast_mapper_to0.ml. Since parsetree0.ml is the stable v0 boundary used by existing PPXs, keep it unchanged and define the helper in the mapping module instead.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed, and one step further than requested: rather than relocating the helper, it's gone entirely.
label_declaration now reuses Ast_helper.Type.field_attributes, exactly as constructor_declaration already reused constructor_attributes. So parsetree0.ml is back to a zero diff against master, and the whole change to ast_mapper_to0.ml is three lines with no payload construction of its own.
Fixed in a03315d.
| }; | ||
| ] | ||
| in | ||
| (Location.mkloc "as" loc, payload) :: attrs |
There was a problem hiding this comment.
For a field written as @dead("x") @as("wire") name, extraction leaves @dead in pld_attributes, but this cons operation sends the external PPX [@as; @dead] rather than the original order. The later location-based reconstruction hides the change from round-trip snapshots, while order-sensitive PPXs still observe it; insert the synthesized attribute according to its saved location, as field_attributes already does.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the change above resolves this one too: field_attributes inserts the synthesized attribute at its saved location, so a field written @dead("x") @as("wire") a now reaches an external ppx as [@dead; @as], in the order written.
Added field_runtime_name_keeps_its_place_on_the_wire to tests/ounit_tests/ounit_ast_mapper0_tests.ml, per the AGENTS.md guidance that v0 wire shape belongs there. I checked it earns its place: reverting the insertion to a cons-at-front makes it fail.
|
|
||
| and label_declaration = { | ||
| pld_name: string loc; | ||
| pld_runtime_name: String_literal.string_literal loc option; |
There was a problem hiding this comment.
For every valid record-field @as, Type.field removes the attribute and stores it only in pld_runtime_name, but Printast.label_decl and Sexp_ast.label_declaration still emit only pld_attributes. Consequently -dparsetree silently drops the rename and makes renamed and unrenamed record fields appear identical; reconstruct the attribute with Ast_helper.Type.field_attributes or print this field explicitly, as the constructor dump now does.
AGENTS.md reference: AGENTS.md:L160-L164
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed for Printast.label_decl, which now prints Ast_helper.Type.field_attributes ld — the same reconstruction the constructor dump already used. -dparsetree on @dead("x") @as("wire") a shows both attributes again, in source order.
One note on the other half: there is no Sexp_ast module in this repository, so that part doesn't apply here. Printast was the only dumper reading pld_attributes directly.
4623e05 to
553cd7b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lambda/split-traversals #8619 +/- ##
===========================================================
+ Coverage 77.35% 77.41% +0.05%
===========================================================
Files 471 471
Lines 63476 63549 +73
===========================================================
+ Hits 49102 49194 +92
+ Misses 14374 14355 -19
🚀 New features to boost your workflow:
|
553cd7b to
a03315d
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
a03315d to
498a0e7
Compare
|
Astra found that converting object types containing spreads could drop a record field's Fixed in commit 498a0e7. The two object-spread conversions in |
A record field is named by its [@as("...")] attribute when it has one and by its declared name otherwise. That rule was written out nine times inside lambda.ml, once more in typecore_record_rest, and reached record_attributes_check through Lambda.find_name - so the IR module was parsing attribute payloads on behalf of two other layers. Record_runtime holds it now, next to Variant_runtime, which already owns how a variant is represented at run time. The nine sites in lambda.ml ask for the name instead of deriving it, and the other two callers share the same definition. lambda.ml no longer mentions Parsetree or Ast_payload. It still takes Types.label_description, since the tag_info builders are given labels; passing them decoded names instead would move the decoding to translcore, which is a separate question. Generated JavaScript is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
Lambda's nine block and field builders each took a Types.label_description and worked out the field's runtime name themselves. The name is now decided once, when the label description is built, and the builders take it as a plain string: -val fld_record : Types.label_description -> field_dbg_info +val fld_record : string -> field_dbg_info so Lambda no longer reaches into Types or Typedtree to find out what a field is called. Note that typecore's dictionary path copies a label description with a replaced name, so it has to replace the runtime name with it; otherwise every dictionary field reads the name of the original. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
@as("x") renames a record field at run time. It was an attribute that seven separate places re-read and re-interpreted: record construction and access, record spread, record coercion, signature inclusion, duplicate-field detection, the @tag conflict check, @deriving(abstract) and gentype. The declaration now records the runtime name, and they all read the same field. Extraction happens in Ast_helper.Type.field, so the parser, every ppx and the frozen-AST bridge inherit it: ast_mapper_from0 needs no change at all, and ast_mapper_to0 puts the attribute back the way it already does for pld_optional. This removes the two hooks installed into Builtin_attributes to reach an attribute reader from compiler/core, and the module that filled them. @as keeps its surface syntax. The parsetree holds the literal with its source spelling and its own location, so printers put it back exactly where it was written; the typed tree holds only the decoded name, which is what Asttypes.constant already does for every other string literal. A payload that does not denote a name is left alone, and a second @as is left for the type checker rather than rejected at parse time, so an invalid file can still be formatted. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
Following the same move for record fields, a constructor's @as runtime tag is now recorded on the declaration instead of being re-interpreted from its attributes. Extraction happens in Ast_helper.Type.constructor, so the parser, every ppx and the frozen-AST bridge inherit it, and ast_mapper_to0 puts the attribute back the way it does for records. The parsetree keeps the payload's source spelling, so @as(0xA) and @as("A") print back as written rather than as 10 and "A". The typed declaration keeps only the decoded tag. This deletes process_tag_type, which decoded the same attribute a second time, and with it four payload readers that had no other caller. has_undefined_literal was dead. constructor_tag and block_runtime took an attribute list that every caller passed empty, since they build tags for constructors the compiler generates itself, so they now say that. An out-of-range integer in @as used to abort the compiler with Failure("int_of_string"); it now reports its own error at the annotation. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
The seven cases a constructor's @as can state existed twice: once as declared_tag, and again as seven of tag_type's eight constructors, joined by a conversion. Nothing ever matched on declared_tag for a decision; every use mapped it straight to tag_type. So tag_type now holds the narrower type rather than repeating it, and the conversion becomes the constructor: type tag_type = Literal of literal_tag | Untagged of block_type Renamed from declared_tag, because two sites build the literal for a constructor that declares nothing -- the default encoding is the constructor's own name -- and because the surrounding code already calls these literals: literal_tags, literal_cases, is_a_literal_case, check_literal. Functions that only ever handled literals now say so in their type: js_exp_make's tag_type splits into the JS value a literal stands for and the typeof string an untagged payload answers to, which were unrelated jobs sharing one match, and printtyp's @as reconstruction takes a literal_tag. Where a match arm per literal remains it unwraps Literal once instead of in every arm, and variant_coercion's four arms that differed only in the expected type become a table. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
A constructor's stored tag could only ever be a declared literal: the
untagged shape is synthesized during matching, at one place, from a
payload's block type. The record now says so, and the two roles get
separate names rather than one field standing for both:
type tag = {name: string; literal: literal_tag option}
type matchable_tag = {name: string; tag_type: tag_type option}
val to_matchable_tag : tag -> matchable_tag
so constructor_tag returns a literal_tag option, literal_tags and
literal_cases are literal lists, and the coercion errors carry literals.
Their readers stop unwrapping a case that could not occur -- printtyp's
@as reconstruction loses its "should never happen" arm, and the overlap
predicates in the untagged checks stop matching on Untagged to answer
questions only a literal can answer.
Three functions had no callers. tag_can_be_undefined guarded the
optimisation removed in 244cdad, which compiled {field: VariantCase}
to field: VariantCase and made a single-case variant look exhaustive.
get_tag_name was a one-line wrapper that 108aa8f inlined past.
tag_type_to_user_visible_string is dead as of this commit: its Literal
branch delegated to literal_tag_to_user_visible_string, which is what
the narrowed errors call, and its Untagged branch was unreachable.
Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
Three functions read the single @as an item may carry, differing only in the payload they accept: a string, an int, or any of int, string and json. Each repeated the same skeleton -- find the attribute, reject a second one, mark it used -- so that part is now written once and takes the decoder: let as_string (attrs : t) : string option = single_as attrs ~decode:(fun ~loc payload -> match Ast_payload.semantic_string_of_payload payload with | None -> Bs_syntaxerr.err loc Expect_string_literal | Some v -> v) The decoder runs before a second attribute is looked at, so a malformed payload is still reported ahead of the duplicate that follows it rather than the other way round. Renamed from iter_process_bs_string_as and its siblings, which described the traversal rather than the result and kept a bs prefix that no longer means anything. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
iter_process_bs_string_int_unwrap_uncurry advertised uncurry, which it does not handle, and omitted ignore, which it does. It answers which of @string, @int, @ignore and @unwrap an external's argument carries, so it is now arg_encoding. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
Every @as on a record field was marked used, which made sense when the attribute was what the compiler read. Now the one that names the field is taken out of the attributes when the field is built, so an @as still there is either a second one, which the type checker rejects, or a payload that is not a name at all. @as(42) on a record field was accepted in silence: it renamed nothing and nothing reported it. Only the attribute the compiler acts on is marked now, so that case warns and a duplicate still reports just its own error rather than both. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
498a0e7 to
326f17c
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 326f17c222
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | [] -> (None, []) | ||
| | (({txt = "as"; loc}, payload) : Parsetree.attribute) :: rest -> ( | ||
| match Ast_payload.constructor_tag_of_payload payload with | ||
| | Some txt -> (Some {Asttypes.txt; loc}, List.rev_append seen rest) |
There was a problem hiding this comment.
When completion analyzes a syntactically valid @as on a record field or variant constructor, these branches remove the attribute from pld_attributes/pcd_attributes, but Ast_iterator.default_iterator still traverses only those lists (compiler/ml/ast_iterator.ml lines 529-543). Consequently the attribute callback in analysis/src/completion_front_end.ml never sees the decorator, regressing decorator completion and cursor handling for these declarations; reconstruct the full attribute list in the iterator as the mapper and printers do.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
| let written_earlier (({loc = other}, _) : Parsetree.attribute) = | ||
| other.loc_start.pos_cnum < loc.loc_start.pos_cnum | ||
| in | ||
| let earlier, later = List.partition written_earlier ld.pld_attributes in | ||
| earlier @ ((Location.mkloc "as" loc, payload) :: later) |
There was a problem hiding this comment.
When a PPX produces [dead; as] with generated or Location.none locations, both attributes have the same pos_cnum, so this strict comparison classifies dead as later and reconstructs the list as [as; dead]. Thus a from0→to0 bridge changes the wire order observed by the next order-sensitive PPX; the new location-based reconstruction fixes distinct source locations but equal locations are fresh evidence that the ordering issue remains, so retain the original list position or otherwise preserve ties.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
2 participants
@asrenames a record field or a variant constructor at run time. It was anattribute that many parts of the compiler re-read and re-interpreted
independently: record construction and access, record spread and coercion,
signature inclusion, duplicate-field detection, the
@tagconflict check,@deriving(abstract)and gentype each derived the runtime name forthemselves. The declaration now records it once and they all read one field.
Extraction happens in
Ast_helper.Type.fieldandType.constructor, so theparser, every ppx and the frozen-AST bridge inherit it without changes of
their own.
@askeeps its surface syntax: the parsetree holds the literalwith its source spelling and its own location, so printers put the attribute
back exactly where it was written, and only the typed tree keeps the decoded
name. Two indirection hooks installed into
Builtin_attributesare gone withthe module that filled them.
Three user-visible consequences, each with a fixture:
@asthat exceeds the compiler's range usedto abort with
Failure("int_of_string"); it now reports an error.@as(42)on a record field renamed nothing and was accepted in silence; itnow warns as the unused attribute it is.
@aspayload written with backquotes formats with ordinary quotes, sinceit names the same thing either way.
Magic numbers are bumped once per stage, for the parsetree and for the cmi
and cmt.
Part of #8573. Stacked on #8618.