Fix: dangling else/else-if split from its if by a #if/#end boundary#85
Draft
mallyskies wants to merge 26 commits into
Draft
Fix: dangling else/else-if split from its if by a #if/#end boundary#85mallyskies wants to merge 26 commits into
mallyskies wants to merge 26 commits into
Conversation
…mprehensions Fixes github.com/vantreeseba/issues/60's underlying gap (a WIP PR attempting the same thing) with a from-scratch implementation against the Haxe language reference (haxe.org/manual/expression-*.html, block through inline), not a port of that PR's approach. `for`, `while`, `do`, `try`, and `catch` were bare placeholder keyword tokens (`keyword: choice('catch', 'do', 'for', 'try', 'while')`) with no real grammar rule at all. A statement like `for (i in 0...10) trace(i);` parsed with NO error node -- but only because `for`, `(i in 0...10)`, and `trace(i);` happened to each independently satisfy some other, unrelated statement/expression rule as three disconnected siblings, not because `for` was actually understood as a loop. Same story for `while`, `do ... while`, and `try ... catch`. This is a "wrong tree, not a broken one" bug -- has_error-based checks are blind to it, which is exactly why it went unnoticed this long. Implemented for real, following the manual directly: - `for_statement`: `for (binding in iterable) body`, where binding is either a plain identifier or a key => value pair (Haxe 4.0+, `for (k => v in map)`). - `while_statement` / `do_while_statement`. - `try_statement` with repeatable `catch_clause`s, each with an optional type annotation (Haxe 4.1+ wildcard catch: `catch (e) { ... }`). - `conditional_statement` (if/else if/else) rewritten so bodies are $.statement instead of $.block -- Haxe's `if (cond) expr;` (no braces) is standard, idiomatic syntax that was completely broken here, forcing braces on every branch. - Array comprehensions (`[for (i in 0...10) i * 2]`, `[while (...) ...]`, manual: lf-array-comprehension.html), replacing a placeholder rule that didn't match real comprehension syntax at all. Supports nesting (`for`/`while`/an `if`-filter inside a comprehension body), matching real code like `[for (a in 1...11) for (b in 2...4) if (a % b == 0) a + "/" + b]`. All four statement bodies reuse $.statement rather than a separate braces-only rule, which is what makes braceless bodies work "for free" without duplicating the block/single-statement logic per construct. **Found and fixed along the way, once real testing against a large depot surfaced them:** - The float literal regex allowed repeated dots (`[\.]+` instead of a single `\.`), so `0...10` (an integer immediately followed by the range operator -- the single most common shape for a for-loop's iterable) was being lexed as ONE malformed float token, "0...10", silently swallowing the range operator. Tree-sitter's regex engine has no lookahead, so there's no way to keep a bare trailing-dot float (`3.`) valid while still telling `0.` apart from `0...`; fixed by requiring at least one digit after the dot, which is a real but comparatively rare trade-off (a genuine standalone trailing-dot float in real, non-comment/string code) against a very common one (every `for (i in 0...N)` loop). - `range_expression` previously baked `identifier 'in' ...` directly into itself -- a workaround from when `for` was unimplemented, using it to soak up a for-loop's `(i in 0...10)` content via the same kind of accidental-sibling trick. Investigated replacing it with a clean, standalone `min...max` rule now that `for_statement` provides its own real binding structure, but found (via direct testing, not assumption) that a dedicated rule is actually unreachable dead code here: `...` is already one of the generic `$.operator` choices, so the existing plain operator-chain alternative in `expression` already parses `0...10`, `arr.length - 1 ... arr.length + 5`, etc. correctly on its own with no dedicated rule at all. Removed rather than left in as dead code. - `subscript_expression` (`arr[i]`) could appear as a chain's head (`arr[i] = x`, fixed in an earlier commit) but not its tail -- `while (null != m_cache[id]) { ... }` still failed. Fixed by including it in `_chain_term`. - A pre-existing, narrower ambiguity surfaced once control-flow bodies became $.statement instead of $.block: a genuinely EMPTY `{}` body resolves to an empty $.object (an object-literal expression statement) rather than $.block. Investigated at length -- declaring `[$.block, $.object]` in `conflicts` is flagged unnecessary by tree-sitter's own analysis (it doesn't see this as a real, GLR-forkable ambiguity), and neither `prec`/`prec.dynamic` on either rule (tried up to prec(1000)) nor an explicit `choice($.block, $.statement)` at the body field change the outcome, which suggests table construction is merging the two empty-content states before any declared precedence would be consulted -- not something fixable from grammar.js alone. Left as a documented, narrow limitation (42 files in this depot use a genuinely empty control-flow body) rather than blocking the other ~99% of this fix on it; non-empty bodies are entirely unaffected. Verified: full corpus suite, 190/190 (added dedicated corpus files for conditional_statement braceless bodies, loop statements, try/catch, and array comprehensions; updated two pre-existing tests to match corrected behavior -- the old array-comprehension placeholder test's expected output, and removed a test for the now-unsupported bare trailing-dot float). Depot-wide sweep of 5,332 real .hx files: 0 regressions, 251 files newly error-free (has_error-based, which undercounts real impact since many files have multiple independent issues) -- a much clearer picture from counting the actual new node types recognized: 8,601 for-loops (1,751 files), 1,206 while-loops (502 files), 182 do-while loops (106 files), 400 try-statements / 403 catch-clauses (222-223 files), 871 array comprehensions (353 files), and 53,288 braceless if-bodies across 3,241 files -- all previously either disconnected-sibling nonsense or a hard parse error.
…ted node The while-comprehension test's expected tree still described upstream's generic call_expression-shaped fallback (upstream has no dedicated comprehension_while rule). This fork adds one as part of this PR, so the expected tree should reflect it instead.
A parenthesized expression could previously only ever be an entire standalone `expression` (the top-level $._parenthesized_expression choice) -- never one term of a longer operator chain. So `0...(10)`, `a + (b)`, `(a + b) * c`, `total = (a + b) / 2`, etc. all hard-errored, even though grouping parentheses in ordinary arithmetic are about as common as it gets. Found via a depot-wide Haxe parse-error sweep: the minimal repro was `0...(width*height)` in haxe/src/com/masque/mah/common/ZMap.hx, but testing plain `a + (b)` confirmed the gap was general, not range-operator-specific. Two additions, mirroring the existing $.subscript_expression-as-head / $.subscript_expression-as-tail precedent already in this grammar: - $._chain_term (a chain's TAIL term) now includes $._parenthesized_expression alongside $._rhs_expression and $.subscript_expression. - `expression` gets a new repeat1-gated alternative, seq($._parenthesized_expression, repeat1(seq($.operator, $._chain_term))), for a parenthesized sub-expression as a chain's HEAD. repeat1-gated so a solo `(a + b)` alone still resolves via the existing standalone $._parenthesized_expression choice, not this one. Verified: 225/225 corpus tests pass (3 new, covering tail position, head position, and the original range-operator repro). Depot-wide sweep against the Masque depot, combined with the prior switch-case-without-block fix in this same session: of 548 files with a fully-destroyed class node, 342 recovered a proper class_declaration node (548 -> 206 remaining severe); 2,840 files still show has_error overall (down from 3,275), the remainder being further, separate issues -- confirmed one more while verifying this fix: `return <ternary>;` fails to parse regardless of parens, and a ternary used as a term inside a larger chain (e.g. string concatenation) has the same "not includable as a chain term" gap this fix addresses for parenthesized expressions specifically. Not fixed here -- flagged as a separate follow-up.
a parenthesized chain as a ternary condition Two related gaps in expression positions that hand-roll their own restricted shape list instead of delegating to the general chain grammar: 1. `return`/`untyped` only accepted $._rhs_expression (plus a couple of chain/subscript alternatives) as their value -- never a bare $.ternary_expression or a $._parenthesized_expression. So `return a ? b : c;`, `return (a ? b : c);`, and `return (a + b);` all hard-errored. Added both as bare choices, mirroring the existing bare $.subscript_expression choice already there. 2. $._ternary_condition's own "plain chain" alternative used $._rhs_expression for its repeated tail terms (not $._chain_term, which already gained parenthesized-term support in the prior commit), and had no head-position alternative for a parenthesized head term at all. So `(x >= 0) && (x < 10) ? a : b` hard-errored even though the unparenthesized `x >= 0 && x < 10 ? a : b` worked fine. Fixed both tail (switch to $._chain_term) and head (new repeat1-gated $._parenthesized_expression alternative, mirroring the same pattern already used in $._chain_term / `expression` for the general case). Found while verifying the prior commit's parenthesized-chain-term fix against the real file that originally surfaced it (haxe/src/com/masque/mah/common/ZMap.hx): its `return (ix!=-1) ? heights[ix] : -1;` (line 162) and `trace(... ((yTop + y >= 0) && (yTop + y < 10) ? " " : "") ...)` (line 210) were still erroring even after that fix landed. Verified: 229/229 corpus tests pass (5 new: bare/parenthesized ternary return, parenthesized non-ternary return, and a ternary condition built from parenthesized chain terms). ZMap.hx -- the file that surfaced bugs 3, 4, and 5 across this session -- now parses with ZERO errors. Depot-wide sweep: files with a fully-destroyed class node dropped from 206 (after the prior commit) to 169; cumulative from session start (548), a 69% reduction. Same graphify test suite baseline before and after (471 pre-existing failures, unrelated -- missing optional tree-sitter grammar packages in this environment).
case_statement required exactly one $.statement after the ':', so a case
clause with two or more statements and no explicit block -- the idiomatic
Haxe/AS3-style pattern this codebase uses constantly, e.g.:
switch (i) {
case 0: xT = x; yT = y;
case 1: xT = x+1; yT = y;
}
-- failed to parse. The second statement onward became an ERROR, and
tree-sitter's recovery flattened the ENTIRE enclosing class declaration
into loose top-level tokens rather than a nested class_declaration node,
destroying the class's own node, inherits/implements edges, and correct
method scoping for graphify's downstream extraction.
Changed both case_statement alternatives from a single $.statement to
repeat($.statement), matching the same repeat($.statement) idiom already
used by $.block for a brace-delimited statement sequence.
Verified: 222/222 corpus tests pass (1 new, covering multiple statements
in case/default clauses with no block). Depot-wide sweep against the
Masque depot: of 548 files with a fully-destroyed class node (has_error
true, zero properly-formed top-level declaration), 34 recovered a proper
class_declaration node after this fix; the remainder are unrelated
issues (found: a conditional type in a parameter position, an if/else
split by a preprocessor #if/#end, and a parenthesized operand in a range
expression -- all separate bugs, not fixed here).
function-type segment
Two related gaps in $.structure_type's reach, found via real depot
files during a graphify practice-session review:
1. typedef_declaration's RHS still listed $.block as a choice alongside
$.structure_type (a leftover from before $.structure_type existed as
its own rule) -- a bare 'identifier : expression' is also a valid
one-statement $.block via $.pair's low-precedence bare-literal
alternative, so a single-field, no-trailing-comma struct
('typedef X = { f:T }', real example: 'private typedef JJSON = {
settings:String }' in
haxe/src/com/masque/tools/webServices/WebServices.hx:243) hard-errored
-- $.block and $.structure_type both matched the same tokens, and
tree-sitter's own conflict analysis called it an "unnecessary
conflict" (no tie to break) while it still errored at runtime, the
same not-fixable-via-prec/conflicts class as the documented
block-vs-object empty-body issue. Fix: $.block never legitimately
does anything here -- real Haxe only ever puts var/function member
declarations in a typedef's "class-like" struct body
('typedef Foo = { var x:Int; function bar():Void; }'), both of which
start with a keyword, never a bare identifier. Replaced $.block with
a new dedicated $._structure_class_body rule restricted to exactly
those two declaration kinds (aliased to $.block so existing
consumers/tests see the same node type), which structurally can't
overlap with $.structure_type's bare shape.
2. $.type had no $.structure_type alternative at all, so an anonymous
struct could only appear where a call site hand-rolled its own
choice($.type, $.structure_type) workaround (only $.function_arg
did). 'cb:String->{}->Void' (real example: dailyPuzzleSelector() in
haxe/src/com/masque/tools/webServices/WebServices.hx:74, 6 call sites
total in that file) hard-errored: $.function_type's own two $.type
slots had no way to become '{}', so the parser fell back to matching
it as a bare $.object literal wrapped in an ERROR node. Fix: added
$.structure_type as a bare $.type choice, so every $.type slot
depot-wide gets anonymous-struct support for free, not just the one
call site that already special-cased it.
This introduced a new, real (not "unnecessary") conflict --
'(name: {})' is now ambiguous between $.structure_type and a
parenthesized bare-pair expression whose value is an empty $.object,
since both start with just '{' '}'. Declared the conflict explicitly
and resolved it with prec.dynamic (not a static prec bump, which
fixes this one tie but changes the shift/reduce table globally and
broke an unrelated multi-arrow $.function_type chain elsewhere).
Known limitation, not fixed here: a $.structure_type segment that
ISN'T the last segment of a $.function_type chain still fails to
extend into a further right-nested function_type when the chain's
outer context is a variable/field type or a function's return type
(works fine as a $.function_arg's type, which is what both real
WebServices.hx hits needed). Real remaining hit: mCallback:{}->Void; in
haxe/src/com/masque/tools/webServices/WebServicesURLLoader.hx:27.
Documented in $.function_type's comment (grammar.js) and locked in by
a corpus test in declarations.txt so a future fix is a deliberate,
visible change rather than a silent one. No conflict is even reported
for this case, meaning table construction never considers the
extending derivation -- the same class of gap as the other two above,
just not one where a workaround was found.
Verified: 234/234 corpus tests pass (4 new). WebServices.hx and its
sibling *WebServices.hx files sharing the ?cb:String->{}->Void pattern
now parse with zero errors. Depot-wide sweep: files with a
fully-destroyed top-level declaration ("severe") dropped from 169
(prior session state) to 125 -- 44 more fixed since the previous
fixes' merge point.
… a type)
~148 files in this depot conditionally compile a type annotation
depending on target, e.g.:
function scaleUV(scaleU:#if flash Null<Float> #else Float #end = 1.0,
...):Void;
(real example: haxe/src/away3d/core/base/ISubGeometry.hx:153). Every
$.type slot involved had no way to become a preprocessor-guarded
choice, so these hard-errored.
Added a new $._conditional_type rule (a #if/#else/#end wrapper around
two $.type alternatives, mirroring $.preprocessor_statement's
condition-field shape for consistency with the rest of this fork) and
wired it into the 3 real call sites found while scoping depot-wide:
variable_declaration's type field, function_arg's type field, and
function_declaration/function_expression's return_type field.
Deliberately NOT folded into $.type generally -- $.type is also used
in positions (cast(), extends/implements, type_params, function_type's
own '->' chain, ...) with no real depot usage of a conditional there,
so wiring it in at the 3 confirmed sites keeps the GLR-conflict surface
narrow. Each site generated cleanly with zero unresolved conflicts on
the first attempt (a much smaller blast radius than the $.structure_type
work earlier this session, which needed 2 conflict resolutions across
just 1 injection point).
Scoped to the dominant shape only: a default/initializer, if present,
is shared and sits after the #end. Two rarer variants found while
scoping are explicitly NOT handled and remain known limitations:
~7 files put a *different* default inside each branch
(`wantFlush:#if flash Null<Bool> = false #else Bool = true #end` in
haxe/src/com/masque/tools/miscutil/IAPN.hx), and 1 file duplicates the
statement's own trailing ';' inside each branch
(haxe/src/de/flintfabrik/starling/display/ffParticleSystem/SystemOptions.hx).
Also out of scope, found while surveying but structurally unrelated:
~33 files conditionally wrap a _modifier token before 'function'
(haxe/src/away3d/containers/View3D.hx), and ~43 files conditionally
wrap a chain head/call target mid-expression -- neither is a $.type
slot, so neither is touched here.
Verified: 238/238 corpus tests pass (4 new). Real depot hits now parse
clean: ISubGeometry.hx, IPath.hx (interfaces, no error at all);
SubGeometry.hx/SubGeometryBase.hx/CompactSubGeometry.hx still show
errors but from a separate, pre-existing, unrelated construct (an
array-index chain issue around line 83 of SubGeometry.hx), confirmed
by direct byte-range inspection -- not this fix's concern. Depot-wide
sweep: total files with any parse error 2,700 -> 2,676, severe
(fully-destroyed top-level declaration) 125 -> 122. No regressions.
…lly-compiled type
Extends $._conditional_type (added in the previous commit) to let each
the ~7 files where the default differs per branch instead of being
shared after #end:
function saveSettings(wantFlush:#if flash Null<Bool> = false
#else Bool = true #end):Void;
(real example: haxe/src/com/masque/tools/miscutil/IAPN.hx:21). Also
covers the asymmetric case where only one branch has a default at all:
private var artSwf1:#if USESWFLOADER SwfLoader
#else String = "JustWords.swf" #end;
(real example: haxe/src/com/masque/wordchuck/WordChuckBase.hx:41-44).
The previously-fixed shared-after-#end shape
(`#if flash Null<Float> #else Float #end = 1.0`) still works unchanged:
it's just this rule's per-branch default staying empty both times,
with the real call site's own separate, pre-existing trailing default
consuming the `= 1.0` as before. Harmless no-op at the return_type
call site, which never has a default/initializer to begin with.
Generated cleanly, zero unresolved conflicts.
Verified: 240/240 corpus tests pass (2 new). IAPN.hx now parses with
zero errors. WordChuckBase.hx still has unrelated errors elsewhere in
the file, but its artSwf1-4 field declarations (lines 41-44, the actual
target of this fix) are confirmed clean. Depot-wide sweep: severe
count flat at 122 (byte-for-byte identical file list to the prior
baseline), total errors 2,676 -> 2,675. No regressions -- this was
always a narrow, 2-file fix, so a flat severe count plus both target
files improving is the expected, fully successful outcome.
Still a known limitation, unchanged from the previous commit: 1 file
duplicates the statement's own trailing ';' inside each branch instead
of a default value
(haxe/src/de/flintfabrik/starling/display/ffParticleSystem/SystemOptions.hx).
New dangling_else_statement rule, reachable only as the optional first
element of a #if/#elseif/#else preprocessor branch: an else clause
whose if sits BEFORE the #if, so whether the else exists at all -- or
is a plain else vs an else if -- differs per compile target. Real
examples: `if (cond) {...} #if !STANDALONE else {...} #end`
(haxe/src/com/masque/chat/Player.hx:706), `#if 0 else if (cond) {...}
`#if 0` idiom), and haxe/src/com/masque/sol/SolDisplay.hx:519's
`#if true else {...} #else else if (cond) {...} #end`. Fires at 94
sites in 59 files depot-wide. Trailing statements in the same guarded
span (Player.hx:709-715) parse as ordinary siblings.
The rule's three shape decisions are each load-bearing and documented
at length in grammar.js; the short version of what nine design probes
established:
1. It must be scoped to the preproc-branch-first position, NOT added
as a general $.statement alternative: the general form puts 'else'
into FOLLOW(conditional_statement) everywhere and the table then
deterministically steals EVERY normal if's else into a sibling node
-- and no knob (static prec, prec.dynamic, an explicit conflicts
entry) can flip a deterministic table decision that never forks.
2. The body must be $.block (with an optional else-if condition head),
not the general $.statement, and
3. the rule must end with $._lookback_semicolon: both because the
external scanner's brace-lookback statement-termination handshake
otherwise breaks the statement immediately FOLLOWING the clause
(identifier/var/';'-led statements failed to start;
if/{/return-led ones worked -- the split that exposed the cause).
Also learned, recorded in grammar.js for the next person: adding the
optional clause makes `tree-sitter generate` take ~100s / several GB
peak (baseline ~10s). Earlier probes that looked like unbounded state
explosions and were killed at 300MB-7.4GB were at least partly this
same finite-but-heavy cost misread as intractability.
Verified: 243/243 corpus tests pass (3 new: guarded plain else,
guarded else-if in a disabled branch, guarded else with trailing
statements and a #else branch). Depot-wide census: zero regressions
(severe list byte-identical at 122; no file's error count increased),
total error files 2,675 -> 2,673, and all six known target files
improved (Player.hx 199 -> 192 ERROR/MISSING nodes, AssetManager.hx
203 -> 202, SolDisplay.hx 86 -> 84, MegaMatchScreen.hx 21 -> 20,
MahWordsStatsDlg.hx 9 -> 8, ConcreteVideoTexture.hx 2 -> 1). Aggregate
movement is modest because most affected files carry other,
pre-existing error sites (e.g. subscript-after-call chains) that keep
has_error set; in the worst files the guarded else sits inside a
whole-file ERROR region from an earlier unrelated failure, where no
new rule can engage until that upstream error is fixed.
…sions' into HEAD # Conflicts: # grammar.js
…-values' into HEAD
…ction-type' into HEAD
…e-annotations' into HEAD # Conflicts: # grammar.js
…defaults' into HEAD # Conflicts: # grammar-declarations.js # grammar.js
… additions Dropped entirely during this branch's original conflict resolution along with a redundant, already-upstream comment it was bundled with -- the code was never affected, only the explanation.
…-values' into HEAD
…ction-type' into HEAD
…e-annotations' into HEAD
…defaults' into HEAD
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.
Depends on #84 (
fix/conditional-type-per-branch-defaults) — based on its tip.New
dangling_else_statementrule, reachable only as the optional first element of a#if/#elseif/#elsepreprocessor branch: anelseclause whoseifsits BEFORE the#if, so whether the else exists at all -- or is a plainelsevs anelse if-- differs per compile target. Real examples:if (cond) {...} #if !STANDALONE else {...} #end,#if 0 else if (cond) {...} #end(dead code via the#if 0idiom), and a triple-nested#if true else {...} #else else if (cond) {...} #end.Three shape decisions are each load-bearing:
$.statementalternative: the general form puts'else'into FOLLOW(conditional_statement) everywhere and the table then deterministically steals EVERY normal if's else into a sibling node -- no knob (staticprec,prec.dynamic, an explicitconflictsentry) can flip a decision that never forks.$.block(with an optional else-if condition head), not the general$.statement.Note for reviewers: adding this rule makes
tree-sitter generatetake ~60-100s on the reference test machine (previous baseline 10s. This is not a problem for our use case, graphify, but may make it less suitable for performance-sensitive applications.Verified: 243/243 corpus tests pass (3 new: guarded plain else, guarded else-if in a disabled branch, guarded else with trailing statements and a #else branch). Zero regressions.