Insert type checks when necessary when we redirect factories in place

[efortuna]

Currently, the kernel FE compiles redirecting factories like so:
(dart code)
class A {
A(int i) {}
A.foo(int j) = A;
}
main() {
new A.foo(3);
}

(kernel)
class A extends core::Object {
static field dynamic _redirecting# = [self::A::foo];
constructor •(core::int i) → void
: super core::Object::•() {}
static factory foo(int j) → self::A
let dynamic #redirecting_factory = self::A::• in invalid-expression;
}
static method main() → dynamic {
new self::A::•(3);
}

Specifically, that is, instead of calling "new A.foo(3)" we directly call "new A(3)" which is what the redirecting factory constructor calls. Great.

The problem is, sometimes a redirecting constructor can return a subtype or something that's not related type-wise at all. We need to insert a check at the call-site when compiling in "checked mode" and strong mode (something like (new self::A::•(3) as A) though it doesn't really make sense in this trivial example) since we're essentially doing inlining on behalf of the redirecting factory constructor to ensure that the inlined type is indeed the type claimed by the factory constructor.

@johnniwinther said he talked with @eernstg about this issue.

[johnniwinther]

Even in strong mode, where returning something not A from A.foo is probably not allowed, we still need additional parameter checking, like in:

class A {
  A(num i);
  factory A(int j) =A;
}
main() {
  dynamic v = 0.5;
  new A.foo(v); // We need a check against `double` here.
}

[eernstg]

As Johnni mentioned, I agreed that it seems appropriate to check every step of a redirection chain, but also that the current language specification does not imply firmly that these checks should be performed. For a redirecting generative constructor it says that

A redirecting constructor has no body; instead, it has a redirect clause that specifies
which constructor the invocation is redirected to, and with what arguments.

and for a redirecting factory constructor it says

A {\em redirecting factory constructor} specifies a call to a constructor of another
class that is to be used whenever the redirecting constructor is called.

The latter pretty clearly specifies that one invocation should be replaced by the other invocation (repeatedly, if a redirecting factory constructor redirects to another redirecting factory constructor), and the former at least allows for that interpretation. Hence, it seems reasonable to claim that static and dynamic checks should not be performed on the actual arguments relative to the invocations that were replaced, it's only necessary to check the final invocation.

I'll write a proposal for a specification update saying that all these checks must be performed. This could presumably be implemented as a single check per argument where it's needed, and a single check on the result if needed, where the check will succeed if and only if all the corresponding checks would have succeeded. So, presumably, it won't have to cost anything, it would just be more correct. ;-)

With that proposal in the pipeline I'd still need to get buy-in from Lasse and Leaf, but we would at least know how far we've come in the clarification of this issue.

[eernstg]

Here's the CL for that. Note that there is no guarantee at this point that the language will indeed be adjusted to follow this specification.

If the specification is adjusted as proposed in that CL, I believe that all the static and dynamic checks requested in this issue would be correct. However, we still need to resolve exactly which checks that would be (in the current version of the CL, redirecting factory constructors allow downcasts during redirection, but redirecting generative constructors do not). We may want to change that, for consistency, but the current (seemingly inconsistent) approach arose for a reason, so it's not completely obvious what to do.

[eernstg]

Updated said CL; at this point it does not suffer from the problem described above (redirecting generative & factory constructors are now treated much more uniformly, and they do allow downcasts during redirection, and it is checked).

[eernstg]

The spec CL is in rather good shape now. It's not yet done, reviews awaited. Note that downcasts are allowed during the redirection for a redirecting generative constructor, but not for a redirecting factory constructor. This is what Lasse and I concluded was the most reasonable approach (and I think we're lucky that this is what strong mode does already ;-).

[askeksa-google]

Is this spec change going in?

[eernstg]

My latest remark in the CL is 'PTAL', I'll send a reminder. I do not think that there is anything in there which is about the substance, so it should be safe to assume that it will be landed with the same meaning.

[eernstg]

Said CL has been landed in 715d597. Note in particular that a generative redirecting constructor passes arbitrary arguments on to its redirectee and they are subject to an assignability check (and a dynamic check if there's a downcast), whereas a factory redirecting constructor passes exactly the same arguments along that it receives, and in that case the types must be safe (so each parameter type of the redirecting constructor must be the same as the corresponding parameter of the redirectee, or a subtype thereof—no downcasts allowed and hence no dynamic checks needed). Of course, the original invocation of a redirecting factory constructor may contain downcasts, it's just the step from the redirecting constructor to the redirectee which cannot involve a downcast. Other than that, I don't think there should be anything particularly surprising.

[eernstg]

This CL was landed in 9282a34, and this changes the specification to require dynamic checks (if needed) on actual arguments passed to a redirecting factory constructor:

class A implements B {
  var x;
  A(Object this.x);
}

class B {
  factory B(String s) = A;
}

main() {
  B(42 as dynamic);
}

With the old specification the invocation of the B constructor in main could be compiled directly into A(42 as dynamic) which would not incur any dynamic errors; with the new specification we must check that the actual argument passed to B(...) satisfies the type annotation of that redirecting factory, not just the redirectee.

In other words, with respect to actual arguments, this spec update introduces the requirement for exactly the kind of checking that @efortuna requested in the first place (and it was a mistake that the spec previously did not require them).

On the other hand, there is no need to check the type of the returned object dynamically, because the spec always (since Ivan Posva committed it in 2013, at least ;-) required the redirectee to "have a function type" which is a subtype of the function type of the redirecting factory constructor. The new spec update makes sure that we take type arguments into account when we compute the function type of the redirectee, but that's just a bug fix and the intention hasn't changed.

[askeksa-google]

This seems to have been fixed a while back. All of the examples now give the proper runtime error.