Skip to content

language: specify if type literals are valid const map keys #21553

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

Closed
rakudrama opened this issue Nov 9, 2014 · 24 comments
Closed

language: specify if type literals are valid const map keys #21553

rakudrama opened this issue Nov 9, 2014 · 24 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@rakudrama
Copy link
Member

rakudrama commented Nov 9, 2014

The spec as I read is does not tell us whether or not the following program is legal. It depends on whether the implementation of Type overrides operator==(). The VM and dart2js differ in this regard which is a problem for portability as programs developed on the VM don't work on dart2js (#17207)

It would be helpful to have a ruling on whether this specific combination of features is supposed to work and then have it implemented uniformly.

class A {}

const table = const {
    A: 'Apple',
};

main() {
    print(table[A]);
}

Wrapping the type to avoid operator== as follows leads to surprising behaviour.

class A {}
class B {}

class Key {
    final Type type;
    const Key(this.type);
}

const table = const {
    const Key(A): 'Apple',
    const Key(B): 'Banana',
};

main() {
    print(table[const Key(A)]);  // Apple
    print(table[new Key(B)]);    // null
}

Related issues: #17123 #17207

@rakudrama
Copy link
Member Author

Marked this as blocking #17207.

@gbracha
Copy link
Contributor

gbracha commented Nov 10, 2014

I note that the dartdoc for Type does not list a == method. So either it needs to change, or dart2js should not have a ==. In any case, we should agree on whether any predefined class implements == across all implementations of Dart and what its semantics are and have that specified in the dartdoc.

Since dart2js has different needs than the VM and has a hard time without this override, this would tend to force the VM to have an ==.

That doesn't necessarily mean that we disallow const types as keys in maps. Doing so is arguably a breaking change (at least for server apps) and it seems to be an annoying restriction.

We could add Type to the list of exclusions in section 16.8 and 17.9 - types that implement == and yet can be used as keys or in switches. In this scenario, the code would be legal, and we would have a binding specification == for Type in the dartdoc saying when two Type objects were equal.

Comments? Objections?


cc @floitschG.
cc @iposva-google.
Added Accepted label.

@floitschG
Copy link
Contributor

I don't think dartdoc's output has any impact. Type is an interface and classes that implement it can reasonably override == and hashcode without that being visible in the Type interface.

Will reply to the actual issue later.

@floitschG
Copy link
Contributor

Never mind. The spec actually explicitly states that it must be an instance of "Type". "The run-time type of every object is represented as an instance of class Type which can be obtained by calling the getter runtimeType[...]".

@gbracha
Copy link
Contributor

gbracha commented Nov 11, 2014

So, are we ok with explicitly allowing constant Type objects to be used as const map keys and switch statements? This will go in post ECMA-408 2nd edition (which hopefully will be official in a month's time).

@floitschG
Copy link
Contributor

Stephen has a proposal to make Types canonicalized in dart2js.
I haven't yet had the time to look into it.
Please give me/us a few more days to sort this out (but please keep nagging, if it looks like no progress is made).

@rakudrama
Copy link
Member Author

Regarding #­2:

I believe we need to allow const Types as const map keys.

I would prefer to avoid specifying Type.== and fall back on Object.== since invariably the method has to deal with non-Type arguments and is is worth avoiding the cost of the 'arg is Type' as it is quite high.

Regarding #­4:
I think the spec means only that (x.runtimeType is Type) ==> true, not that there is a single non-abstract class Type with no other implementations.
A clarification here would he helpful.

On the VM: x.runtimeType.runtimeType prints as '_Type'.

I would hope that we can have multiple implementations of Type that also implement an internal interface so that the runtime can do interesting things with specialized subtypes that have optimized private methods.

It would be a penalty in code and heap space to have to wrap the internal type simply to avoid there being multiple implementations of Type.

@gbracha
Copy link
Contributor

gbracha commented Nov 12, 2014

As Stephen says, anything that implements TYpe is considered an instance of Type. It doesn't matter for the == issue though: anyone who chooses to override == must obey the contract for the built in class Type.

@floitschG
Copy link
Contributor

cc @karlklose.

@floitschG
Copy link
Contributor

floitschG commented Nov 13, 2014

Just discussed this with Karl. No resolution yet, but a few discussion points:

  • type literals are always raw. That is, you can't write List<int> as a type literal.
    This means that switching on generic types is dangerous.
  • there can be an infinity of runtime types. Canonicalization would either require a weak set, or could be potentially the source of a memory leak. In practice, it probably doesn't matter.
  • for dart2js the easiest solution would be to canonicalize types that are visible at compile-time (since we already have them as internal constants anyways), and create new types for dynamically generated types. These can only be generic types and wouldn't work in switch clauses.
    The nicest way to enforce this in the spec would be to say that non-generic types and raw types must be canonicalized, but the rest doesn't. That is, all type literals would be canonicalized: int, String, List, Map, ...
    Generic non-raw types, like List<int>, Map<int, String>, wouldn't need to be canonicalized. Note that List&lt;dynamic> would need to be identical to List.

Internally we can easily use two different classes for these types, and can guarantee that the canonicalized version doesn't override "==".
Personally I'm torn: the proposal feels inconsistent, but would solve pretty much all practical problems:

  • switch would work.
  • it's easy to understand.
  • no memory leak.

@iposva-google
Copy link
Contributor

Florian,

  • there can be an infinity of runtime types. Canonicalization would either require a weak set, or could be
    potentially the source of a memory leak. In practice, it probably doesn't matter.

In practice canonicalizing types at runtime is a memory benefit even if they are not stored in a weak set: You limit the number of type objects floating around in the heap.

@floitschG
Copy link
Contributor

I forgot to follow-up, but the more I think of it, the more I think we can canonicalize in the VM too.
I will need to discuss this more with the others, though.

@rakudrama rakudrama added Type-Defect area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). labels Nov 21, 2014
@kevmoo kevmoo added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed accepted labels Feb 29, 2016
@harryterkelsen
Copy link
Contributor

I think that Type should be allowed as const Map keys, and there should be an exception made for them in the spec as there is with Symbol literals.

@lrhn
Copy link
Member

lrhn commented Jun 13, 2016

If we prevent Type from being implementable (like int, String, etc.) it would probably make it easier to special-case it without also special-casing user implementations.

On the other hand, I don't think you should ever be using equality as the test on types. If possible, you should always use is-subtype to test, and we should make that possible and easy.

@leafpetersen
Copy link
Member

Also note this use case from #30257 , which is abstracted from real code that I ran into internally.

class  A {}

void main() {
  var x = new A();
  switch (x.runtimeType) {
    case A: print("hello");
  }
}

cc @lrhn @eernstg @munificent @floitschG

@eernstg
Copy link
Member

eernstg commented Jul 26, 2017

Note that we have abstained from introducing the "user can't extend/implement" constraint on Type on earlier occasions. The concrete reason was that the package reflectable needs to create "fake types" for Type values that our current run-time semantics does not support obtaining without reflection. This may indicate that reflectable is special (so we can have that constraint for "normal" user code anyway), or it may indicate that other "reflection-ish" software may need to create "fake types" as well. Or we may add some other way to create those fake types.

@matanlurey
Copy link
Contributor

FWIW AngularDart is starting to remove all support for using Type at runtime to mean "lookup" something because of the code size effect of supporting "reflection-ish" scenarios. We'd strongly support whatever language option makes our code the smallest over whatever adds more features.

@lowagner
Copy link

I'm writing a flutter app with some tests, and ran into this issue with the switch case, using class inheritance...

i would like to do something like this:

class Parent {}
class Child1 extends Parent {
 int value;
}
class Child2 extends Parent {
 double data;
}
Parent x = Child1();
switch (x.runtimeType) {
 case Child1: doSomethingWith(x.value); break;
 case Child2: doSomethingElseWith(x.data); break;
}

however, the block does not recognize that x must be Child1 inside of that case, so it errors on accessing x.value. Strangely enough, it works if i put extra logic in the block:

switch (x.runtimeType) {
 case Child1:
  if (x is Child1) doSomethingWith(x.value);
  break;
 ...
}

is there a way Dart can be smarter about inferring the types? i understand there's an issue if we use labels in the switch case (or continues), but barring those, we should be able to infer types in these case blocks.

a related issue is that if we're in a block and we check something like this:

for (Parent p in [Child1(), Child2()]) {
 if (!(p is Child1)) continue;
 // do something with p as Child1:
 print(p.value); // ERROR, compiler doesn't see p as type Child1
}

in this block, after the continue, we should be able to infer the type of p as Child1, but no such luck...

@lrhn
Copy link
Member

lrhn commented Jun 15, 2018

@lowagner
We can't infer anything from comparisons to runtimeType since classes can override runtimeType and return arbitrary values. The is check is safe, it actually checks that the object has that type, where x.runtimeType == Child1 only checks that runtimeType returns Child1, which a non-Child1 class can do too.
To get promotion, you need to do an is check.

We could also be smarter about that kind of type promotion, but improved type promotion is another issue (many other issues, actually, e.g., #32236).

@zoechi
Copy link
Contributor

zoechi commented Jun 15, 2018

@lrhn wasn't overriding runtimeType removed in Dart 2?
I think to remember reading that a while ago.

@lrhn
Copy link
Member

lrhn commented Jun 18, 2018

No, I don't think that was ever planned. We were hoping to remove runtimeType completely, and requiring you to use a new Type.of method/constructor instead, but our breaking-change vs importance budget wasn't high enough for that. We may still introduce Type.of at a later time, so there is an alternative to runtimeType, but unless we also allow subtype checks between Type objects, their usefulness really isn't that great.

My recommendation still stands: Never use runtimeType or Type objects for anything except reflection.

@lrhn
Copy link
Member

lrhn commented Jun 22, 2018

To allow Type literals as const map keys (or switch case expressions), the specification needs to be changed to state that it allows objects created from type literals.

It's possible to implement Type both with and without overriding ==, so we cannot let the implementation decide. It should be assumed, and currently is, that Type objects do override ==, so you can't use Type objects.

In general, we need to specify that for all objects that can be constant expressions and where the actual implementation class is platform specific.

@srawlins
Copy link
Member

Type literals are now allowed as Map keys. The following works in the VM, dart2js, and analyzer:

class A {}

const table = const {
    A: 'Apple',
};

main() {
    print(table[A]);
}

I think this issue can be closed; the long discussion about runtimeType being usable in a switch seems like a separate issue.

@eernstg
Copy link
Member

eernstg commented Sep 13, 2019

Agreed; we have separate threads about how to determine equality of reified types, and the title of this issue is a question which was resolved in 0808d29.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests