Skip to content

Parse the verbose end that closes a member block - #254

Open
pcshrosbree wants to merge 1 commit into
ionide:mainfrom
pcshrosbree:fix/type-extension-end
Open

pcshrosbree wants to merge 1 commit into
ionide:mainfrom
pcshrosbree:fix/type-extension-end

Conversation

@pcshrosbree

Copy link
Copy Markdown

The verbose-syntax end that closes a member block is not in the grammar, so it parses as a stray identifier:

type C
  with
     member x.M() = 1
  end
let z = 2

On main the tree is type_definition, then (long_identifier_or_op (identifier)) for end, then let z. There is no ERROR node, so the mis-parse goes unnoticed. It gets worse when the next element is an expression: inside a module, end followed by do f x becomes an application of end to the do block.

FSC accepts an explicit end in these places (pars.fsy):

  • after a type augmentation's members: tyconDefnAugmentation: WITH classDefnBlock declEnd;
  • after the members of a record, union or exception: opt_classDefn: WITH classDefnBlock declEnd;
  • after an interface implementation's members: opt_interfaceImplDefn: WITH objectImplementationBlock declEnd;
  • after an object expression's first member block: objExprBindings: … | OWITH localBindings OEND. LexFilter turns a real end that balances the with into OEND.

declEnd is ODECLEND | OEND | END.

Change

fsharp/grammar.js: an optional "end" after

  • the with block of _type_extension_with (augmentations and exceptions) and of type_extension_elements (members after a record or union);
  • interface_implementation's members;
  • the first member block of an object expression (_object_expression_inner).

common/scanner.h: an offside rule for end. In a verbose class

type T () =
  class
    interface I with
      member x.M i = i
  end

the interface's optional end becomes valid as soon as its members close. Without the rule, the scanner emitted end for the interface, and the class reported a missing end. The scanner now prefers DEDENT when both are valid and the end is left of the innermost open scope (here the class body, at the column of interface). This mirrors FSC's offside rule: that scope closes first, and the end goes to the class. An end at the interface's own column, or further right, still closes the interface. There are two exceptions:

  • Paren and brace scopes. A closing bracket ends them, not indentation, so in { new I with … interface J with … end } the end may sit left of the brace scope's column. This matches the else/elif branches, which already leave paren scopes alone.
  • Error recovery (valid_symbols[ERROR_SENTINEL]), where every symbol is valid and the grammar position is unknown. This follows the other recent token sites in the scanner.

Recording the keyword's column. F# measures a member block's end against the type keyword of its definition group (also for an and definition: FSC's layout filter opens the group's context at type, and and only continues it) or against exception. But this grammar opens a record's scope at a { on the = line, and the indented and alternative's scope at the type name. So the rule above closes such a scope before an end that the member block inside it should take:

type t1 = { t2: t2 }
   with
     member t1.M1() = 1
   end
and t2 = { t1: t1 }
   with
     member t2.M1() = 2
   end

The scanner learns the keyword's column from three new zero-width external tokens, _after_type_keyword, _after_and_keyword and _after_exception_keyword. The grammar places them right after their keyword. When the scanner is asked for one, nothing has been skipped since the keyword, so the keyword ends at the lexer's column. The scanner records that column in its serialized state (two bytes). An and keeps the group's type column; it records its own only when none is known, at the start of a parse or after error recovery skipped the mark.

The column is never cleared. Type definitions do not nest, and every definition group records its type keyword before its bodies, so wherever _end_dedent is valid the latest keyword is that definition's. After the definition ends, a stale column can only make an end left of it close the innermost scope first (the offside rule above), and the constructs that take an end after their scope closes still take it after that DEDENT. The column is used in two places:

  • _end_dedent replaces the plain DEDENT in one case. The grammar accepts an end right after the record, union or and scope only after _end_dedent. The scanner emits it only when all of these hold:

    • the rule above closed the scope;
    • the member block inside could have taken the end (END was valid);
    • the end is at or right of the keyword.

    Further left, the end belongs to an enclosing construct. Where no member block wants it, a plain DEDENT keeps it from being taken. So in module M = begin type R = { A: int } end, the end still closes the module.

  • The offside rule above also treats an end left of the keyword as offside, whatever the innermost scope (paren and brace scopes aside, as above). That matters when that scope opened left of the keyword, as when attributes come before the keyword on a begin line: in module M = begin [<A>] type R with member x.M = 1, the module's body opens at [<A>], but F# still measures the end against type. An end between the two columns closes the module, not the augmentation. (Where the scope is at or right of the keyword, such an end is left of the scope anyway; restricting the rule to scopes opened mid-line fails two of the tests.)

