ArgParser: backtick a record field name when constructing the parsed record - #604
Merged
Merged
Conversation
…record The assemble callback in toParseSpec rebuilds each field's name as a fresh Ident (via SynLongIdent.create [ Ident.create ident ]) when it constructs the record-construction expression. Fantomas prints an Ident exactly as its idText reads, with no backticking of its own -- it can only reproduce backticks for a name by slicing the original source text at that node's range, and a freshly-constructed Ident has no such range. So a field declared with backticks because its name is not a plain identifier (a space, a keyword, ...) reached the generated file unbackticked and did not compile. Fixed by routing the reconstructed name through Fantomas.FCS.Syntax.PrettyNaming.NormalizeIdentifierBackticks, which already exists in the Fantomas.FCS dependency for exactly this purpose. Audited every other record-construction site in this file and in JsonParseGenerator, RemoveOptionsGenerator, and the two mock generators: all of them reuse the field's original Ident node (SynLongIdent.createI) rather than rebuilding one from a raw string, so none share this bug. Discriminated union case names are likewise reused directly and are unaffected. I did spot two similarly-shaped latent bugs (also reconstructing a name via string concatenation, in ArgParserGenerator's and RemoveOptionsGenerator's Default<FieldName> member-reference convention) -- filing those separately rather than folding them into this fix, since they're a different feature each and orthogonal to this record-construction bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codex review of the previous commit found that PrettyNaming.NormalizeIdentifierBackticks leaves an active-pattern-shaped name (|A|_|, |A|B|, ...) unbackticked, since that shape is a meaningful bare token in other grammar positions (an active-pattern reference). It is not valid as a bare record-construction label, though, so a field declared as `` ``|A|_|`` `` still reached the generated file unbackticked and failed to compile -- the same failure mode the previous commit fixed, just via a different gap in the same library function. Swept the rest of NormalizeIdentifierBackticks's behaviour against every F# keyword (including the "reserved for future use" ones, which it correctly leaves bare, since those really are legal identifiers today) plus a handful of other identifier-adjacent shapes, and found exactly one more gap: a bare `_` (the wildcard pattern) is also left unbackticked despite not being a valid record label. Verified both empirically against fsi before writing the fix. Added `backtickRecordLabel`, which defers to NormalizeIdentifierBackticks for everything it already gets right, and force-backticks only these two specific shapes. Extended AwkwardFieldName in ConsumePlugin/Args.fs with `` ``_`` `` and `` ``|A|_|`` `` fields and the corresponding test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Second Codex review round found more gaps in the previous commit's exception list: PrettyNaming.NormalizeIdentifierBackticks also leaves the word-form operator keywords (mod, land, lor, lxor, lsl, lsr, asr) and the context-sensitive dunder constants (__LINE__, __SOURCE_FILE__, __SOURCE_DIRECTORY__) unbackticked, for the same reason as the previous two shapes: each is a meaningful bare token in some other F# grammar position, so the general-purpose helper treats it as already fine, but none of them is valid as a bare record-construction label. Hand-enumerating F#'s lexer special cases is a losing game -- this is the second round of gaps found in exactly that approach, and there is no reason to believe it's now exhaustive. Replaced the whole exception list with a parse-oracle: splice the candidate bare into a minimal snippet using it as both a field declaration and a construction label (the two positions the real generated file needs it to work in), and ask Ast.parse -- the same parser this codebase already depends on for its own rejection tests -- whether that parses at all. Backticking is always a legal alternative spelling of any identifier, so falling back to it whenever the probe fails is unconditionally safe. Extended AwkwardFieldName with `mod` and `__LINE__` fields covering the two newly-found shapes, verified against a real dotnet fsi compile (not just the parser) that backticked mod/fixed/__LINE__ round-trip correctly before writing the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Third Codex round on the record-construction backtick fix found that the parse-oracle has a blind spot: words like `break`, `virtual`, `sealed` parse fine bare (so the oracle calls them safe) but the real compiler reserves them "for future use" and emits FS0046, a warning by default but an error under `--warnaserror`, which this repo enables. Fantomas's own parser doesn't model that distinction, so the oracle can't be asked about it. Unlike the previous rounds' gaps, this set is closed and documented, so it's handled with an explicit list rather than another guess -- verified by compiling each candidate from the F# keyword reference against the actual compiler, since the reference itself turned out to list three words (const, event, external) that no longer warn at all. The same review also found the oracle can be fooled by a field name that smuggles extra syntax into the probe (e.g. an embedded block comment), making it parse as a different, shorter label than intended. Left deliberately unfixed and documented in place: no real field name looks like that, and the failure mode is the same one already being fixed here -- a generated file that doesn't compile -- not silent corruption. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Smaug123
commented
Jul 30, 2026
| else | ||
|
|
||
| try | ||
| Ast.parse $"module M\ntype T = {{ %s{ident} : int }}\nlet _ = {{ %s{ident} = 1 }}" |
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.
Summary
toParseSpec's record-construction expression rebuilds each field name as a freshIdent(SynLongIdent.create [ Ident.create ident ]). Fantomas prints anIdentexactly as itsidTextreads and only reproduces backticks by slicing the original source text at that node's range; a freshly-synthesized node has no such range, so a field declared with backticks (a space, a keyword, ...) reached the generated file unbackticked and the generated file did not compile.Fantomas.FCS.Syntax.PrettyNaming.NormalizeIdentifierBackticks, which already exists in theFantomas.FCSdependency for exactly this.AwkwardFieldNametoConsumePlugin/Args.fs(modelled onAwkwardLongForms) plus a round-trip test asserting the parsed value.Audit
Checked every other record-construction site (
JsonParseGenerator,RemoveOptionsGenerator, both mock generators) and DU case-name construction inArgParserGenerator: all of those reuse the field/case's originalIdentnode (SynLongIdent.createI) rather than rebuilding one from a raw string, so they don't share this bug (confirmed by testing a backticked DU case name, which round-trips fine already).Found two similarly-shaped latent bugs elsewhere (the
Default<FieldName>member-reference convention in bothArgParserGeneratorandRemoveOptionsGeneratorreconstructs a name the same unsafe way) — filed as separate follow-up tasks rather than folded in here, since each is a different feature.Test plan
dotnet test— 3 / 124 / 1104 passeddotnet fantomas .clean