Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constructor tear-off of forwarding factory constructors is under-defined. #3427

Open
lrhn opened this issue Oct 24, 2023 · 7 comments
Open
Labels
bug There is a mistake in the language specification or in an active document

Comments

@lrhn
Copy link
Member

lrhn commented Oct 24, 2023

The constructor tear-off specifiction defines the behavior of a tear-off as "equivalent to" tearing off a corresponding static function, which has the same type parameters as the class, the same parameter list as the constructor, and a return type which is the class-type instantiated with those type arguments.

That definition doesn't work for all redirecting factory constructors with optional parameters, because those cannot have default values, which makes the corresponding static function also not have default values, even if the optional parameter's type is non-nullable. That is, the "corresponding static function" is not a valid Dart function.

(And this is, again, why we shouldn't define things in terms of desugaring.)

The current approach cannot be saved by saying that we should use the default value of the (transitive) redirect target's parameter, which must also be optional and theregore surely have one. That parameter can have a wider type, and a default value which isn't valid for the redirecting factory's parameter.

Example:

class C { 
  factory C([int x]) = D; 
}
class D implements C {
  C([int? x]) { assert(x == null); }
}
void main() {
  C Function([int]) f = C.new;
  assert(f.runtimeType == typeof<C Function([int])>);
  f();
}
typedef typeof<T> = T;

There is no valid desugaring which can introduce a Dart function for C.new which behaves like calling C.new should
(that is, calling that function works just like calling C.new with the same arguments would).

I suggest we rewrite the places in the specification where we introduce a wrapper which copies default values, into forwarding argument lists.

Constructor tear-off:

The result of tearing off a constructor D.C of a class declaration D with type parameters TP and parameter list P
is a function value with signature D<TP> Function(P).
When that function is invoked with type arguments TS and argument list A, it returns the result of invoking the constructor D.C on the instantiated type D<TS> with argument list A.

The result of tearing off the same constructor from an instantiated type, tearing off D<TS>.C, is a function with
signature D<TS> Function(P') where P' is P with type arguments TS substituted for type parameters TP.
Invoking that function with argument list A returns the result of invoking D<TS>.C with argument list A.

(or something to that effect.)

Basically accept that not all function signatures have default values, even where user-declared functions must have them.

The same principle should be applied to generic function instantiation, implicit or explicit. It introduces a new function with a signature which has no type arguments and where the parameter and return types replaces the type parameters with the instantiating type arguments. Calling that function invokes the original function with those type arguments and the same argument list. Again, without any attempt to copy default values, because there are functions which do not have default values - even if it's only redirecting factory constructor tear-offs, for now.

@lrhn lrhn added the bug There is a mistake in the language specification or in an active document label Oct 24, 2023
@eernstg
Copy link
Member

eernstg commented Oct 25, 2023

So how do we understand the nature of a redirecting factory constructor? Note that it is quite similar to an abstract instance member declaration in several ways:

abstract class A {
  A._();
  factory A([int x]) = B;
  void foo([int x]);
}

class B extends A {
  B([num x = 3.25]): super._();
  void foo([num x = 3.25]) {}
}
  • The instance member A.foo is a compile-time-only entity that specifies a member signature constraint on the concrete declaration of foo that any instance typable as A must have. So (apart from possible covariant parameters), the concrete method must have a function type which is a subtype of the function type of A.foo. Default values are not needed for a compile-time-only entity, and for A.foo we wouldn't be able to specify a non-misleading value anyway.
  • Redirecting factory constructors are described in the language specification as compile-time-only entities (they 'avoid the need for forwarders'). In this case the rules enforce (without exceptions) that the redirectee (the constructor which is the ultimate target of the redirection, here: the constructor named B) has a function type which is a subtype of the function type of the redirecting factory (here: the constructor named A). Hence, the redirecting factory does not need default values, and in some cases (like the example above) we can't even claim that there is a default value that works for the given declaration and isn't misleading. (Of course, we aren't even allowed to specify a default value in a redirecting factory declaration anyway.)

So we should probably have specified constructor tear-offs to use the ultimate redirectee when a redirecting factory is torn off, and given the tear-off expression the function type of the redirecting factory. In the example, this means that the tear-off expression A.new would evaluate to B.new, but it would have the static type A Function([int]).

Compared to the approach used today, this would preserve the static type of tear-off expressions, and it would yield a run-time value whose type is a subtype thereof. In other words, this would be a non-breaking change (except that A.new.runtimeType == A Function([int]) would no longer be true, but A.new is A Function([int]) would still be true).

This removes the need to do anything magic/inconsistent about this kind of tear-off, because there is no need for a forwarding function with special powers wrt default values, and there is no need for new concepts like functions that are forwarding an actual argument list (which is not something that we can write in Dart).

I don't see a need to introduce any special magic for generic function instantiation: If a generic function has one or more default values then they must be type correct for every possible actual type argument list, and hence there's no need to invoke the special power of "passing on the actual argument list".

In the case where we would introduce a more powerful kind of default values such as non-constant expressions in the scope of the enclosing function declaration, we would presumably not be able (or willing) to prevent side effects. This would make the ability to "pass on the actual argument list" observable, which makes it more of a liability to sneak it in at this time.

@lrhn
Copy link
Member Author

lrhn commented Oct 26, 2023

Tearing off the (transitive) target is a sound solution.

It's not without its own leakage, though.
If the target is a private constructor which provides the ability to pass in values that are not otherwise possible using the public API, perhaps because it takes an extra argument that requires validation or is encoded on an implementation specific rest, then that ability would leak through the tear-off.
You'd have to cast the function to it's actual runtime type, but you can.
And if someone does, it's potentially breaking to change the underlying constructor, even if it's private.

An alternative is to say that synthetic methods are allowed to have a default value which isn't assignable to the parameter type.
What a synthetic methods does internally is not limited by what Dart syntax can describe, it does what it's described to do, neither more nor less.
That is effectively what the VM is doing, and Dart2JS in production mode, so we'd be specifying the current functioning implementation, just disabling one type check in checked mode.

(If we do introduce non-constant default values, then forwarding the actual arguments list is probably precisely what we'd want, rather than having to specify copying of an expression outside of its scope.)

@eernstg
Copy link
Member

eernstg commented Oct 26, 2023

There is no valid desugaring ...

Actually, this shouldn't matter because the declaration of C is a compile-time error in the first place:

It is a compile-time error if a formal parameter of $k'$ has a default value
.

However, this error apparently hasn't been implemented. So we need to consider whether we want to implement it (a breaking change, perhaps with some actual breakage), or we want to change the specification such that it isn't an error anyway.

In the latter case we must of course specify what to do with that ill-defined default value.

The language specification says that a redirecting factory specifies a call to another constructor, not a constructor in itself, and it says that the invocation of a redirecting factory gives rise to an invocation of the redirectee with the exact same actual argument list.

All these elements match up perfectly with the perspective that there is no entity corresponding to a redirecting factory, it is just like an abstract member signature, and it's all just a compile-time device that allows us to access the ultimate redirectee using a different static type and a different name. At run time we get the redirectee, no ifs and buts.

It's not without its own leakage, though.

That's a very good point! It doesn't quite match up with the language specification, but it seems to be in line with the constructor tear-off feature specification.

However, if we want to support the specified semantics and turn a redirecting factory constructor into a real thing then we need to specify that the torn-off redirecting factory constructor is a function that has optional parameters as declared, and it forwards every call with exactly the same actual arguments. No default values needed, just a magic ability to determine whether or not an optional argument was passed, and then a body that is able to call the redirectee passing an argument to every possible subset of the optional parameters.

This is a capability that we might want to provide in some other situations as well (in order to obtain a faithful forwarding function), so we probably want to turn that concept into a proper language mechanism. It would not necessarily be a feature which is accessible to developers (perhaps we don't specify any concrete syntax for it, at least not at this time), but the mechanism must be well defined.

@lrhn
Copy link
Member Author

lrhn commented Oct 26, 2023

the declaration of C is a compile-time error in the first place

So it is.
That phrasing is not actually sufficient (it seems to assume that k' is not itself a redirecting constructor with no default value), but the intention is clear.

Since we have had code violating that restriction on the SDK platform libraries, it's probably at least a little dangerous to enable it now. And mostly unnecessary, we haven't had any problems with it before constructor tearoffs.

we need to specify ...
(Forwarding the actual arguments from a function).

That's one approach, and I'm all for having it available - it can solve other similar issues too, but for this particular issue, it's sufficient to allow that particular function to have a default value which is not a subtype of its parameter type.
Or not have a default value at all, for an optional parameter.
It's all invisible from the outside, where a user should only be able to see what is returned when calling with any particular argument list. Extensional behavior only, only guided and limited by the public function signature.

No matter how it is done, it will be detectable to the user that the function behaves differently when you don't pass an argument, to how it behaves for any allowed argument. They just can't see how it's done, which is a great place to be for implementation freedom.

(That is: we should specify how things behave, extensionally, not how they should be implemented.)

@eernstg
Copy link
Member

eernstg commented Nov 4, 2023

Note that forwarding functions would allow us to get the desired semantics (and not make the situation described in this issue a compile-time error).

@davidmorgan
Copy link
Contributor

Just a note that we've seen confusion due to this

class FooImpl implements Foo {
  FooImpl([int? x]);
}

abstract class Foo {
  factory Foo([int x]) = FooImpl;
}

void main() {
  Foo(null); // ???
}

what was intended was that Foo should accept null x just like FooImpl, as an oversight the ? was missed; because we were mid-null-safety-migration it was not immediately breaking everywhere, and then callers of the API were incrementally fixed as we migrated to pass a non-null default. Whoops.

So making the mismatch an error would have helped in this case.

Another case where it has shown up is when the concrete constructor is generated. In that case the exact constructor type is not very visible, so it's easy to typo it --> also probably good for it to be an error if there is a mismatch.

Thanks.

@eernstg
Copy link
Member

eernstg commented Nov 15, 2023

... probably good for it to be an error if there is a mismatch

About the ability of a redirecting factory constructor to declare formal parameters with more strict types than the redirectee:

class FooImpl implements Foo {
  FooImpl([num x = 1.5]);
}

abstract class Foo {
  factory Foo([int x]) = FooImpl; // The default value would be a type error here.
}

void main() {
  Foo(); // Creates a `FooImpl(1.5)`, but we can't pass that argument explicitly.
}

There is an error which is currently specified, but not implemented, and the spec might be changed to make it a non-error. However, that's only about the situation where the parameter is optional in the redirectee, and it has a default value whose type isn't a subtype of the type of the corresponding parameter of the redirecting factory.

So we'd actually need a check (a lint?) that flags all cases where the formal parameter types in the redirecting constructor are proper subtypes of the ones in the redirectee, not just the cases where we have the default value with those extra properties mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug There is a mistake in the language specification or in an active document
Projects
None yet
Development

No branches or pull requests

3 participants