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

Jump to bottom

CFE inference bug: redirecting const factory constructors #33813

Closed
sigmundch opened this issue Jul 11, 2018 · 6 comments
Closed

CFE inference bug: redirecting const factory constructors #33813

sigmundch opened this issue Jul 11, 2018 · 6 comments
Assignees
@chloestefantsova @jensjoha
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-bazel customer-dart2js P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone
Dart 2.1.1

Comments

@sigmundch
Copy link
Member

When using redirecting factory const constructors, inferred type-parameters seem incorrect.

Consider these 2 programs v1.dart:

class _X<T> {
  const factory _X() = _Y<T>;
}

class _Y<T> implements _X<T>{
  const _Y();
}

class A<T> {
  _X<T> x;
  A(this.x);
}

class B<T> extends A<T> {
  B(): super(const _X());
}

main() {
 print(new B().x.runtimeType);
}

and v2.dart:

class _Y<T> {
  const _Y();
}

class A<T> {
  _X<T> x;
  A(this.x);
}

class B<T> extends A<T> {
  B(): super(const _Y());
}

main() {
 print(new B().x.runtimeType);
}

In v1.dart, we print _Y<Null>, in v2.dart this produces an error because we try to create _Y<B.T>.

You can reproduce this on the VM (where you get an error in v2) and dart2js (where you get a compiler crash because we didn't expect such input).

Note this was working correctly in -dev.67. I'm seeing this issue in internal customer apps using the latest code in master. Their code contains uses of Stream.empty, similar to the code below:

import 'dart:async';

class A<T> {
  Stream<T> x;
  A(this.x);
}

class B<T> extends A<T> {
  B(): super(const Stream.empty());
}

@mit-mit could you help us triage?
@sigmundch sigmundch added P1 A high priority bug; for example, a single project is unusable or has many test failures area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-dart2js customer-bazel labels Jul 11, 2018
@sigmundch sigmundch added this to the Dart2Stable milestone Jul 11, 2018
@chloestefantsova
Copy link
Contributor

Thanks for filing it, @sigmundch. It's a long-standing one. I've recently landed a pre-requisite for the fix: #32130. Going to look at this one tomorrow and fix soon.

@sigmundch
Copy link
Member Author

Thanks Dima, in case it is relevant, I did a bisect and noticed that that same commit aa6dc1e to fix #32130 is where this problem started to show up.

@chloestefantsova
Copy link
Contributor

So, I initially thought that it's an instance of #32049 which is a known issue with type inference and redirecting factory constructors. But I've come to the conclusion that it's not quite what I though it is originally.

My understanding of type inference is that the argument to super in both cases should be inferred as B.T, and the error should be reported in both examples. Although, I'm not sure if it should be a compile-time error or a run-time error. @leafpetersen Could you clarify how type inference should work here?

@sigmundch
Copy link
Member Author

just an update on our end, we have a workaround for the issue, so I don't consider this a blocker anymore. I've removed the Dart2Stable milestone.

@sigmundch sigmundch modified the milestones: Dart2Stable, Dart2.1 Jul 12, 2018
@leafpetersen
Copy link
Member

My understanding of type inference is that the argument to super in both cases should be inferred as B.T, and the error should be reported in both examples.

Not quite. Inference is done exactly as with anything else, but then the inferred type arguments are replaced with their least closure. So B.T is inferred, and then replaced with Null in each of them. This is a valid static and runtime value to provide, so there are no errors.

@chloestefantsova
Copy link
Contributor

Thanks for the clarification, Leaf!

I've created CL 64841 to fix the issue. It appeared to be a one-liner that passes the isConst flag to the inference procedure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chloestefantsova chloestefantsova

@jensjoha jensjoha

Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-bazel customer-dart2js P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Milestone
Dart 2.1.1
Development

No branches or pull requests
4 participants
@chloestefantsova @sigmundch @leafpetersen @jensjoha