Fix optional default-arg typing and improve generated match errors

[cknitt]

Summary

This PR fixes issue #8385 and also addresses a related older bug in optional-argument default lowering.

It does two things:

1. improves the error context for compiler-generated matches used when lowering optional args with defaults
2. fixes typing of annotated optional args with defaults, including the original @react.component / module-signature repro from Regression in optional args typecheck #8385

Background

Optional arguments with defaults are lowered in two steps: first the raw optional value is bound, then the resolved value is rebound after handling Some / None.

That caused two separate problems:

• error reporting could mention a synthetic switch/match that does not exist in user code
• for patterns annotated as option<_>, the compiler reused the original annotation even for the resolved binding, where the value is no longer option<_>

A reduced repro of the older issue is:

let f = (~test: option<int>=42) => {
  let _: int = test
}

The original issue #8385 exposed the same underlying shape through the JSX transform with a @react.component constrained by a module signature.

Changes

1. Improve error message for generated optional-default match

The compiler now marks the synthetic match generated for optional-default lowering and reports errors in a function context instead of as a user-written switch.

This avoids misleading messages like:

this switch is expected to return ...

for code that contains no explicit switch.

2. Fix typing of resolved default-value bindings

When lowering an optional arg with a default, the compiler now adapts the original pattern before rebinding the resolved value.

For example, the synthesized rebinding for:

(~test: option<int>=42)

now treats the resolved binding as int, not option<int>.

This fixes both:

• plain functions with annotated optional args and defaults
• the original JSX / @react.component regression from Regression in optional args typecheck #8385

Tests

Added / updated coverage for:

• improved error output in super_errors
• plain optional-default annotation typing
• the original JSX/module-signature shaped repro in tests/tests

Result

The following now typechecks correctly:

let f = (~test: option<int>=42) => {
  let _: int = test
}

and the original @react.component repro from #8385 also builds again.

[pkg-pr-new]

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@8386

@rescript/darwin-arm64

npm i https://pkg.pr.new/@rescript/darwin-arm64@8386

@rescript/darwin-x64

npm i https://pkg.pr.new/@rescript/darwin-x64@8386

@rescript/linux-arm64

npm i https://pkg.pr.new/@rescript/linux-arm64@8386

@rescript/linux-x64

npm i https://pkg.pr.new/@rescript/linux-x64@8386

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@8386

@rescript/win32-x64

npm i https://pkg.pr.new/@rescript/win32-x64@8386

commit: 979541f

[cristianoc]

Not sure why this should type check?

let f = (~test: option<int>=42) => {
  let _: int = test
}

This type checks:

let f = (~test: int = 42) => {
  let _: int = test
}

That's how things have been expected to be until now, as far as I can remember.
Maybe I'm not up to date with more recent discussions?

[cknitt]

@cristianoc Hmmmm. Maybe the root problem is indeed something different.

Past behavior was this:

module type Test = {
  @react.component
  let make: (~test: int=?) => React.element
}

// Compiles with warning:
// React: optional argument annotations must have explicit `option`. Did you mean `option<int>=?`?
module A: Test = {
  @react.component
  let make = (~test: int=?) => {
    ignore(test)
    React.null
  }
}

// Compiles without warning
module B: Test = {
  @react.component
  let make = (~test: option<int>=?) => {
    ignore(test)
    React.null
  }
}

// Compiles without warning
module C: Test = {
  @react.component
  let make = (~test: int=42) => {
    ignore(test)
    React.null
  }
}

// Compiles without warning
module D: Test = {
  @react.component
  let make = (~test: option<int>=42) => {
    ignore(test)
    React.null
  }
}

The warnings seems to imply that the version with option is the correct one, at least for =?. And it works with =defaultValue, too. So we have always been using that in .res, even though it's actually weird.

Maybe that warning could be removed and the version without option be made the canonical form?
Not sure why we have that warning.

[cknitt]

Same example, but where you can see in the body which type it actually gets:

module type Test = {
  @react.component
  let make: (~test: int=?) => React.element
}

// Compiles with warning:
// React: optional argument annotations must have explicit `option`. Did you mean `option<int>=?`?
module A: Test = {
  @react.component
  let make = (~test: int=?) => {
    // test becomes option<int>
    test->Option.mapOr(React.null, React.int)
  }
}

// Compiles without warning
module B: Test = {
  @react.component
  let make = (~test: option<int>=?) => {
    // test becomes option<int>
    test->Option.mapOr(React.null, React.int)
  }
}

// Compiles without warning
module C: Test = {
  @react.component
  let make = (~test: int=42) => {
    // test becomes int
    React.int(test)
  }
}

// Compiles without warning
module D: Test = {
  @react.component
  let make = (~test: option<int>=42) => {
    // test becomes int
    React.int(test)
  }
}

[cknitt]

I now think that we should remove the warning for A and all special handling, and that A and C should be the allowed forms, everything else should give a type error (B would have option<option<int>> which does not match the body, and D would fail because the body would have option<int> and the default would need to be Some(42), not 42).

Will try that out.

What do you think @cristianoc?

[cristianoc]

I now think that we should remove the warning for A and all special handling, and that A and C should be the allowed forms, everything else should give a type error (B would have option<option<int>> which does not match the body, and D would fail because the body would have option<int> and the default would need to be Some(42), not 42).

Will try that out.

What do you think @cristianoc?

I think it should reflect what happens in the non-component case when defining just a function.
Can't remember if the ? case behaves differently when giving a constant 3 in that setting.
If it does, changing would be a breaking change.
Which can be considered, but will happen in every use of ? in existing code.

[cknitt]

@cristianoc Actually, checking again for "plain" functions without @react.component, we have the exact same behavior there when implementing an interface:

module type Test = {
  let f: (~test: int=?) => int
}

module A: Test = {
  // compiles
  let f = (~test: int=42) => test + 1
}

module B: Test = {
  // compiles
  let f = (~test: option<int>=?) => test->Option.getOr(42) + 1
}

module C: Test = {
  // Error:
  // This pattern matches values of type int
  // but a pattern was expected which matches values of type option<'a>
  // 
  // The value you're pattern matching on here is wrapped in an option, but you're trying to match on the actual value.
  // Wrap the highlighted pattern in Some() to make it work.
  let f = (~test: int=?) => test->Option.getOr(42) + 1
}

[cknitt]

So basically it is ~test: int=42, but ~test: option<int>=? for "plain" function, and it should be the same for @react.component, so the behavior reported in #8385 is actually correct (the change from #8296 just caused the check to become stricter for the PPX case).

The only remaining question is why for PPX ~test: int=? works, too (albeit with a warning).