Skip to content

Remove dead error variants and warning constructors#1

Draft
JonoPrest wants to merge 31 commits into
masterfrom
jono/remove-dead-errors
Draft

Remove dead error variants and warning constructors#1
JonoPrest wants to merge 31 commits into
masterfrom
jono/remove-dead-errors

Conversation

@JonoPrest

@JonoPrest JonoPrest commented May 27, 2026

Copy link
Copy Markdown
Owner

Remove dead error variants and warning constructors

Cleanup follow-up to rescript-lang#8446. Splits dead-variant removals into two
principled categories:

  1. Truly dead — variants with no raise site, transitively dead
    chains, or always-false guards. Declaration, raise, reporter, and
    any supporting helpers are deleted entirely.

  2. Defensive unreachable — variants whose raise site exists but is
    provably unreachable from the ReScript parser (parser blocks the
    required AST shape, or a sibling check fires first). The raise site
    becomes assert false (* reason *) with a comment citing the specific
    blocker; the named variant and its reporter are deleted. This matches
    the existing typer-module convention (57 assert false uses across
    typecore/typedecl/typemod/typetexp; zero Location.raise_errorf
    calls in those four modules in master).

Variants that turned out to be reachable, or that I couldn't conclusively
prove unreachable, are kept as named variants — no code change to them.

Truly dead — declaration + raise + reporter all removed

ctype.ml (3 exceptions, never raised under compiler/):

  • Tags of label * label
  • Recursive_abbrev
  • Unification_recursive_abbrev of type_pairs

Relay variants (only raised inside catch arms for the dead exceptions above):

  • typecore.Recursive_local_constraint
  • typetexp.Variant_tags

bs_syntaxerr.ml:

  • Conflict_bs_bs_this_bs_meth — no construction site anywhere

warnings.ml (14 constructors):

  • Declared, never raised under compiler/: Comment_start (1), Comment_not_end (2), Method_override (7), Instance_variable_override (13), Illegal_backslash (14), Implicit_public_methods (15), Eol_in_string (29), Eliminated_optional_arguments (48), Bad_docstring (50), Bs_fragile_external (105), Bs_unimplemented_primitive (106)
  • Always-false guard: Statement_type (10) — caller hard-codes statement = false; Unerasable_optional_argument (16) — disabled around its only check; Bs_uninterpreted_delimiters (108) — trigger AST rewritten by builtin PPX before the warning iterator runs

Warning numbers are kept as holes so existing user configs like -w "+a-1-2" keep working (parse_options silently ignores numbers no live constructor maps to).

Defensive unreachable — raise becomes assert false (* reason *)

Variant + reporter removed; raise site rewritten to match the established convention.

