Repository navigation
Parse the verbose end that closes a member block - #254
Open
pcshrosbree wants to merge 1 commit into
Open
pcshrosbree wants to merge 1 commit into
pcshrosbree wants to merge 1 commit into
Conversation
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.
Author
|
This PR is one of a set; #255 maps them all, with their dependencies and merge order. |
This branch has not been deployed
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
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.
The verbose-syntax
endthat closes a member block is not in the grammar, so it parses as a stray identifier:On
mainthe tree istype_definition, then(long_identifier_or_op (identifier))forend, thenlet 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,endfollowed bydo f xbecomes an application ofendto thedoblock.FSC accepts an explicit
endin these places (pars.fsy):tyconDefnAugmentation: WITH classDefnBlock declEnd;opt_classDefn: WITH classDefnBlock declEnd;opt_interfaceImplDefn: WITH objectImplementationBlock declEnd;objExprBindings: … | OWITH localBindings OEND. LexFilter turns a realendthat balances thewithintoOEND.declEndisODECLEND | OEND | END.Change
fsharp/grammar.js: an optional"end"afterwithblock of_type_extension_with(augmentations and exceptions) and oftype_extension_elements(members after a record or union);interface_implementation's members;_object_expression_inner).common/scanner.h: an offside rule forend. In a verbose classthe interface's optional
endbecomes valid as soon as its members close. Without the rule, the scanner emittedendfor the interface, and the class reported a missingend. The scanner now prefers DEDENT when both are valid and theendis left of the innermost open scope (here the class body, at the column ofinterface). This mirrors FSC's offside rule: that scope closes first, and theendgoes to the class. Anendat the interface's own column, or further right, still closes the interface. There are two exceptions:{ new I with … interface J with … end }theendmay sit left of the brace scope's column. This matches theelse/elifbranches, which already leave paren scopes alone.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
endagainst thetypekeyword of its definition group (also for ananddefinition: FSC's layout filter opens the group's context attype, andandonly continues it) or againstexception. But this grammar opens a record's scope at a{on the=line, and the indentedandalternative's scope at the type name. So the rule above closes such a scope before anendthat the member block inside it should take:The scanner learns the keyword's column from three new zero-width external tokens,
_after_type_keyword,_after_and_keywordand_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). Anandkeeps the group'stypecolumn; 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
typekeyword before its bodies, so wherever_end_dedentis valid the latest keyword is that definition's. After the definition ends, a stale column can only make anendleft of it close the innermost scope first (the offside rule above), and the constructs that take anendafter their scope closes still take it after that DEDENT. The column is used in two places:_end_dedentreplaces the plain DEDENT in one case. The grammar accepts anendright after the record, union orandscope only after_end_dedent. The scanner emits it only when all of these hold:end(END was valid);endis at or right of the keyword.Further left, the
endbelongs to an enclosing construct. Where no member block wants it, a plain DEDENT keeps it from being taken. So inmodule M = begin type R = { A: int } end, theendstill closes the module.The offside rule above also treats an
endleft 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 abeginline: inmodule M = begin [<A>] type R with member x.M = 1, the module's body opens at[<A>], but F# still measures theendagainsttype. Anendbetween the two columns closes the module, not the augmentation. (Where the scope is at or right of the keyword, such anendis 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_columncounts 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 anendat F#'s keyword column minus one would then be taken by the definition instead of the enclosingbegin … endmodule.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
endis not corrected: it is compared in code points, as the scopes are. A record cannot be trusted for theend's line, because a string can carry the scan onto that line without the scanner seeing the newline. Correcting theendfrom 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 anendon its own line makes theendone 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:
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
typebeing the last. Recording the column removes the inference.I also corrected the comment on
_paren_indentin 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.txtand 2 intest/corpus/expr.txt. No existing test changes, and FCS (Fantomas.FCS7.0.5) parses every input with no diagnostics. For the tests about which construct owns anend, I checked the owner against FCS's AST.34 fail on
main.29 have a stray
endidentifier and no ERROR or MISSING node:endleft of the record body, and union members withendleft of the cases;end;and, withendleft of the record body, at theandcolumn, and withandon its own line;endexactly at thetypecolumn;endright oftype;endexactly at the keyword's column;anddefinition's members closed byendat the group'stypecolumn, right of which theandsits;éin the attribute: a character in the Basic Multilingual Plane is one column for F# too, so theendis still at the keyword's column;endat the keyword's column is the augmentation's;;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:
endswallows the next declaration;Three others have an ERROR, all object expressions:
end;endleft of the brace scope;endafter 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:endleft of an interface implementation, which closes the class, and the same with theendexactly one column left;begin … endcases, where theendsits left of athenblock or at theifcolumn;endthat closes an enclosing verbose module:andtype with a class body;typekeyword;beginline;endleft of the keyword is the module's:beginon the same line;let.begin … end, a class'send, and a nested module'sendin a module whosebeginline held an attributed type;endone 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:beginline, on a line of its own, or is nested;Mutants of the scanner, against the corpus suite:
<=)- 1)valid_symbols[DEDENT]term_end_dedentnever emitted_end_dedentwithout the END-valid condition_end_dedentwithout the keyword check>)typemeasured as 3 charactersandmeasured as 4 charactersexceptionmeasured as 8 charactersendleft of the keyword<=)_end_dedentallowed with no recorded column;separatorandrecords its own columnandrecords nothing, even with no column knownelse/elif, and because a paren scope is closed by its bracket. It is untested.ERROR_SENTINELnot valid the condition is unchanged, so it cannot alter a decision outside recovery. Without it, one corpus file recovers worse:E_orderingOfAccessibilityKeyword_member01.fsgoes from 1 error node to 2. No.fsxfile changes.type,andandexceptionkeyword, before any_end_dedentcan be valid. It is unknown only at the start of a parse, or after error recovery skipped the mark.andwith no column known, andand's length. Anandalways follows its group'stype, 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.TYPE_DECL_NEWLINEpeeks 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.Corpus
check:baseline(.fs): 4458 → 4417. Five files parse better because of this change:members/basics/test.fs44 → 21;members_basics.fs45 → 22;neg09.fs3 → 0;neg10.fs3 → 2;E_InterfaceNotFullyImpl02.fs1 → 0.Two other
.fsfiles change because of error-recovery noise from the regenerated parse tables, not because of this change:CheckExpressions.fs128 → 154 andprim-types.fs228 → 212. So do three.fsxfiles:quotes/test.fsx396 → 246,tools/eval/test.fsx181 → 67 andhelloWorld/provider.fsx44 → 43. An unrelated grammar change (a separate PR forlazyandassertblocks) produces exactly the same counts on these five files.CheckExpressions.fsis recorded at its new count. The baseline is updated.check:invalid: all 346 recorded errors are still found.check-parse-baseline.fsxsweeps 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 undertestenv/bin, which hold one error on every build): 2090 → 1813, none worse. Four files improve because of this change:members/basics-hw99 → 90,members/incremental5 → 4,subtype2 → 1 andsyntax17 → 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
.fsfiles gain MISSING nodes:Array4Module.fsgains two, which the unrelated grammar change above reproduces (table noise), andneg12.fsgains one. Onmain,neg12.fsalready misparses an object expression and leaves itsendas an identifier; here thatendis taken, and error recovery adds a MISSINGendfurther on. Exact totals,main→ this branch:.fs5,560 ERROR + 409 MISSING → 5,520 + 404;.fsx2,073 + 124 → 1,796 + 125;.fsiunchanged.parser.cgrows 4.2% (fsharp) and 5.3% (fsharp_signature), almost all of it from the newendalternatives; the three keyword tokens add about 26 KB to each.test/parser-size.txtis updated.I also parsed the inputs written during review of this change, on
mainand 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 noendleft 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):endon its line;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
endon 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
endinside an#ifblock of a class, closing an interface implementation only in that configuration:#ifline, theendcloses the interface, and#endifis then left without its#if(one ERROR). Onmaintheendwas a stray identifier inside the#if. A grammar alternative doesn't reach it, because the directive is consumed before the grammar sees it.endwhere no construct takes one is still a stray identifier, as onmain.[<A>] type R = { X: int }, or inmodule M = begin type R = { X: int }, followed bywithandendon their own lines left oftype, FCS gives theendto the member block. Here, as onmain, it is a stray identifier. The same holds forexception, at the top level and in a namespace. It also holds for an augmentation whosewithis at thetypecolumn, which neither build parses as an augmentation.beginline closes its body at any later element left of that definition (and,let,exception), as onmain. Onmain, a member block'sendhid this by closing the module instead. Here the member block takes itsend, and the module gets a MISSINGend. This is 20 fuzzed review inputs, such asmodule M3 = begin type U4 = A | B/with/member x.M5 = 1 end/and R6 = A | B/end.endon the same line (… member x.M = 1 end let y = 1) now gets an ERROR. Onmainthe tree was silently wrong. A module body needs a line break between its elements:module M = begin type A = int type B = int endfails onmaintoo.member M: intafterwithin a.fsi) fail onmainand here alike.endin that gap is taken by the definition; the module then gets a MISSINGend, wheremainparsed the file clean. It misses:module ``😀`` = begin … type …line);endon its own line, which theend's column never counts;"…",@"…",$"…") carried the scan onto.endwith the recorded scope column. Onmainthe 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.