Columns in F#'s units. F# counts columns in UTF-16 units; tree-sitter's get_column counts code points. A character outside the Basic Multilingual Plane (an emoji, in an attribute's string, a comment or a ``quoted`` name) is one column for tree-sitter and two for F#. Before the keyword on its line, it would put the recorded column one left of F#'s, and an end at F#'s keyword column minus one would then be taken by the definition instead of the enclosing begin … end module.

The scanner cannot look back at the characters before the keyword, and its state survives only through the tokens it emits. So it keeps a record of the line it is on. After a token whose scan crossed a newline (and after the first token of a parse), the token's end is already marked, so the scanner can read ahead to the end of the line it is now on. It records the columns of such characters (up to eight), the line's length and its last sixteen characters. (A nested comment counts as crossing a newline only if it contains one.) At a keyword mark it reads the rest of the keyword's line and checks that the record is of that line: the same length, the same last characters, and the same such characters right of the keyword. Then it adds one column for each recorded character left of the keyword. The state holds the full record only when it has such a character; otherwise the record adds one byte.

The end is not corrected: it is compared in code points, as the scopes are. A record cannot be trusted for the end's line, because a string can carry the scan onto that line without the scanner seeing the newline. Correcting the end from such a record, or against a keyword column that falls short, broke more inputs than it fixed. So a character outside the BMP left of an end on its own line makes the end one column further left for the scanner than for F# (see "Not fixed here").

The record can hold fewer such characters than the line has left of the keyword:

  • where a comment or string carried the scan onto the keyword's line, the record starts where it ended, so a character inside it is not counted;
  • past the eighth, characters are not counted.

The correction then falls short of F#'s column, but it never passes it. Where no scan of the scanner's crossed into the keyword's line before the keyword, the record is of an earlier line. That happens in a multi-line attribute's argument list, and on a line that a one-quote string ("…", @"…", $"…") carried the scan onto (the grammar's lexer reads those strings, not the scanner). The checks reject such a record and the column is not corrected, unless that line has the same length, the same last sixteen characters, and the same such characters from the keyword's column on. The review inputs reach the checks with an earlier line's record only where they were built to, and each check has a test.

On every review input without such a character (110,292, the fuzz set included), the tree is identical to a build without the correction. Parse time on the example corpus (.fs) is 0.7% higher than without the correction, by the median of three alternating runs (10,453 → 10,528 ms over 5,317 files). That run was on a shared host, and its ranges overlap. On an otherwise idle host, the previous revision of this code measured 1.8%, with ranges that did not overlap; a reviewer measured 3% to 7% on a busier host. On one-line lists of 1,000 to 16,000 elements (eleven alternating runs per size), the median is 0.4% to 3.3% higher, and at every size the two builds' run ranges overlap.

Earlier versions inferred the keyword's level from the indent stack: first from scopes that start a line, then from scopes the grammar marked as type bodies. Reviewers found inputs that defeated each inference, attributes before type being the last. Recording the column removes the inference.

I also corrected the comment on _paren_indent in the externals list. It said the token pushes 0; it pushes the column of the first token after the bracket, which is what the paren exemption above relies on.

Tests

There are 62 new tests: 60 in test/corpus/type_defn.txt and 2 in test/corpus/expr.txt. No existing test changes, and FCS (Fantomas.FCS 7.0.5) parses every input with no diagnostics. For the tests about which construct owns an end, I checked the owner against FCS's AST.

34 fail on main.

29 have a stray end identifier and no ERROR or MISSING node:

  • an augmentation, and a one-line augmentation;
  • an interface implementation, inside an augmentation and in a class;
  • record members, and exception members;
  • record members with end left of the record body, and union members with end left of the cases;
  • mutually recursive augmentations;
  • a one-line record whose member block and module are both closed by end;
  • record members after and, with end left of the record body, at the and column, and with and on its own line;
  • record members with end exactly at the type column;
  • an attributed record's members closed by end right of type;
  • an attributed augmentation's and an attributed exception's members closed by end exactly at the keyword's column;
  • an and definition's members closed by end at the group's type column, right of which the and sits;
  • the same augmentation with an é in the attribute: a character in the Basic Multilingual Plane is one column for F# too, so the end is still at the keyword's column;
  • the same augmentation with an emoji right of the keyword, with emoji on both sides of it, and after a comment line of the same length with an emoji: the end at the keyword's column is the augmentation's;
  • five with the keyword on a line entered inside a multi-line attribute's argument list, after a line with an emoji. Each previous line matches the keyword's line in all but one way: its length, its last sixteen characters (two tests: in one only the sixteenth character from the end differs, in the other the last four are the same), the emoji right of the keyword, or the keyword line's own emoji. The record of the previous line must not be used for the keyword's line, and each test fails without the one check that tells the lines apart;
  • two where the keyword's line follows the first line of a multi-line string, or an attribute line ending in ; and a comment, with the same length and last sixteen characters: the scanner must take a new record at the newline in the string, and at the one after the ;.