typecore.ml:

  • Invalid_intervalassert false (* parser has no construction site for Ppat_interval *)
  • Invalid_for_of_patternassert false (* parser's normalize_for_of_pattern replaces non-var patterns with Ppat_any *)
  • Incoherent_label_order → collapsed into Apply_wrong_label with a comment (restoring has_label for an assert-false marker would mean re-introducing 3 helper functions just to flag one branch)

typetexp.ml:

  • Unbound_type_constructor_2assert false (* parser rejects bare-Tvar Tconstr body shapes like type 'a t = 'a *)
  • Ill_typed_functor_application, Apply_structure_as_functorassert false (* both branches live behind Longident.Lapply, which the parser never constructs *)

typedecl.ml:

  • Type_clash, Parameters_differassert false (* every recursive abbreviation hits Cycle_in_def first *)
  • Null_arity_externalassert false (* Primitive.parse_declaration always assigns the magic prim_native_name encoding *)
  • Bad_fixed_typeassert false (* is_fixed_type and expand_head agree on every parser-produced shape *)
  • Varying_anonymousassert false (* parser rejects \_` in type-parameter positions *)`
  • Val_in_structureassert false (* pval_prim = [] outside a signature only arises from parser external-recovery, post syntax error *)

bs_syntaxerr.ml:

  • Unhandled_poly_typeassert false (* parser misreads inline 'a. as deprecated uncurried (. …) syntax *)

env.ml:

  • Illegal_value_nameassert false on both guard branches (parser doesn't emit "->" or #-containing identifiers)

Variants restored to named-variant form

Three variants had already been restored during the original audit pass
(typedecl.Rebind_wrong_type via extension_rebind_mismatch.res,
typecore.Label_mismatch via label_mismatch_record_literal.res, and
typecore.Abstract_wrong_label via abstract_wrong_label.res).

Two additional variants restored in the same review pass — empirical
fixtures confirmed they fire on real ReScript source:

  • typemod.With_cannot_remove_constrained_typemodule type T = S with type t<'a> := 'a against S containing type t<'a> constraint 'a = int. Fixture: with_cannot_remove_constrained_type.res.
  • bs_syntaxerr.Misplaced_label_syntax — labelled args passed via operator-identifier syntax: \"->"(x, ~b=...). Fixture: misplaced_label_syntax.res.

Three more typemod variants restored conservatively — I couldn't
construct a reproducing fixture, but couldn't conclusively prove
unreachability either, so they stay as named variants pending stronger
evidence:

  • typemod.Cannot_eliminate_dependency (non-generative functor application paths)
  • typemod.With_makes_applicative_functor_ill_typed (destructive substitution on applicative-functor signatures)
  • typemod.Scoping_pack (first-class module packing with with type constraints)

What's left in the catalog

After this PR, ERROR_VARIANTS.md contains:

  • — covered by at least one fixture.
  • ☐ (needs <X> harness) — reachable but requires a CLI / binary / PPX / build harness, out of scope for super_errors{,_multi}.
  • ? — three typemod variants where reachability couldn't be confirmed or denied with a fixture.

No rows.

Validation

  • rg confirmed no remaining references to truly-dead removed variants.
  • 745 super_errors + 54 super_errors_multi fixtures pass; 925 stdlib mocha tests pass.
  • make checkformat clean.
  • Five fixtures added/restored across the audit: extension_rebind_mismatch.res, label_mismatch_record_literal.res, abstract_wrong_label.res, with_cannot_remove_constrained_type.res, misplaced_label_syntax.res.

@JonoPrest JonoPrest left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see review below)

Comment thread compiler/ml/typecore.ml
| Abstract_wrong_label of arg_label * type_expr
| Scoping_let_module of string * type_expr
| Not_a_variant_type of Longident.t
| Incoherent_label_order

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: sibling error fires first (S). Original raise site: typecore.ml:3894 in type_unknown_args's argument-label dispatch — fires when arity_ok && has_label l1 ty_fun (the label exists in the function type but at a different position). In modern uncurried ReScript, type_function (typecore.ml:3480–3488) eagerly unifies the function literal's arity-aware Tarrow with ty_expected_ via unify_exp_types, which raises Expr_type_clash before the wrong-position branch is reachable. Reproduction attempts — f(~b=1, ~a=2), cross-pipe label-flipping (apply_both(f, f) with mirrored call sites), partial application against fresh Tvars — all routed through Expr_type_clash. The dead-removal collapses the else raise Incoherent_label_order arm into Apply_wrong_label, which is the same diagnostic the sibling not (has_label l1 ty_fun) arm already raises.

Comment thread compiler/ml/typecore.ml
| Modules_not_allowed
| Cannot_infer_signature
| Not_a_packed_module of type_expr
| Recursive_local_constraint of (type_expr * type_expr) list

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: transitively dead (G). Only raised in unify_pat_types_gadt's Unification_recursive_abbrev → catch arm (typecore.ml:368, removed in the same commit). Unification_recursive_abbrev is itself only raised in Ctype.unify's Recursive_abbrev → catch arm (ctype.ml:2734). Ctype.Recursive_abbrev has zero raise sites under compiler/rg 'raise.*Recursive_abbrev' (excluding the unrelated live typedecl.Recursive_abbrev variant) returns nothing. Chain is dead end-to-end.

Comment thread compiler/ml/typecore.ml
| Recursive_local_constraint of (type_expr * type_expr) list
| Unexpected_existential
| Unqualified_gadt_pattern of Path.t * string
| Invalid_interval

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: parser blocks AST (P). Only raised when type_pat_aux sees Ppat_interval _. ReScript's parser (compiler/syntax/src/res_core.ml) has no construction site for Ppat_interval — interval patterns ('a' .. 'z') are OCaml syntax not part of the ReScript grammar. Fixture 'a' .. 'z' in a switch compiles silently (the parser sees a range expression, not an interval pattern). Replacement Location.raise_errorf keeps the original message for any PPX-forged AST.

Comment thread compiler/ml/typecore.ml
| Unqualified_gadt_pattern of Path.t * string
| Invalid_interval
| Invalid_for_loop_index
| Invalid_for_of_pattern

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: parser normalizes (P). Only raised in type_pat_aux for for ... in ... bindings whose pattern isn't a variable or _. ReScript's parser runs normalize_for_of_pattern (compiler/syntax/src/res_core.ml:3841) which catches every non-var, non-_ shape, emits a syntax error, and replaces the pattern with Ppat_any before the typer runs. Fixture for (a, b) in xs { ... } rejected at parse time with Did you forget a \"(\" here?. The named typer variant is dead behind the parser check; Location.raise_errorf preserves the message for any PPX-forged shape.

Comment thread compiler/ml/typetexp.ml
type error =
| Unbound_type_variable of string
| Unbound_type_constructor of Longident.t
| Unbound_type_constructor_2 of Path.t

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: parser blocks AST (P). Raised in transl_type_aux (typetexp.ml:472/617) when an inherited type's body expands to a bare Tvar — needs type 'a t = 'a plus a downstream inherit. ReScript syntax requires type parameters in angle brackets (type t<'a> = 'a); the parser rejects leading-'a form. Fixture type 'a t = 'a rejected as syntax error. The shape needed (bare-Tvar Tconstr body) is parser-unreachable; Location.raise_errorf covers any PPX-forged AST.

Comment thread compiler/ext/warnings.ml
| Open_shadow_identifier of string * string (* 44 *)
| Open_shadow_label_constructor of string * string (* 45 *)
| Attribute_payload of string * string (* 47 *)
| Eliminated_optional_arguments of string list (* 48 *)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: no raise site (G). Declared but never raised. rg 'prerr_warning.*Eliminated_optional_arguments' returns zero matches under compiler/.

Comment thread compiler/ext/warnings.ml
| Attribute_payload of string * string (* 47 *)
| Eliminated_optional_arguments of string list (* 48 *)
| No_cmi_file of string * string option (* 49 *)
| Bad_docstring of bool (* 50 *)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: no raise site (G). Declared but never raised. rg returns zero matches outside warnings.ml. Also default-disabled in bsc_warnings.ml.

Comment thread compiler/ext/warnings.ml
| Bs_polymorphic_comparison (* 102 *)
| Bs_ffi_warning of string (* 103 *)
| Bs_derive_warning of string (* 104 *)
| Bs_fragile_external of string (* 105 *)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: no raise site (G). Declared but never raised. rg returns zero matches outside warnings.ml.

Comment thread compiler/ext/warnings.ml
| Bs_ffi_warning of string (* 103 *)
| Bs_derive_warning of string (* 104 *)
| Bs_fragile_external of string (* 105 *)
| Bs_unimplemented_primitive of string (* 106 *)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: no raise site (G). Declared but never raised. rg returns zero matches outside warnings.ml.

Comment thread compiler/ext/warnings.ml
| Bs_fragile_external of string (* 105 *)
| Bs_unimplemented_primitive of string (* 106 *)
| Bs_integer_literal_overflow (* 107 *)
| Bs_uninterpreted_delimiters of string (* 108 *)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification: PPX rewrites the trigger AST (S). Raised at bs_warnings.ml:29 for Pconst_string (_, Some "js"). The parser does emit Some "js" for all strings in ParseForTypeChecker mode (res_core.ml:1066), but the builtin PPX runs first: Bs_builtin_ppx.mapper calls Ast_utf8_string_interp.transform_exp which replaces Some "js" with Some "j" (escaped) or Some "*j" (backtick) before Bs_ast_invariant.check_constant ever sees the constant. With -bs-no-builtin-ppx, the warning iterator is skipped entirely (ppx_entry.ml:58–62). Either way the warning never fires for a real ReScript source program.

@JonoPrest JonoPrest left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(superseded — see other reviews for full validation)

@JonoPrest JonoPrest left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline validation comments — for each removed item, the verification method and concrete evidence. Categories: G grepped dead (no raise site under compiler/), P parser blocks the required AST, S sibling error fires before this one, R reachable but the post-removal Location.raise_errorf emits the same user-facing message. Two variants are marked R — they're still safe to remove, but the original audit's "unreachable" framing was too strong.

@JonoPrest JonoPrest left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update (commit 4c8c646)

Reworked per review feedback. Two principled changes:

1. Defensive unreachable raise sites now use assert false (* reason *) instead of Location.raise_errorf — matching the established typer-module convention (57 existing assert false uses in typecore/typedecl/typemod/typetexp; zero Location.raise_errorf calls in those four modules in master). The earlier approach moved code rather than removing it and introduced a foreign pattern.

2. Five variants restored to named-variant form:

  • typemod.With_cannot_remove_constrained_type — confirmed reachable (module type T = S with type t<'a> := 'a with constrained S). New fixture: with_cannot_remove_constrained_type.res.
  • bs_syntaxerr.Misplaced_label_syntax — confirmed reachable (\"->"(x, ~b=...)). New fixture: misplaced_label_syntax.res.
  • typemod.Cannot_eliminate_dependency, typemod.With_makes_applicative_functor_ill_typed, typemod.Scoping_pack — couldn't reproduce, retained conservatively as named variants with ? status in the catalog rather than removed.

Inline comments above: the 14 still-anchored variant-removal comments correctly identify each variant as unreachable. The replacement-mechanism notes in them say Location.raise_errorf — that has now been switched to assert false (* reason *) (same justification, different syntax). The 5 comments for now-restored variants have been deleted.

Net diff: 22 named error variants removed (down from the original 24) + 3 supporting Ctype exceptions + 1 helper (has_label) + 14 warning constructors. 545 deletions / 328 additions across 29 files.

dependabot Bot and others added 5 commits May 28, 2026 17:10
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5 to 6.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@v5...v6)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-version: '6'
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Upload PR dev-playground bundles

* Add safe dev playground PR bundle previews

* Post a comment with the playground URL
* Use Yojson and lsp types for analysis JSON output

Replace the vendored JSON helpers with Yojson and the OCaml lsp types
across analysis, reanalyze, and tools JSON handling.

This updates LSP response construction for completions, hovers,
diagnostics, code actions, symbols, signature help, and related CLI
commands, refreshes analysis snapshots, and adjusts extract-codeblocks
decoding for the new serialized output.

* Delete Protocol.ml

* Delete comments

* Fix LSP JSON serialization

Serialize analysis command results with Yojson output helpers instead of
string extraction, fixing document symbols so they return JSON arrays
instead of crashing.

Also use typed LSP enum constructors for completion and inlay hint
kinds, clean up JSON config parsing, and update analysis test snapshots.

* Use Lsp.Uri for URI handling

Replace the custom URI record with Lsp.Uri.t and delegate path/URI
conversion to the LSP library.

This fixes interface-aware analysis results so definitions, references,
and renames consistently target .resi files when an interface is
present,
and updates the analysis snapshots accordingly.

* remove `null`

* Update GenericJsxCompletion.res.txt

* Update tests

* Update tests/analysis_tests/tests-sourcedirs-dependency/src/expected/Main.res.txt

* Update tests/tools_tests

* Remove null fields, `data` and `deprecated`

* Update tests/analysis_tests/tests-generic-jsx-transform/src/expected/GenericJsxCompletion.res.txt

* Update analysis_tests

* Update analysis_tests

* Remove unused pathToUri

* docstrings: remove decodeFromJson implementation

* Use basename instead of full URI in Xform test output

* Fix rename `newUri`

* Use only `Uri` instead of `Lsp.Uri`

* update CHANGELOG.md

* Omit null fields from doc extraction JSON

Only include optional doc extraction fields like deprecated, detail,
payload, and moduletypeid when they have values. Update tool test
snapshots to match the leaner JSON output.

* Update dune-project and dune files with yojson

* Add `version` for textDocument in rename

* Apply codex suggestions

* rename: use `Uri.toPath`
* Update lsp and yojson dependencies

Relax LSP-related dependencies to allow 1.23.0+ and update yojson pins
to 3.0.0 across dune-project and opam package files.

Also trims trailing whitespace in the CI workflow.

* Update dune-project
…#8454)

* analysis: fix namespace parsing after Yojson migration

* Changelog

* Move yojson helpers to reanalyze and reuse them there

* Some more tests
@JonoPrest JonoPrest marked this pull request as ready for review June 1, 2026 15:45
@JonoPrest JonoPrest force-pushed the jono/remove-dead-errors branch from 9ed7f8a to 4c8c646 Compare June 1, 2026 15:45
@JonoPrest JonoPrest marked this pull request as draft June 1, 2026 15:56
@JonoPrest JonoPrest force-pushed the jono/remove-dead-errors branch from 6c59d51 to 3b05612 Compare June 1, 2026 19:50
JonoPrest added 17 commits June 1, 2026 20:02
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.
JonoPrest added 9 commits June 1, 2026 20:02
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 JonoPrest force-pushed the jono/remove-dead-errors branch from f98f796 to 890bcc0 Compare June 1, 2026 20:02
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.

3 participants