Remove dead and unreachable error and warning variants#8459
Merged
JonoPrest merged 26 commits intoJun 2, 2026
Conversation
Drop Ctype.Tags and Ctype.Recursive_abbrev because both exceptions were declared and caught but had no raise site anywhere under compiler/. This also removes the unreachable Typetexp.Variant_tags and Typecore.Recursive_local_constraint wrappers that only existed to report those exceptions. Validation: rg found no remaining references to the removed constructors, and make completed successfully.
Drop Conflict_bs_bs_this_bs_meth because the constructor was only declared and printed; rg found no Bs_syntaxerr.err call or other construction site for it under compiler/. Validation: rg found no remaining references. The full rebuild is delayed by an unrelated coverage Dune process that currently holds _build/.lock.
Drop Label_mismatch, Abstract_wrong_label, and Incoherent_label_order. Label_mismatch's pattern path was catching Ctype.Unify around unify_pat_types even though that helper already translates Unify to Pattern_type_clash. The expression path now uses the existing Expr_type_clash machinery if the defensive invariant ever fails. Abstract_wrong_label and Incoherent_label_order were defensive branches behind modern function and labeled-argument normalization; any fallback now uses the existing Too_many_arguments or Apply_wrong_label diagnostics. Validation: rg found no remaining references. A full rebuild will run once the unrelated coverage Dune process releases _build/.lock.
Drop Invalid_interval and Invalid_for_of_pattern from Typecore.error. The ReScript parser has no source construction site for Ppat_interval, and normalize_for_of_pattern reports destructuring for...of syntax before replacing it with Ppat_any. The old typer guards now use direct Location errors only as malformed-AST fallbacks. Validation: rg found no remaining references to the removed named variants.
Drop Unbound_type_constructor_2, Ill_typed_functor_application, and Apply_structure_as_functor from Typetexp.error. The first needs an inherited constructor whose body expands to a bare Tvar, which ReScript source cannot construct; the latter two only sit under Longident.Lapply, which the parser does not emit. The old guard sites now use direct Location errors as malformed-AST fallbacks. Validation: rg found no remaining references to the named variants.
Drop Type_clash, Parameters_differ, Null_arity_external, Rebind_wrong_type, Bad_fixed_type, Varying_anonymous, and Val_in_structure from Typedecl.error. These guarded paths are rejected or normalized before the named diagnostics can fire in modern ReScript: recursive aliases are caught earlier, external declarations are encoded or rejected earlier, extension rebind result types unify by construction, fixed rows preserve their open row shape, anonymous type parameters are not parsed, and structure val declarations with empty prims do not reach the typer from source. The old guard sites now use direct Location errors as malformed-AST fallbacks. Validation: rg found no remaining references to the named variants.
Drop Cannot_eliminate_dependency, With_makes_applicative_functor_ill_typed, With_cannot_remove_constrained_type, and Scoping_pack from Typemod.error. The dead reports were guarded by paths the current module pipeline does not reach: nondep_supertype falls back instead of raising Not_found, applicative Papply constraints are not parsed from source, destructive type substitutions only get fresh type variables, and the package scoping mismatch is a defensive fallback. The old guard sites now use direct Location errors as malformed-AST fallbacks. Validation: rg found no remaining references to the named variants.
Drop Unhandled_poly_type and Misplaced_label_syntax from Bs_syntaxerr.error. Inline polytypes in external arrow chains and labeled arguments to the legacy ->/#=/## rewrites are not emitted by the ReScript parser. The old guard sites now raise direct Location errors as malformed-AST fallbacks. Validation: rg found no remaining references to the named variants.
Drop Illegal_value_name from Env.error. The ReScript parser does not emit value identifiers named -> or containing #, so the named environment error is unreachable from source. The check_value_name guard now raises a direct Location error for malformed PPX-forged identifiers. Validation: rg found no remaining references to the named variant.
Drop has_label after removing the unreachable Incoherent_label_order path. The full make checkpoint caught it as an unused helper under the repository warning policy. Validation: make -j2 completed successfully after this cleanup.
Drop warning constructors that have no live raise path: old OCaml lexer/class/docstring warnings, unused BuckleScript warnings, Statement_type, Unerasable_optional_argument, and Bs_uninterpreted_delimiters. Statement_type was guarded by a check_application_result argument that is only ever false. Unerasable_optional_argument was disabled around its only check. Bs_uninterpreted_delimiters only covered the old unprocessed js delimiter warning path, which modern string handling no longer uses. The warning numbers remain holes; no existing warning numbers were reused. Validation: rg found no remaining references to the removed warning constructors, and make -j2 completed successfully.
Run the repository formatter after removing the unreachable diagnostics. This keeps the edited compiler modules passing make checkformat without changing behavior.
Record the diagnostics removed on jono/remove-dead-errors and remove their table rows. Reclassify the audited items that are not completely dead, including stale-CMI module-type paths, the live Syntaxerr printer gap, and UTF-8 helper errors retained for defensive/test entry points.
Full build testing showed extension_rebind_mismatch.res exercises Rebind_wrong_type, not Rebind_mismatch. Restore the typed unification diagnostic and update the audit to mark Rebind_wrong_type live while leaving Rebind_mismatch unconfirmed.
Reproductions during PR review showed that two of the three typecore label variants removed by 52bed6c7f are still reachable from regular ReScript source: - Label_mismatch fires when a record literal without an expected type mixes fields from two different record types — disambiguation picks one type per label and the cross-type unify inside type_label_exp fails. Fixture: label_mismatch_record_literal.res. - Abstract_wrong_label fires when a multi-arg function literal has an inner argument label that doesn't match the expected arrow's label. Fixture: abstract_wrong_label.res. Restore both variants, their raise sites, and reporter cases. The third removed variant (Incoherent_label_order) stays removed — every attempt to reach it routed through Expr_type_clash via the modern arity-aware unify path. Update tests/ERROR_VARIANTS.md to mark both rows ✓ with the new fixtures and drop them from the removal list. The pattern-path catch in type_pat_aux stays removed (was genuinely dead because unify_pat_types already translates Ctype.Unify to Pattern_type_clash).
…d_label_syntax
PR-review feedback: replacing dead raise sites with Location.raise_errorf
moves code rather than removing it, and introduces a pattern that doesn't
exist anywhere else in the typer modules (which use raise (Error ...) for
real errors and assert false for unreachable defensive code; zero inline
Location.raise_errorf calls in master).
This commit aligns the cleanup with the existing convention:
1. Variants whose raise sites are confirmed unreachable from the ReScript
parser have those sites rewritten to `assert false (* reason *)` with
a comment citing the parser block or sibling check. The named variant
(decl + reporter) stays removed.
- typecore.Invalid_interval, Invalid_for_of_pattern (parser blocks /
normalizes the AST)
- typetexp.Unbound_type_constructor_2, Ill_typed_functor_application,
Apply_structure_as_functor (parser doesn't construct Lapply or
bare-Tvar Tconstr body)
- typedecl.Type_clash, Parameters_differ (Cycle_in_def fires first),
Null_arity_external (Primitive.parse_declaration sets prim_native_name
always), Bad_fixed_type (is_fixed_type and expand_head agree),
Varying_anonymous (parser rejects `_` type params), Val_in_structure
(parser rejects val outside .resi)
- bs_syntaxerr.Unhandled_poly_type (parser misreads inline 'a. as
deprecated uncurried syntax)
- env.Illegal_value_name (parser doesn't emit '->' or # identifiers)
- typecore.Incoherent_label_order: collapsed into Apply_wrong_label
with a comment explaining why the original second arm was
unreachable
2. Variants where reproduction confirmed they ARE reachable, or where I
couldn't prove unreachability, are restored to named-variant form:
- typemod.Cannot_eliminate_dependency (couldn't reproduce, but
reachable from non-generative functor application paths in
principle - retained conservatively)
- typemod.With_makes_applicative_functor_ill_typed (couldn't reproduce
- retained conservatively)
- typemod.With_cannot_remove_constrained_type (REACHABLE via
destructive substitution on a constrained type, fixture
with_cannot_remove_constrained_type.res)
- typemod.Scoping_pack (couldn't reproduce - retained conservatively)
- bs_syntaxerr.Misplaced_label_syntax (REACHABLE via operator-as-
identifier syntax with labelled arg, fixture
misplaced_label_syntax.res)
3. tests/ERROR_VARIANTS.md updated to (a) split the Removed section into
"Truly dead" and "Defensive unreachable" categories, (b) note the
restored typemod variants with `?` status, (c) add covered rows for
the two new fixtures.
Truly-dead removals (no raise site, transitively dead, or always-false
guard) are unchanged: 3 Ctype exceptions, 2 relay variants
(typecore.Recursive_local_constraint, typetexp.Variant_tags),
bs_syntaxerr.Conflict_bs_bs_this_bs_meth, 14 warning constructors.
Validation: rg confirmed no remaining references to the removed
variants. make compiler, super_errors (745 fixtures), super_errors_multi
(54 fixtures) all pass. make checkformat clean.
Re-audited the defensive "unreachable" removals in this branch by driving each assert false to a real crash with a minimal .res (against a master oracle build). 8 were actually reachable or unproven and are restored: Reachable (each was a confirmed compiler crash; regression fixtures added): - typetexp.Unbound_type_constructor_2 (identity alias in inherit position) - typecore.Invalid_interval (non-char interval pattern, e.g. `1 .. 5`) - typedecl.Null_arity_external (`?`-prefixed external name, arity 0) - typedecl.Varying_anonymous (variance annotation on constrained GADT param) - typedecl.Parameters_differ (non-uniform recursion via object manifest) - typedecl.Bad_fixed_type (fully-bounded closed private polymorphic variant) - env.Illegal_value_name (escaped identifier `\"->"`) Appear dead but unproven (no structural proof, no repro found) — retained as named variants with explanatory comments at definition and raise site rather than assert false, pending further investigation: - typecore.Incoherent_label_order - typedecl.Type_clash Also restores the helpers needed by these paths (list_labels/list_labels_aux/ has_label) and updates tests/ERROR_VARIANTS.md with the corrected reachability and a re-audit correction note. The genuinely-dead removals (14 warnings, the Longident.Lapply functor paths, Tags/Variant_tags, ctype Recursive_abbrev/Unification_recursive_abbrev, Unhandled_poly_type, Conflict_bs_bs_this_bs_meth, the unescaped-"js"-delimiter path, Invalid_for_of_pattern, Val_in_structure) are kept removed.
Continued the reachability investigation of the two variants that were restored-but-unproven. - typecore.Incoherent_label_order: now PROVEN reachable. A not-yet- generalized function value applied more than once with labelled args in conflicting orders (`let f = g => (g(~a=1, ~b=2), g(~b=3, ~a=4))`) reaches the leftover/tvar path in type_unknown_args (type_args reconciles labels by name, so only this path is left). Added fixture labeled_args_incoherent_order.res and replaced the "appears dead" comments with the real trigger. - typedecl.Type_clash: still no reproduction after ~37 recursive / mutual / constraint / row / object / alias shapes. update_type unifies t<fresh params> against t's own manifest — a type against an alpha-renamed copy of itself — which cannot head-clash; every real inconsistency is caught by Cycle_in_def / Recursive_abbrev / Parameters_differ / Constraint_failed / Type_arity_mismatch first. Kept as a named variant (not assert false) with the strengthened structural rationale, pending an airtight proof. 7 of the 8 restored variants now have reproducing fixtures.
The comment claimed the parser "misreads inline `'a.` as the deprecated
uncurried `(. …)` form and rejects it" — that is wrong. Validated the
unreachability from scratch (structural + ~13 empirical externals):
- list_of_arrow only recurses an arrow's return and is only called on an
external's type annotation. The external type, and every arrow arg/return,
is parsed by parse_typ_expr, which never routes to parse_poly_type_expr.
- An inline `'a. …` in an external's arg/return position is a plain syntax
error ("Did you forget a `=` here?"), not a misread uncurried dot.
- Ptyp_poly is produced only for record/object field types and signature
`val` descriptions; a field-nested poly is a non-arrow leaf that
list_of_arrow stops at, never the recursed return.
Conclusion (unreachable) unchanged; only the rationale is corrected. No
behavioral change. ERROR_VARIANTS.md already stated the correct reason.
The branch's earlier label-diagnostic churn left Abstract_wrong_label moved from after Too_many_arguments (master) to after Apply_wrong_label, in the typecore error type (.ml + .mli) and the report_error printer. The move is purely cosmetic (sum-type constructor order / match-arm order) and only created needless diff noise. Moved all three back to match master.
Reviewed the whole diff vs master and reverted reorderings that were not
real changes, so the review shows only the actual edits in place:
- typemod.ml: reverted entirely to master. Its only diff was two report_error
arms (Cannot_eliminate_dependency, With_changes_module_alias) reordered —
pure cosmetic churn, both still raised, no content change.
- typecore.ml: moved list_labels/list_labels_aux back to their original spot
(after type_approx, before check_univars) with the original comment. I had
re-added them before lower_args when restoring, creating a spurious
delete+add pair.
- bs_syntaxerr.ml/.mli: restored to master and re-applied only the intended
change in place — Misplaced_label_syntax had been relocated to the end of
the error type and pp_error; now it stays in its original position and the
only diff is the message typo fix ("support" -> "supported"), which the
fixture already expects.
env.ml/.mli were already byte-identical to master. No behavioral change;
build + format clean, super_errors suite green.
Keep the one-liner at the definition; the full rationale lives at the update_type raise site it points to.
It's a live variant with a fixture (labeled_args_incoherent_order.res), so the behavior is captured by the test, not a comment. Comments are reserved for removals. The raise site is now identical to master.
The 9 re-audited variants (Type_clash, Parameters_differ, Null_arity_external, Bad_fixed_type, Varying_anonymous, Invalid_interval, Incoherent_label_order, Illegal_value_name, Unbound_type_constructor_2) had their per-module table rows deleted and the info relocated to the prose list, causing line drift. Each row is now back in its master position with an in-place status update (⚠ -> ✓ + fixture; Type_clash stays ⚠ "appears dead, retained"), so the diff reads as a row-level edit instead of a move. Verified all four module tables (typecore/typedecl/env/typetexp) now match master row order; remaining gaps are only the genuinely-removed variants.
…handled_poly_type) A prior revert that fixed the Misplaced_label_syntax line move also restored these two dead variants. Neither is reachable: Conflict_bs_bs_this_bs_meth is never raised, and Unhandled_poly_type's only use was replaced with assert false in ast_core_type. Remove both again; keep Misplaced_label_syntax in place.
JonoPrest
force-pushed
the
jono/remove-dead-errors
branch
from
f98f796 to
890bcc0
Compare
JonoPrest
commented
Jun 1, 2026
Contributor
Author
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Notes on why each removed variant is dead or unreachable. Reachable variants are left in place and covered by fixtures; see tests/ERROR_VARIANTS.md for the full per-variant status.
|
|
||
| exception Unify of type_pairs | ||
|
|
||
| exception Tags of label * label |
Contributor
Author
There was a problem hiding this comment.
Tags, and Recursive_abbrev/Unification_recursive_abbrev below, are declared and caught but never raised, so they and the typecore/typetexp relays that depend on them are dead.
| with Unify trace -> | ||
| undo_compress snap; | ||
| raise (Unify (expand_trace !env trace)) | ||
| | Recursive_abbrev -> |
Contributor
Author
There was a problem hiding this comment.
Catches Recursive_abbrev, which is never raised, so this arm is dead.
| let unify_pat_types loc env ty ty' = | ||
| try unify env ty ty' with | ||
| | Unify trace -> raise (Error (loc, env, Pattern_type_clash trace)) | ||
| | Tags (l1, l2) -> |
Contributor
Author
There was a problem hiding this comment.
Tags is never raised, so this relay to Variant_tags is dead. Same for the matching arms in unify_exp_types and unify_pat_types_gadt below.
| | Unify trace -> raise (Error (loc, !env, Pattern_type_clash trace)) | ||
| | Tags (l1, l2) -> | ||
| raise (Typetexp.Error (loc, !env, Typetexp.Variant_tags (l1, l2))) | ||
| | Unification_recursive_abbrev trace -> |
Contributor
Author
There was a problem hiding this comment.
Relays Unification_recursive_abbrev, which is never produced, so this and Recursive_local_constraint are dead.
| Less_general (kind, [(ty, ty); (ty_expected, ty_expected)]) )) | ||
|
|
||
| (* Check that a type is not a function *) | ||
| let check_application_result env statement exp = |
Contributor
Author
There was a problem hiding this comment.
Statement_type was only emitted here, and the sole caller passed statement=false, so it never fired. Removed with the warning.
| Types.val_loc = loc; | ||
| val_attributes = valdecl.pval_attributes; | ||
| } | ||
| | [] -> raise (Error (valdecl.pval_loc, Val_in_structure)) |
Contributor
Author
There was a problem hiding this comment.
A bare val, or an external with no name, is rejected by the parser before typing, so pval_prim = [] outside a signature never reaches here.
| type top_level_unit_help = FunctionCall | Other | ||
|
|
||
| type t = | ||
| | Comment_start (* 1 *) |
Contributor
Author
There was a problem hiding this comment.
These warnings have no emit site anywhere. They are OCaml lexer and object-system warnings that ReScript never produces.
| match ty.ptyp_desc with | ||
| | Ptyp_arrow {arg; ret; arity} when arity = None || acc = [] -> | ||
| aux ret (arg :: acc) | ||
| | Ptyp_poly (_, ty) -> |
Contributor
Author
There was a problem hiding this comment.
External types are parsed by parse_typ_expr, which never produces Ptyp_poly, so this never triggers. Replaced with assert false.
|
|
||
| let check_constant loc (const : Parsetree.constant) = | ||
| match const with | ||
| | Pconst_string (_, Some s) -> |
Contributor
Author
There was a problem hiding this comment.
The builtin PPX rewrites js string delimiters before this iterator runs, so this check never fires. Removed along with its error_unescaped_delimiter and is_unescaped helpers.
|
|
||
| type error = | ||
| | Unsupported_predicates | ||
| | Conflict_bs_bs_this_bs_meth |
Contributor
Author
There was a problem hiding this comment.
Conflict_bs_bs_this_bs_meth is never raised. Unhandled_poly_type below had one use, now assert false in ast_core_type. Both dead.
JonoPrest
commented
Jun 1, 2026
| begin_def (); | ||
| let vars, ty_arg, ty_res = instance_label false label in | ||
| if vars = [] then end_def (); | ||
| (try unify_pat_types loc !env ty_res record_ty |
Contributor
Author
There was a problem hiding this comment.
Dead: unify_pat_types already turns a Unify failure into Pattern_type_clash, so this Unify handler never fires. Record-field pattern mismatches report Pattern_type_clash with or without it (verified against master), so removing it does not change behaviour.
Contributor
Author
|
@codex review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8459 +/- ##
==========================================
+ Coverage 60.68% 60.93% +0.25%
==========================================
Files 373 373
Lines 54095 54016 -79
==========================================
+ Hits 32826 32916 +90
+ Misses 21269 21100 -169
🚀 New features to boost your workflow:
|
![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: 890bcc0a88
ℹ️ 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".
| Lapply has no construction site in compiler/syntax/src/ — ReScript's | ||
| type-level applicative-functor syntax (OCaml's M(X).t) isn't part | ||
| of the grammar *) | ||
| assert false |
There was a problem hiding this comment.
This branch is not only fed by the ReScript parser: the compiler supports -ppx rewriters that can output a modified Parsetree, so a PPX can still produce a Longident.Lapply path. In that case, if the applied module resolves to a structure, this now raises Assert_failure instead of the previous Apply_structure_as_functor diagnostic, turning an invalid/unsupported generated type path into a compiler crash for PPX users.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
@cknitt this is very much up for debate.
I decided to remove these particular variants because I'm not sure how much consideration we should be giving to ppx. Especially with AST that's not producible by the language itself.
Lmk what you think.
Member
There was a problem hiding this comment.
I think this is fine to ignore if Longident.Lapply is indeed unused in ReScript.
(In that case, I think we should remove Longident.Lapply in a separate PR.)
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
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
2 participants
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
Removes compiler error and warning variants that can never be produced, and
adds fixtures for the ones that turned out to be reachable.
Details
Two kinds of removal:
but never raised (such as the ctype
TagsandRecursive_abbrevchains),plus warning constructors with no emit site.
triggering AST shape the grammar never builds. These become
assert falsewith a short comment rather than full deletions.
Every reachable variant stays in place. A few that were initially assumed
dead turned out to be reachable; those are kept and covered by new fixtures in
tests/build_tests/super_errors/.tests/ERROR_VARIANTS.mdis a per-module catalog recording the status andfixture for every variant, and is updated to match.
One case,
typedecl.Type_clash, could not be proven reachable or unreachable,so it is kept with a comment pending further investigation.