Skip to content

Join a record update to its base's type class as an occurrence - #12683

Open
Karakatiza666 wants to merge 1 commit into
roc-lang:mainfrom
Karakatiza666:fix-record-update-poisons-base
Open

Karakatiza666 wants to merge 1 commit into
roc-lang:mainfrom
Karakatiza666:fix-record-update-poisons-base

Conversation

@Karakatiza666

Copy link
Copy Markdown
Contributor

Fixes #12682

The bug

A record update { ..base, f: v } that mismatches its annotation makes post-check panic with "erroneous checked type reached Monotype instantiation". This happens when an earlier top-level constant reads base.

How it happens:

  1. A lookup of base joins the variable class of base.
  2. resumeRecordUpdateCheck in src/check/Check.zig ran unify(record_being_updated_var, expr_var). Unification keeps its second operand as the checked representative (see design.md, "the checked representative"). So the update's own variable became the representative of the whole class of base.
  3. The annotation check on the update fails, and poisonOnMismatch poisons the representative's class. That class is the class of base, so the binding base becomes .err.
  4. The earlier constant was checked before the mismatch and was not retired. Post-check lowers it, meets .err in the type of base, and panics.

If the constant comes after the update, the checker sees the .err when it checks the constant and retires it, so there is no panic. That is why source order matters in the repro.

Why this is a bug

  • It is inconsistent with the other expressions whose type is the type of an operand. A lookup does unify(expr_var, use_var) and a block does unify(expr_var, final_expr_var). In both, the operand keeps the representative and the expression joins the class as an occurrence. The record update used the opposite order.
  • A type error in one expression must not poison a well-typed binding that it reads. base is well typed, and other definitions may already have used its type.

The fix

Unify (expr_var, record_being_updated_var). The base keeps the checked representative, and the update joins its class as an occurrence. The mismatch against the annotation then poisons only the update, and base keeps its type. The user gets the type mismatch report.

The PR also adds a design.md paragraph next to the description of the checked representative. It states the rule for expressions whose type is the type of an operand.

No CACHE_VERSION bump: the serialized layout does not change, and earlier poisoning fixes (#11745, #10707) did not bump it either.

Tests

  • src/check/test/rejected_relation_owner_test.zig, 4 tests:
    • the original repro: ruth keeps type Vet and ruth_label keeps type Str;
    • the same with Str.inspect(base) and no alias;
    • an update in a function body: label : Vet -> Str keeps its type (without the fix it became Error -> Str);
    • the control order (constant after the update) still reports exactly one mismatch and keeps the base type.
  • test/cli/RecordUpdateAnnotationMismatch.roc with a subcommands case in src/cli/test/parallel_cli_runner.zig: roc check reports "type mismatch" and "1 error and 0 warnings", with no panic.

All 4 checker tests and the CLI test fail with the old operand order and pass with the fix. Also run: run-test-zig-module-check (2010/2010), run-test-cli --suite subcommands --filter "record update", run-check-snapshots, run-check-zig-format, run-check-zig-lints, run-check-tidy, run-check-type-checker-patterns, run-check-semantic-audit, run-check-test-wiring.

resumeRecordUpdateCheck unified (base, update). Unification keeps its
second operand as the checked representative, so the update's variable
became the representative of the base's whole class. When the update
then mismatched its annotation, poisonOnMismatch poisoned that whole
class, and the base binding became erroneous. Post-check then lowered an
earlier constant that read the base, met the erroneous type, and
panicked with "erroneous checked type reached Monotype instantiation".

Unify (update, base), the order that a lookup and a block's final value
already use, so a mismatch at the update poisons only the update.
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.

roc check panics on a record update that mismatches its annotation when an earlier constant reads the base

1 participant