Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

DDC generates invalid const objects. #503

Closed
floitschG opened this issue Apr 15, 2016 · 6 comments
Closed

DDC generates invalid const objects. #503

floitschG opened this issue Apr 15, 2016 · 6 comments

Comments

@floitschG
Copy link

floitschG commented Apr 15, 2016

It must always be possible to allocate const objects at compile-time.
DDC doesn't respect this property:

class A<T> {
  List<T> foo() => <T>[];
} 

class C<T> {
  A<T> bar() => const A();
}

main() {
  var a1 = new C<int>().bar();
  var a2 = new C<String>().bar();
}

In this example DDC will happily create two different 'const' instances in the bar function. It would be trivial to change the code to generate an infinity of different "const" objects (since the number of generic types is unbound in Dart).

Generated code:

  const C$ = dart.generic(function(T) {
    class C extends core.Object {
      bar() {
        return dart.const(new (A$(T))());
      }
    }
    dart.setSignature(C, {
      methods: () => ({bar: [A$(T), []]})
    });
    return C;
  });
  let C = C$();
  function main() {
    let a1 = new (C$(core.int))().bar();
    let a2 = new (C$(core.String))().bar();
  }
@floitschG
Copy link
Author

Note that this could have important implications, since the following (now valid) code wouldn't be valid anymore:

class A<T> {
  Iterator<T> _currentExpansion = const EmptyIterator();
}

class EmptyIterator<E> implements Iterator<E> {
  const EmptyIterator();
  bool moveNext() => false;
  E get current => null;
}

@leafpetersen
Copy link
Contributor

Yes. In DDC, const objects are hash-consed at runtime rather than at compile time, to deal with this exact issue. Since strong mode won't let a Foo<dynamic> masquerade as a Foo<NotDynamic>, you need to be able to create const Foo<T> inside of a generic class if you want to get a usable Foo object out. The EmptyIterator case is an example of this.

As we look at implementing strong mode on other platforms, we're going to have decide how to deal with this. Either we change the semantics of const more broadly to deal with this, or else we lose some useful code. For the EmptyIterator case, you could make EmptyIterator be non generic, implementing Iterator<bottom> (if we exposed bottom as a user level type). I don't think this generalizes too far though.

@vsmenon
Copy link
Contributor

vsmenon commented Apr 16, 2016

There are two things happening here. Consider a slightly modified example:

class A<T> {
  const A();
  List<T> foo() => <T>[];
} 

class C<T> {
  A<T> bar() => const A();
}

main() {
  var a1 = new C<int>().bar();
  var a2 = new C<String>().bar();
  var a3 = new C<int>().bar();

  print(a1 == a3);
  print(a1 == a2);
}

In DDC, this will print:

true
false

Where as the VM and dart2js will print:

true
true

So, the two things:

  • DDC is canonicalizing at runtime (the dart.const helper). Because of this, the first print returns true.
  • Strong mode is doing contextual inference on the call to const A() and inferring an const A<T>() instead. DDC allocates that instead. An alternative is to statically reject the code (A<dynamic> is not a valid A<T>).

@lrhn
Copy link

lrhn commented Apr 16, 2016

So strong mode changes const A() to const A<T>(), and the latter isn't a valid compile-time constant expression - but because the former is, and that's the actual syntax, the compiler accepts it anyway. The result is, predictably, inconsistent.

@vsmenon
Copy link
Contributor

vsmenon commented Apr 16, 2016

Yeah, options aren't great in general. In this case, it may be better to reject the code as A<dynamic> can't be used soundly as an A<T>.

@leafpetersen
Copy link
Contributor

This is really a language issue: DDC is just implementing the current strong mode semantics. I've opened a bug here dart-lang/sdk#26291 for discussing this, closing this one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

4 participants