inconsistencies in constants representations with CFE

[sigmundch]

Dart2js with the new frontend produces a different representation of constants that:

• use "!="
• use redirecting const factory constructors.

Examples:

• 1 != 2 is represented as !(1 == 2) with the CFE

• with the following code, const B<int>() is represented as const A<B<int>>

class A<T> implements B {}
class B<S> {
  const factory B() = A<B<S>>;
}

Some of these cases are seen in tests/compiler/dart2js/model/constant_expression_test.dart

/cc @johnniwinther

[lrhn]

The type of A<B<S>> is B<dynamic>, which is often not a valid instance of B<S>.
In particular const B<int>() should be a compile-time error because const A<B<int>>() would throw when returned from the B<int> constructor (because it doesn't implement B<int>, which is the return type of the constructor).

Am I missing something?

[eernstg]

I'd say that the factory declaration ought to be a compile-time error because it will not satisfy the requirement that the factory returns an instance of the required type. Let's assume that the requirement is satisfied for all S, that is, A<B<S>> <: B<S>. Then we must have B<dynamic> <: B<S>. But that is obviously not true for all S (it used to be true in Dart 1, but it isn't true in Dart 2).

So the factory is unsafe in the sense that the actual type arguments passed to the A constructor do not ensure that the newly created object has the required type.

However, the language specification does not make this an error (Dart 1 lingo: a static warning), it basically specifies that invocations of redirecting factories should be checked as if they had been desugared at the call site; so for const B<int>(); we just check that const A<B<int>>(); is OK on its own, and then we ignore the relationship between the types B<int> and A<B<int>>.

This means that, in general, there is no reason to believe that const B<int>() has type B<int> or in general that an instance creation expression has the type that it contains! (It could be a redirecting factory, and it could yield anything whatsoever.)

That cannot be the intention, so we need to add some constraints on factories.

We do have an update in the pipeline which will make it an error to violate redirecting (factory or not) constructor type annotations in an invocation, but we still do not cover anything which directly says "a redirecting factory constructor which passes actual type arguments to the redirectee constructor must do so in a way where every actual type argument list to the enclosing class which satisfies the type variable bounds must also satisfy the type variable bounds on the redirectee".

I'd suggest that we assume the language specification will specify that a redirecting factory must return an object of the apparent type (e.g., const B<int>() must yield something that has type B<int> or a subtype thereof), and then I'll make sure that said spec update says so.

@lrhn, @leafpetersen, do you agree that we will definitely end up having such a requirement?

[lrhn]

It's more than reasonable, it's essential. The static type of const B<int>() is B<int>. If the result of the invocation is not a B<int>, and it also doesn't throw, then we have a strong-mode type safety violation.

At the very least, the attempt to return a B<dynamic> from an expression with static type is B<int> must be a run-time type error (a failed down-cast). Since this is a const expression, that run-time error becomes a compile-time error.

[eernstg]

I think we should make unsafe redirection in a redirecting factory constructor a compile-time error at least when redirecting to a generative constructor: In that case the static type of the newly created object is known exactly from the redirection expression, so it's not just a downcast (or sidecast, the two types could be completely unrelated with today's treatment), it is a downcast which is statically guaranteed to fail every single time. Just like List<int> xs = <num>[];.

For a redirecting factory whose redirectee may return a subtype (say, it's redirecting to another redirecting factory and at some point to a regular factory) we could say that this is a regular downcast, it may or may not succeed, so we allow it (and prohibit it with --no-implicit-casts) as usual.

[lrhn]

Which is saying that we should treat it as a normal assignment. If the RHS is known exactly, we can make the guaranteed cast error into a compile-time error, otherwise (if redirecting to another factory constructor) we fall back on an implicit down-cast.

We can choose to special-case chains of redirecting factory constructors, because we know that the value is unchanged along the chain, but it's probably not worth it.

(Sorry about hijacking this issue, do go back to the regularly scheduled issue handling).

[eernstg]

I just made this issue part of the background material for the CL on checking redirections.

[sigmundch]

Thanks for looking into this - I had not looked in detail at the inconsistencies of this example with 2.0 semantics (there are more in

sdk/tests/compiler/dart2js/model/constant_expression_test.dart

Line 140 in f66854a

class A<T> implements B {

).

The reason I filed this bug was that I noticed that dart2js has different representations for these constants when using its old front-end and when using the new front-end.

Even in 2.0 valid code like:

class A implements B { const A(); }
class B { const factory B() = A; }

Dart2js shows differences. Here if you creating the constant const B(), dart2js represents it as const B() with the old frontend, but as const A() in the new front end.

[vsmenon]

@sigmundch - I moved this to Beta 3 (trying to delete the beta1 milestone). Is this still an open for dart2js?

@lrhn - Is there a separate issue to file for the for the CFE here? Siggi's original example is a static error in Analyzer (assignability error) but allowed by CFE.

I.e.. the following is a static error in DDC but just prints false in DDK:

class A<T> implements B {}
class B<S> {
  const factory B() = A<B<S>>;
}

void main() {
  print(new B<int>() is B<int>);
}

[sigmundch]

still relevant, beta3 sounds good