Skip to content

Add variable name to error message#1195

Open
Dragon-Hatcher wants to merge 3 commits into
strata-org:main2from
Dragon-Hatcher:diagnostic-improvement
Open

Add variable name to error message#1195
Dragon-Hatcher wants to merge 3 commits into
strata-org:main2from
Dragon-Hatcher:diagnostic-improvement

Conversation

@Dragon-Hatcher
Copy link
Copy Markdown

Add the variable name to the error message when unification fails on initialization.

@Dragon-Hatcher Dragon-Hatcher requested a review from a team as a code owner May 20, 2026 17:35
Copy link
Copy Markdown
Contributor

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

  1. Underlying unifier error is dropped (fun _ => …). For init x : Map int bool := … where the right-hand side has type Map string bool, the original TC.unifyTypes (Core's, line 96 of Languages/Core/CmdType.lean) reports the constructor-level mismatch via Constraints.unify's formatted output. Substituting a generic expected/inferred message hides the why. Consider preserving the original message:

    let τ ← (TC.unifyTypes τ [(xty, ety)]).mapError fun e =>
      md.toDiagnosticF f!"Variable {x} expected type {xty} but initialization expression has inferred type {ety}: {TC.typeErrorFmt e}"

    TC.typeErrorFmt is already in the typeclass for exactly this reason.

  2. No regression test exercises the new branch. I traced the closest existing test (StrataTest/Languages/Core/Tests/StatementTypeTests.lean:81, Impossible to unify bool with int) and it short-circuits inside inferType before reaching the outer unifyTypes [(xty, ety)] you patched, so the new message is currently uncovered by CI. A 3-line #guard_msgs for init x : bool := #2 (or any concrete-type mismatch where the RHS is well-typed on its own) would lock the wording in.

  3. .set arm has the same pattern (line 63) but is not updated. If the rationale for the new wording is that the variable name + types make the error actionable, the same applies to x := expr mismatches. Worth doing in this PR for consistency, ideally via a small helper:

    private def unifyOrAnnotate (md : Md) (x : P.Ident) (xty ety : P.Ty)
        (τ : T) : Except DiagnosticModel T :=
      (TC.unifyTypes τ [(xty, ety)]).mapError fun e =>
        md.toDiagnosticF f!"Variable {x} expected type {xty} but expression has type {ety}: {TC.typeErrorFmt e}"

    then call from both .init and .set (and reuse anywhere else that surfaces type mismatches against a named binding).

Minor style nit: existing error messages in this file write Variable {x} (no quotes) — see lines 36 and 54. New message uses Variable '{x}'. Pick one. Suggest dropping the quotes for consistency.

Dragon-Hatcher and others added 2 commits May 29, 2026 09:03
Both arms now capture the original DiagnosticModel from TC.unifyTypes and
append it via TC.typeErrorFmt, so the constructor-level mismatch reason is
visible instead of being silently dropped. Also fixes inconsistent quoting
around the variable name (dropped quotes to match surrounding messages) and
extends the same annotation to the .set arm for consistency.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests init x : bool := strata-org#2 and set x := strata-org#2 (where x : bool) to exercise
the new error paths and lock in the wording, including the appended
unifier cause.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added the Core label May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants