Skip to content

const maps with type entries doesn't work properly #17123

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
sigmundch opened this issue Feb 25, 2014 · 10 comments
Closed

const maps with type entries doesn't work properly #17123

sigmundch opened this issue Feb 25, 2014 · 10 comments
Assignees
Labels
closed-invalid Closed as we don't believe the reported issue is generally actionable web-dart2js

Comments

@sigmundch
Copy link
Member

It took me a while to figure out a bug related to this. Dart2js currently allows const maps with types as keys, but lookup functions fail.

Here is a simple example that reproduces the problem:

    class A {}
    var m1 = { A: 1};
    const m2 = const {A: 2};
    
    main() {
      var t = new A().runtimeType;
      print('${m1[A]} ${m2[A]} ${m1[t]} ${m2[t]}');
      print(m2.keys.first == t)
    }
    

This program works in the vm, there it prints:
   1 2 1 2
   true

in dart2js it prints:
   1 2 1 null
   true

@floitschG
Copy link
Contributor

https://codereview.chromium.org/177053004


Set owner to @floitschG.
Added Started label.

@floitschG
Copy link
Contributor

Looks like we are going to disallow Types in const maps.

https://codereview.chromium.org/179293005/

There might be a discussion if we have to special case Types.

@sigmundch
Copy link
Member Author

That's odd - aren't Types consider const symbols too?

@floitschG
Copy link
Contributor

They are. But that doesn't mean that they cannot override their == operator (or hashCode).

It's the same reason they cannot be used in case-clauses.

@floitschG
Copy link
Contributor

When I said "disallow in const maps", I meant as keys. They are still fine as values.

@sigmundch
Copy link
Member Author

I see, thanks for the clarification, makes sense.

I'm obviously not familiar with the implementation details, but I guess I am surprised that Type needs to override ==/hashcode.

@floitschG
Copy link
Contributor

It became necessary to construct types for generic functions. Imagine:

class A<T> {
  foo() => new A<A<T>>();
}

main() { new A<int>.foo().foo().foo()... }

As you can see the set of runtime-types is not known at compile-time. This means that we have to construct them dynamically when needed. Instead of canonicalizing them at runtime, dart2js overrides the equality operator to ensure that they are equal.

@rakudrama
Copy link
Member

Regarding #­7 - could we canonicalize the types?
Canonicalized types would allow caching of expensive type tests.
Most real programs run with a bounded number of types.

@karlklose
Copy link
Contributor

Note that we talk about instances of Type here and not about the runtime types that are used in type checks, so this is only about comparing these instances for equality, as there are no subtype tests on Type

Do we have evidence that this is a performance bottleneck anywhere?

@floitschG
Copy link
Contributor

Closing as invalid.
The original example is now a compile-time error, because the Type class overrides "==".

I created a new issue #17207, to track if we can make the behavior the same on the VM and dart2js.


Added Invalid label.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-invalid Closed as we don't believe the reported issue is generally actionable web-dart2js
Projects
None yet
Development

No branches or pull requests

5 participants