Two have a MISSING node:

  • an augmentation in a module, where the stray end swallows the next declaration;
  • object expression member blocks over several lines.

Three others have an ERROR, all object expressions:

  • one member block closed by end;
  • an interface closed by end left of the brace scope;
  • one closed by end after a type definition in a deeper nested module, whose column the scanner still holds: the brace scope's exemption keeps that column from closing the object expression.

28 pass on main. They pin what must not change:

  • an end left of an interface implementation, which closes the class, and the same with the end exactly one column left;
  • two begin … end cases, where the end sits left of a then block or at the if column;
  • an end that closes an enclosing verbose module:
    • after a one-line record, or after an and type with a class body;
    • left of the type keyword;
    • left of a record or a union opened on the begin line;
    • after a record in nested modules, where it closes the nearest one.
  • an attribute before the keyword, where the end left of the keyword is the module's:
    • for a type or an exception after begin on the same line;
    • for a type, an augmentation or an exception after the attribute at the start of a line;
    • for a type under an attribute line;
    • for an attributed type after a let.
  • a later begin … end, a class's end, and a nested module's end in a module whose begin line held an attributed type;
  • an end one column left of the keyword, by F#'s count, after a character outside the BMP: it closes the module, and without the column correction it closes the definition. These are:
    • a record and an augmentation, after an emoji in the attribute;
    • an augmentation after two;
    • an augmentation with eight more right of the keyword, nine on the line;
    • an emoji on a line that a comment carried the scan onto, where the comment opens on the begin line, on a line of its own, or is nested;
    • an emoji before a one-line nested comment on the keyword's line, which must not count as crossing a newline.

Mutants of the scanner, against the corpus suite:

Mutant Result
offside rule removed killed
off by one (<=) killed
old column (no - 1) killed
no valid_symbols[DEDENT] term killed
no brace-scope exemption killed
_end_dedent never emitted killed
_end_dedent without the END-valid condition killed
_end_dedent without the keyword check killed
keyword check off by one (>) killed
type measured as 3 characters killed
and measured as 4 characters survives
exception measured as 8 characters killed
keyword column never recorded killed
no rule for an end left of the keyword killed
that rule only for scopes opened mid-line killed
that rule off by one (<=) killed
no paren-scope exemption survives
no error-recovery guard survives
_end_dedent allowed with no recorded column survives
column correction not applied killed
every non-ASCII character counted as two columns killed
characters right of the keyword counted too killed
line record taken after every token, or only once killed (two mutants)
newline not marked: in a comment, a nested comment, a triple-quoted string, a line's indentation, after a ; separator killed (five mutants)
a nested comment's newline not reported to the comment around it, or a nested comment marked without one killed (two mutants)
newline not marked: after a bare type declaration survives
the first such character past the eighth not noted killed
one such character kept instead of eight killed
four last characters kept instead of sixteen, or the last ones not kept in order killed (two mutants)
the line check without its length, last-characters, pair-list or extra-character part killed (four mutants)
line check compares characters past the eighth killed
record not saved in the state, or a record with such characters not restored killed (two mutants)
a record without such characters not restored survives
and records its own column killed
and records nothing, even with no column known survives
the keyword rule without the paren and brace exemption killed
  • The paren exemption. A reviewer searched 424 probes and the whole example corpus, and I tried eight inputs of my own. Nothing differs with or without the exemption. Member and type declarations don't occur directly inside a paren expression, and object expressions have a brace scope on top. I kept it for consistency with else/elif, and because a paren scope is closed by its bracket. It is untested.
  • The recovery guard. With ERROR_SENTINEL not valid the condition is unchanged, so it cannot alter a decision outside recovery. Without it, one corpus file recovers worse: E_orderingOfAccessibilityKeyword_member01.fs goes from 1 error node to 2. No .fsx file changes.
  • No recorded column. The column is recorded right after every type, and and exception keyword, before any _end_dedent can be valid. It is unknown only at the start of a parse, or after error recovery skipped the mark.
  • and with no column known, and and's length. An and always follows its group's type, which records the column first; it is unknown there only at the start of a parse or after error recovery skipped the mark. and's own length is used only then.
  • A newline mark that a later scan repeats. After a bare type declaration's newline (TYPE_DECL_NEWLINE peeks past it), the next scan crosses the same newline again and records the line. Without the mark, none of the 112,544 review inputs parses differently.
  • A record without such characters, not restored. The next token then records its line again, from where that token starts. That record is of the same line, or of a later one the old record did not describe, so the correction can only get closer to F#'s column. No FCS-correct test can tell the difference, and none of the 112,544 review inputs parses differently. The record is restored so that a line without such characters is read once, not after every token.

Corpus

check:baseline (.fs): 4458 → 4417. Five files parse better because of this change:

  • members/basics/test.fs 44 → 21;
  • members_basics.fs 45 → 22;
  • neg09.fs 3 → 0;
  • neg10.fs 3 → 2;
  • E_InterfaceNotFullyImpl02.fs 1 → 0.

Two other .fs files change because of error-recovery noise from the regenerated parse tables, not because of this change: CheckExpressions.fs 128 → 154 and prim-types.fs 228 → 212. So do three .fsx files: quotes/test.fsx 396 → 246, tools/eval/test.fsx 181 → 67 and helloWorld/provider.fsx 44 → 43. An unrelated grammar change (a separate PR for lazy and assert blocks) produces exactly the same counts on these five files. CheckExpressions.fs is recorded at its new count. The baseline is updated.

check:invalid: all 346 recorded errors are still found.

check-parse-baseline.fsx sweeps only *.fs, so I compared the other example files separately, counting the way the script does:

  • .fsx (770 files; the script's own directory exclusions would leave out two under testenv/bin, which hold one error on every build): 2090 → 1813, none worse. Four files improve because of this change: members/basics-hw 99 → 90, members/incremental 5 → 4, subtype 2 → 1 and syntax 17 → 16. The rest is the same noise as above.
  • .fsi (348 files, each parsed with the signature grammar): 9830 on both, none changed.

The script's count is not an exact count of ERROR and MISSING nodes. Counted exactly, by query, two more FCS-valid .fs files gain MISSING nodes: Array4Module.fs gains two, which the unrelated grammar change above reproduces (table noise), and neg12.fs gains one. On main, neg12.fs already misparses an object expression and leaves its end as an identifier; here that end is taken, and error recovery adds a MISSING end further on. Exact totals, main → this branch: .fs 5,560 ERROR + 409 MISSING → 5,520 + 404; .fsx 2,073 + 124 → 1,796 + 125; .fsi unchanged.

parser.c grows 4.2% (fsharp) and 5.3% (fsharp_signature), almost all of it from the new end alternatives; the three keyword tokens add about 26 KB to each. test/parser-size.txt is updated.

I also parsed the inputs written during review of this change, on main and on this branch: 21,507 distinct inputs from every round except one large fuzz set, which that round's reviewer checked against FCS separately. Signature files were checked in signature mode. FCS accepts 14,163 of them without diagnostics. Counting a parse as clean when it has no ERROR or MISSING node and no end left as an identifier:

  • 6,372 are clean only with this change;

  • 407 are clean only on main. 406 have characters outside the BMP that the column correction does not count ("Not fixed here" below):

    • 120 left of an end on its line;
    • 116 inside a comment or string that ends on the keyword's line;
    • 99 with more than eight before the keyword;
    • 51 on the first line of a file;
    • 20 on a keyword line that a one-quote string or a multi-line attribute carried the scan onto.

    The other one has a member block indented past column 256. Reviewers wrote most of these inputs to find the limits of the correction.

Against a build without the column correction, the correction fixes 329 of these inputs and breaks 96: 51 with such a character left of an end on its line, and 45 built so that an earlier line passes the record's checks for the keyword's line (both under "Not fixed here").

Not fixed here

  • An end inside an #if block of a class, closing an interface implementation only in that configuration:
    type C() =
      interface I with
        member x.M() = 1
    #if FOO
      end
    #endif
    The scanner's indentation walk steps over the #if line, the end closes the interface, and #endif is then left without its #if (one ERROR). On main the end was a stray identifier inside the #if. A grammar alternative doesn't reach it, because the directive is consumed before the grammar sees it.
  • A surplus end where no construct takes one is still a stray identifier, as on main.
  • A member block left of its definition's keyword, when the definition starts mid-line. In [<A>] type R = { X: int }, or in module M = begin type R = { X: int }, followed by with and end on their own lines left of type, FCS gives the end to the member block. Here, as on main, it is a stray identifier. The same holds for exception, at the top level and in a namespace. It also holds for an augmentation whose with is at the type column, which neither build parses as an augmentation.
  • A verbose module whose first definition is on the begin line closes its body at any later element left of that definition (and, let, exception), as on main. On main, a member block's end hid this by closing the module instead. Here the member block takes its end, and the module gets a MISSING end. This is 20 fuzzed review inputs, such as module M3 = begin type U4 = A | B / with / member x.M5 = 1 end / and R6 = A | B / end.
  • A declaration after a member block's end on the same line (… member x.M = 1 end let y = 1) now gets an ERROR. On main the tree was silently wrong. A module body needs a line break between its elements: module M = begin type A = int type B = int end fails on main too.
  • Signature augmentations with abstract-style members (member M: int after with in a .fsi) fail on main and here alike.
  • Characters outside the Basic Multilingual Plane that the line record does not count. The column correction above counts only the characters the record holds. Each one it misses puts the keyword one column further left of F#'s, and an end in that gap is taken by the definition; the module then gets a MISSING end, where main parsed the file clean. It misses:
    • characters on the first line of a file, where no token comes before the keyword (for example a module ``😀`` = begin … type … line);
    • characters inside a comment or string that carried the scan onto the keyword's line;
    • characters past the eighth before the keyword;
    • characters left of the end on its own line, which the end's column never counts;
    • all of them on a line that no scan of the scanner's crossed into before the keyword: one inside a multi-line attribute's argument list, or one that a one-quote string ("…", @"…", $"…") carried the scan onto.
  • A record of an earlier line that passes the checks. Where no scan of the scanner's crossed into the keyword's line, an earlier line with the same length, the same last sixteen characters and the same such characters from the keyword's column on is taken for the keyword's line, and its characters left of the keyword are counted. Review inputs reach this only when built for it, with padded comments that copy the keyword line's end.
  • Columns of 256 and more. The rule compares the end with the recorded scope column. On main the scanner saves those columns in one byte each, so a scope at column 256 or beyond is measured wrongly after a state save, and the rule can then misfire. A separate change widens that storage.

FSC accepts an explicit `end` after a type definition's `with` member
block (augmentations, exceptions, members after a record or union), after
an interface implementation's members, and after the first member block of
an object expression. The grammar had no slot for it, so `end` parsed as a
stray identifier with no ERROR node, and the following declaration was
often applied to it.

Add an optional `end` in those places. In the scanner, when both an `end`
and a DEDENT are valid and the `end` is left of the innermost open scope,
emit the DEDENT first, as F#'s offside rule does: in a verbose
`class ... end` holding an interface implementation, the `end` closes the
class, not the interface. Not for paren and brace scopes, which their
closing bracket ends, and not in error recovery.

F# measures a member block's `end` against the `type` keyword of its
definition group (an `and` only continues the group) or against
`exception`, which the scanner cannot see. Three zero-width externals
placed right after `type`, `and` and `exception` let it record the
group's `type` column, or the `exception` column, in its state (an `and`
records only when no column is known). With it:
- the offside rule above also closes a scope the grammar opens mid-line (a
  record's `{` on the `=` line, the type name after `and`) before an `end`
  the member block inside should take. For that case the scanner emits a
  distinct _end_dedent token, and the grammar accepts the `end` right after
  the record, union or `and` scope only after it. The scanner emits it only
  when the member block could have taken the `end` and the `end` is at or
  right of the keyword, so an enclosing module's `end` is never taken;
- an `end` left of the keyword is offside even when the innermost scope
  opened left of the keyword too, as after `[<A>]` before `type`.

F# counts columns in UTF-16 units and get_column in code points, so a
character outside the BMP before the keyword would put the recorded
column one left of F#'s. After a token whose scan crossed a newline,
the scanner records where such characters are on the line it is now
on (state is only kept through emitted tokens). At the keyword it
checks that the record is of the keyword's line (its length, its last
sixteen characters and such characters right of the keyword) and adds
one column per recorded character left of it. A record that starts after a
comment, or holds only the first eight, falls short of F#'s column; it
never passes it. The `end` stays in code points: a string can carry the
scan onto its line unseen, so no record can be trusted for it.

Five .fs and four .fsx corpus files parse better because of the change.
CheckExpressions.fs gains error nodes from parse-table noise that an
unrelated grammar change reproduces exactly; no .fsi file changes.
@pcshrosbree

Copy link
Copy Markdown
Author

This PR is one of a set; #255 maps them all, with their dependencies and merge order.

This branch has not been deployed

No deployments
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