[typer] error reporting vs call arguments by kLabz · Pull Request #12935 · HaxeFoundation/haxe

kLabz · 2026-06-08T20:45:35Z

Better error reporting for function-body / call-argument errors

When a call argument fails to type — especially a function literal — errors were frequently reported in the wrong place or with the wrong message:

an error inside the body of a function-literal argument was attributed to the enclosing call argument (... For function argument 'x') instead of being shown at its own position;
in diagnostics mode an unresolved identifier used as a call argument produced a false positive instead of letting optional-argument skipping / overload resolution recover (e.g. an unqualified enum value typed against an earlier, non-matching parameter);
a recovered (errored) function body cascaded a spurious Void should be Int unification error on top of the real one;
an argument that is skipped to a later parameter could report its body error twice, or report a stale error belonging to a different argument.

The fix

Four related changes in the typer:

type_local_function resets in_call_args while typing the body, so body errors are reported in place instead of being re-raised and wrapped by arg_error. It is kept set during overload resolution, where a body error must reject the candidate.
type_local_function additionally recovers an expression body error of a call-argument function literal (e.g. () -> x / function() x). A block body already recovers per-statement inside type_block, but an expression body would otherwise let the error propagate out and be mistaken for a signature failure of the enclosing call argument — which then reported a stale skipped-argument error and masked the real one (Invalid error with optional parameter #12934).
type_block recovers a failed value-position expression as a fresh monomorph instead of dropping it to Void, avoiding a spurious cascade (Void should be Int) against the expected type.
type_ident raises for an unresolved identifier while in_call_args even in diagnostics mode, so the skip / overload machinery can recover (no false positive). unify_call_args rolls back the function-literal body errors of an argument that is skipped to a later parameter (they are re-emitted against the parameter it actually binds to), so they are not reported twice, while preserving once-only side-effect errors such as module loading.

Examples

// #10634 / #7924 — unqualified enum value as a call argument, in diagnostics mode
function test(?x:String, ?e:haxe.macro.Expr.ExprDef) {}
test(EBreak); // before: false positive "Unknown identifier : EBreak"; now: clean

// #12934 — TInt skips ?v:Float and resolves against ?a, the real error is `x`
function foo(x:Int, ?v:Float, ?a:Type.ValueType, ?f:Void->Void) {}
foo(0, TInt, function() x);
// before: "Unknown identifier : TInt" against ?v
// now:    "Unknown identifier : x" (at its own position)

Test coverage

tests/misc/eval/projects/Issue10634/ — call-argument / optional-skip scenarios (unknown ident, enum member, abstract cast, array access, var unify, optional skipping in both orders).
tests/misc/eval/projects/Issue12934/ — enum-value skip with a later function literal whose expression body has an error.
tests/misc/eval/projects/function-body-error-reporting/ — see below.
Server diagnostics: tests/server Issue10634, Issue7924 (no false-positive diagnostics).

Two existing tests lost the now-misleading ... For function argument suffix and had their expected output updated (improvements): Issue11450, Issue8488.

function-body-error-reporting scenarios

Error swallowed by optional-argument skipping

Swallow: a real error that is a side effect of typing an argument (loading the Dup module) must still be reported when the argument is skipped to a later optional parameter. Only function-literal body errors are rolled back on skip (they are re-emitted against the parameter the argument binds to); once-only side-effect errors like this one are preserved.

Closes #5837

Spurious "Void should be Int" cascade

CascadeArray / CascadeVar / CascadeReturn / CascadeCallArg: a function literal whose body has an inner error must report only that error, not an additional unification error on the function's type. A failed value-position expression now recovers as a fresh monomorph instead of collapsing the body to Void, so it no longer cascades against the expected type.

Real error masked by the wrong skipped parameter (FAILS, pre-existing)

MaskStructReturn / MaskStructValue: the argument fails against an earlier optional parameter (?n:Int) and against the intended one (?f/?r), but unify_call_args reports the first stored (irrelevant) mismatch and discards the real one — so only ... should be Null<Int> is shown, hiding Object requires field y. Choosing the right parameter to blame depends on which one the argument best fits and cannot be done cleanly here (reporting the current parameter instead would regress Issue8283, where the skipped parameter is the intended one). MaskStructReturn additionally suffers from an object-declaration double error. Reproduces on development; untouched by this fix.

Those tests are added as disabled tests.

Non-return body error masked

MaskBodyVar / MaskBodyCall: a unification error inside the body, on a parameter inferred from the intended later parameter, used to be fully masked (shown as ... should be Null<Int> for the earlier ?n). It is now reported as the real String should be Int at its own position.

Duplicated error on skip+retype

DuplicateReport: a body error used to be reported twice because the argument is typed once per skip candidate. The body errors of a skipped attempt are now rolled back and re-emitted only once, against the parameter the argument binds to.

Multiple / nested function-literal arguments

MultiBind / NestedSkip / MultiNoSkip exercise more than one function literal in the same call:

MultiBind (take(?f, ?g), both bodies error): both errors are reported at their own positions. Previously only the first was shown (wrapped as For optional f) and the second was dropped.
NestedSkip (a function-literal body containing a skipping call whose argument body errors): each error is reported once, in place, even though the inner argument is skipped to a later parameter.
MultiNoSkip (take(?n:Int, ?f) called with two function literals): there is no room to skip, so the first literal cannot fit ?n:Int. Because a function literal can never be an Int, the resulting signature mismatch is pure noise on top of the body error, so it is suppressed and typing recovers (defaulting the parameter) — so both body errors are reported in place.
SignatureKept ((x:Int) -> ... against ?f:String->Void): a real function-vs-function signature mismatch is kept and reported alongside the body error; only the impossible function-vs-non-function case above is suppressed.

Closes #5522
Closes #7197
Closes #7924
Closes #10634
Closes #12910
Closes #12934

…10634, #7924) Server test for #7924 (optional arg skipping + unqualified enum value), and misc compile-fail tests covering error reporting for failures inside a function literal that is passed as a call argument.

Documents several error-reporting issues when a call argument (often a function literal) fails to type. The .stderr files hold the expected (correct) output, so the still-broken cases are red until fixed and the fixed cases stay green. Failing (known issues, expected output is the tentative correct one): - Swallow: a real error that is a side effect of typing an argument (module load) is silently dropped when the argument is skipped to a later optional parameter. - Cascade*: a block-bodied function literal with an inner error reports the real error plus a spurious "Void should be Int" unification error (array/var/return variants reproduce on development; the call-argument variant on this branch). - MaskStruct*: when an argument is skipped past an earlier optional and then fails against the intended one, only the first (irrelevant) mismatch is reported, hiding the real "Object requires field y". Reproduces on development. Passing (guard behavior fixed on this branch): - MaskBody*: a non-return unification error inside the body, fully masked on development, is now reported with the real "String should be Int". - DuplicateReport: an error reported twice on development (skip + re-type) is now reported once. See #5837, #10634.

…tion (#10634, #7924) When typing a call argument, errors that occur inside the body of a function literal were attributed to the enclosing call argument ("For function argument 'x'"), and in diagnostics mode an unresolved identifier produced a false positive instead of letting optional-argument skipping / overload resolution recover. Four related changes fix this: - type_local_function resets in_call_args while typing the body, so body errors are reported in place instead of being re-raised and wrapped by arg_error (kept during overload resolution, where a body error must reject the candidate). - type_block recovers a failed value-position expression as a fresh monomorph instead of dropping it to Void, avoiding a spurious cascade ("Void should be Int") against the expected type. - type_ident raises for an unresolved identifier while in_call_args even in diagnostics mode, so the skip / overload machinery can recover (no false positive). - unify_call_args rolls back the function-literal body errors of an argument that is skipped to a later parameter (re-emitted when it binds), so they are not reported twice, while preserving once-only side-effect errors (e.g. module load).

…ents (#12934) A block-bodied function literal recovers body errors per-statement inside type_block, but an expression body (e.g. `() -> x` / `function() x`) let the error propagate out of type_function. When the function literal is a call argument, that propagated error was caught as a signature failure, which then reported a stale skipped-argument error and masked the real one: foo(0, TInt, function() x) // wrongly: "Unknown identifier : TInt" for ?v Now a call-argument function literal recovers its body error in place (committed while in_call_args is reset), so the argument binds normally and the real error is reported at its own position (here the unresolved `x`), letting `TInt` resolve against the later Type.ValueType parameter.

…ion-literal tests Record the rollback set as the message prefix newer than messages_before, bailing out if that boundary is no longer a tail (a nested call may have rolled messages back via List.filter) so we never roll back unrelated messages. Tests cover multiple function-literal arguments and a nested skipping call: - MultiBind: two function-literal args, both bodies error -> both reported in place. - NestedSkip: a function-literal body containing a skipping call whose argument body errors -> each error reported once, in place. - MultiNoSkip: no skip room, the first function literal cannot fit ?n:Int.

…after a body error When a function literal whose body already reported error(s) in place is passed to a non-function parameter (e.g. `() -> err` against `?n:Int`), it can never fit, so the trailing signature mismatch is pure noise. Suppress it and recover (default the parameter) so later arguments are still checked. A function-vs-function mismatch is a real signature error and is kept (see SignatureKept). Updates MultiNoSkip (now reports both body errors) and adds SignatureKept.

RblSb · 2026-06-08T23:40:47Z

Also fixes #12910 !

Simn · 2026-06-09T04:46:27Z

The analysis sounds good, but having call_arg_body_messages on the context seems a little quirky data-design wise. We have more code dealing with the correct lifetime of this value than working with the actual value, which suggests to me that this isn't quite right.

How about we generalize in_call_args to some call_arg_context option which carries all the related values? This could maybe also absorb delayed_display, which is of similar dubious nature.

Replace g.call_arg_body_messages and f.in_overload_call_args with a single g.call_arg_context option, Some only while unify_call_args resolves a call. Both moved fields share that exact lifetime, so the manual save/restore and cross-file raw mutation collapse into enter_call_args plus a few accessors. in_call_args stays a separate bool: it is additionally toggled off while typing function-literal bodies and display sub-expressions, a finer lifetime than the call_arg_context. delayed_display is left untouched.

kLabz · 2026-06-09T05:26:47Z

The analysis sounds good, but having call_arg_body_messages on the context seems a little quirky data-design wise. We have more code dealing with the correct lifetime of this value than working with the actual value, which suggests to me that this isn't quite right.

How about we generalize in_call_args to some call_arg_context option which carries all the related values? This could maybe also absorb delayed_display, which is of similar dubious nature.

d4c6f74 should make it better

The reviewer's instinct is half right

The smell is real for one of these values: call_arg_body_messages. Look at how lopsided it is — the value is used in exactly one place (the skip/suppress logic in callUnification.ml:166-182), but its lifetime is managed in three: reset per-arg (:155), save/restore around
the whole loop (:191,197), and written from a completely different file (typer.ml:1312). That's the "more plumbing than payload" the reviewer is reacting to, and they're right.

But the proposed consolidation fights the actual lifetimes

The suggestion assumes in_call_args, in_overload_call_args, call_arg_body_messages, and delayed_display share a lifetime so they can live in one call_arg_context option. The code says otherwise — there are two spots where these are deliberately toggled independently, and
a naive Some/None merge would reintroduce the bug this branch fixes:

typer.ml:1280-1314 — while typing a function-literal body it sets in_call_args <- false (so body errors display in place) but then still appends to call_arg_body_messages afterward (:1312). If body-messages lived inside the option and "not in call args" meant option = None, the container would be gone exactly when we need to write to it. in_call_args being off while the message-accumulator stays live is load-bearing.
typerDisplay.ml:594-661 — handle_display sets in_call_args <- false (:596) but then reads in_overload_call_args (:657) to decide whether to stash delayed_display. So even the two bools have decoupled lifetimes; collapsing them into one "Some carries the overload flag"
option breaks this path.

In other words, there are genuinely two concepts here, not one:

f-level, expression-position (in_call_args / in_overload_call_args): "the expression I'm typing right now is a direct call argument." Toggled finely, nests, reset inside bodies/display.
g-level, call-resolution (call_arg_body_messages, and arguably delayed_display): accumulators scoped to "I am resolving this call's arguments."

Merging across that boundary is what makes it awkward.

What I'd actually do

The cleanup that does pay off, and stays within the spirit of the review, is narrower: group the g-level accumulator(s) into a record that's Some only for the duration of unify_call_args, and fold the save/restore + per-arg reset into push/pop helpers. That kills the
manual let x = ctx.g.… ; ctx.g.… <- … ; (fun () -> ctx.g.… <- x) dance and the cross-file raw mutation, which is the part that's genuinely quirky — without touching the f-level bools that legitimately need their own finer lifetime.

I'd leave delayed_display out of this branch. It's pre-existing display infrastructure with its own per-candidate stack discipline (extract/commit in callUnification.ml), it works, and dragging it into a bugfix branch is cross-cutting risk for no functional gain. If we
want to unify it, that's a separate refactor PR judged on its own.

So my reply to the reviewer would be: agree on the smell, but the right move is to encapsulate the call-resolution accumulator lifetime (helpers + a small record on g), not to merge it with the in_call_args flag — the two have deliberately decoupled lifetimes, and I can
point at the two lines that prove it.

kLabz · 2026-06-09T05:38:57Z

Interesting.. #5522 doesn't show any error anymore with this PR, but exit code is still 1 🫣

Edit: fixed by a432cb3

Simn · 2026-06-09T05:45:13Z

Looks better indeed, but I have to push back on this because one of us is confused about something here:

typer.ml:1280-1314 — while typing a function-literal body it sets in_call_args <- false (so body errors display in place) but then still appends to call_arg_body_messages afterward (:1312). If body-messages lived inside the option and "not in call args" meant option = None, the container would be gone exactly when we need to write to it. in_call_args being off while the message-accumulator stays live is load-bearing.

The call to add_call_arg_body_messages on line 1312 occurs after the Std.finally (fun () -> ctx.f.in_call_args <- old_in_call_args) on line 1285, which restores the previous value.

typerDisplay.ml:594-661 — handle_display sets in_call_args <- false (:596) but then reads in_overload_call_args (:657) to decide whether to stash delayed_display. So even the two bools have decoupled lifetimes; collapsing them into one "Some carries the overload flag"
option breaks this path.

But there is also ctx.f.in_call_args <- snd old; on line 655, which again restores the original value.

Please evaluate this part again and clarify these points.

kLabz · 2026-06-09T06:13:30Z

Trying that introduced some regression (CI run -- ignore the 5522 failure).

The reviewer is right that my original justification was wrong — the restores at typer.ml:1285 and typerDisplay.ml:655 re-sync in_call_args with the context before the in_overload/append reads, so those two lines do not demonstrate divergence. I misread the control flow.

But folding in_call_args into g.call_arg_context is still incorrect, for a different, concrete reason that 9067 exposes:

in_call_args lives on ctx.f (per-field). TyperManager.clone_for_field / clone_for_expr mint a fresh typer_field with in_call_args = false every time typing descends into a new field/expression context — including inline expansion.
That auto-reset is load-bearing: while building the inline call for set_a → set_b, the abstract-this error is raised in a sub-context where in_call_args has reset to false, so it displays in place.
g.call_arg_context is on ctx.g (global, shared across every clone). Folding in_call_args into it means it stays Some through the inline expansion, so the error gets mis-wrapped as ... For function argument 'v'. Hence the 9067 regression.

So the two values belong in different places because they have different scoping:

in_call_args (on f) — field/expression-context scoped; resets automatically when typing clones into a sub-context.
call_arg_context (on g) — call-resolution scoped; deliberately spans sub-contexts to accumulate body messages.

Committing an error sets the sticky part_scope.has_error flag in addition to appending the message. When a function-literal call argument is skipped to a later parameter, we roll back the body errors from the abandoned attempt by filtering them out of part_scope.messages — but has_error stayed set, so the compiler exited with an error status and nothing to print (#5522). Add Common.rollback_messages, which drops the messages and recomputes has_error from the survivors, and use it in unify_call_args.

Simn · 2026-06-11T07:06:49Z

The problem with global values like this is that the typing order is undefined. For example, while typing a call argument we might trigger the resolution of a TLazy value, which could trigger type loading and typing in completely separate parts of the codebase. Any global value then affects this operation, which is incorrect. This is also why disable_report_mode is causing problems and should never have been implemented like that.

To be honest I'm having a hard time following the reasoning here. It already starts with the mention of "sub-context" because I'm not sure what is meant by that. Looking at the test for #9067, the real problem appears to be that we cannot distinguish in which cases we should and shouldn't show the "For function argument" extra message. Surely the answer to that is that we should only show this message for a direct unification failure of the provided expression type against the expected argument type.

Another concern I have is that this PR ignores overloads completely, which have exactly the same problems as optional arguments with regards to our "type this - actually never mind, ignore all these errors and type this instead" approach. A solution to this should be general enough to cover this as well.

Sadly, I can't see a clean way of handling this as long as TLazy and typing passes exist.

kLabz added 9 commits June 8, 2026 21:08

[tests] add server test for 10634

fe5d4d8

[tests] server tests: don't throw, display results

93d17c3

[tests] disable out of scope tests

92a1f71

kLabz requested a review from Simn June 8, 2026 20:45

kLabz changed the title ~~[typer] error reporting vs function literal as call argument~~ [typer] error reporting vs call arguments Jun 8, 2026

kLabz mentioned this pull request Jun 8, 2026

Invalid error with optional parameter #12934

Open

kLabz added 3 commits June 9, 2026 06:54

[tests] Add test for #12910 too

8c47d3c

[tests] Add test for #5522

214cf82

kLabz added 3 commits June 9, 2026 08:30

[tests] Add test for #7197

c7a1ee0

no comment

4c5c1c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[typer] error reporting vs call arguments#12935

[typer] error reporting vs call arguments#12935
kLabz wants to merge 15 commits into
developmentfrom
fix/call-arg-errors

kLabz commented Jun 8, 2026 •

edited

Loading

Uh oh!

RblSb commented Jun 8, 2026

Uh oh!

Simn commented Jun 9, 2026

Uh oh!

kLabz commented Jun 9, 2026

Uh oh!

kLabz commented Jun 9, 2026 •

edited

Loading

Uh oh!

Simn commented Jun 9, 2026

Uh oh!

kLabz commented Jun 9, 2026

Uh oh!

Simn commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kLabz commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Better error reporting for function-body / call-argument errors

The fix

Examples

Test coverage

function-body-error-reporting scenarios

Error swallowed by optional-argument skipping

Spurious "Void should be Int" cascade

Real error masked by the wrong skipped parameter (FAILS, pre-existing)

Non-return body error masked

Duplicated error on skip+retype

Multiple / nested function-literal arguments

Uh oh!

RblSb commented Jun 8, 2026

Uh oh!

Simn commented Jun 9, 2026

Uh oh!

kLabz commented Jun 9, 2026

But the proposed consolidation fights the actual lifetimes

What I'd actually do

Uh oh!

kLabz commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simn commented Jun 9, 2026

Uh oh!

kLabz commented Jun 9, 2026

Uh oh!

Simn commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kLabz commented Jun 8, 2026 •

edited

Loading

kLabz commented Jun 9, 2026 •

edited

Loading