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

Literal symbols are not accepted as const map keys. #15445

Closed
peter-ahe-google opened this issue Dec 4, 2013 · 18 comments
Closed

Literal symbols are not accepted as const map keys. #15445

peter-ahe-google opened this issue Dec 4, 2013 · 18 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.

Comments

@peter-ahe-google
Copy link
Contributor

The VM rejects the program reported in issue #15442 (which is an analyzer bug):

class A {}
class B {}

main() {
  var map = const <Symbol, int>{#A:0, #B:1};
  print(map);
}

@DartBot
Copy link

DartBot commented Dec 4, 2013

This comment was originally written by @mhausner


Set owner to @mhausner.
Added Accepted label.

@lrhn
Copy link
Member

lrhn commented Dec 4, 2013

I believe Symbol does override Object.operator==, so it's not a valid key in a const Map (per spec).

@peter-ahe-google
Copy link
Contributor Author

The class is defined here: https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/sdk/lib/core/symbol.dart

Clearly, it doesn't override ==.

The rest is implementation details.

@rmacnak-google
Copy link
Contributor

"It is a compile-time error if the key of an entry in a constant map literal is an instance of a class that implements the operator == unless the key is a string or integer."

The keys are not instances of dart.core.Symbol. They are instances of _collection.dev.Symbol, which does define ==.

We should treat symbols like strings and include them in the exceptions.

(Also, presumably the spec meant to restrict when a class's implementation of == is not the one inherited from Object. As it stands, if I interpret "implements" as "defines", then I could put == in a superclass to sidestep the restriction. If I interpret "implements" as "defines or inherits", then the restriction applies to all classes.)


cc @gbracha.

@peter-ahe-google
Copy link
Contributor Author

As I said: _collection.dev.Symbol is an implementation detail.

@peter-ahe-google
Copy link
Contributor Author

As Lasse points out on misc@, the implementation details are leaking through. I've filed issue #15455 to update the library spec.

@lrhn
Copy link
Member

lrhn commented Dec 6, 2013

It's not an implementation detail if
 const Symbol("a") == new Symbol("a")
but
 !identical(const Symbol("a"), new Symbol("a"))
We don't want to compromise on the former, so the only real solution is to make new Symbol("a") return a canonicalized Symbol instance for each String (and ditto for MirrorSystem.getSymbol).

We have not done so yet, or agreed that it must be done.

The user can create arbitrarily many Symbol objects from strings, and when faced with a similar problem for Type, we decided to not canonicalize to avoid unlimited memory bloat from caching the canonicalized versions.

We might be able to do it, using weak references for the cache to avoid keeping more instances alive than what the program can detect - but I don't know if dart2js has that option.

@gbracha
Copy link
Contributor

gbracha commented Apr 26, 2014

As Ryan said, we should simply add symbols to the set of excluded types allowed as keys. I'll file a spec bug to his effect and inform TC52 of our intentions. I don't expect any issues.

@peter-ahe-google
Copy link
Contributor Author

Gilad, I'm losing track of the negations :-)

Are you suggesting that symbols can be used as keys in constant maps? If so, sounds good to me.

@gbracha
Copy link
Contributor

gbracha commented Apr 26, 2014

Peter, yes, const symbols should be allowed as keys in maps and also in
switch statements.

@lrhn
Copy link
Member

lrhn commented Nov 12, 2014

Allowing symbols in switch cases and const maps has still not made it into the specification for switch. It only special-cases int and String.


Removed Area-VM label.
Added Area-VManguage label.

@kasperl
Copy link

kasperl commented Nov 13, 2014

Assigning to Gilad to double check that the spec is (or gets) updated to reflect our decision to allow Type objects in const maps and switches.


Set owner to @gbracha.
Removed Area-VManguage label.
Added Area-Language label.

@gbracha
Copy link
Contributor

gbracha commented Nov 13, 2014

Confused by this last comment. This bug is about Symbols, not Type. The situation with Type is under review AFIK (see issue #21553). As far as this issue is concerned - if this is a language issue, I can close it as fixed right now. The question is whether it is implemented. Please clarify.

@kasperl
Copy link

kasperl commented Nov 14, 2014

Sorry, I seem to have messed this up. Please disregard my comment about Type. Is the spec updated for symbols? If so, we should just turn this back into a Area-VM issue. Not sure why Lasse changed it in the first place.

@gbracha
Copy link
Contributor

gbracha commented Nov 15, 2014

The spec has been fixed and this is in the TC52 approved version awaiting ECMA ratification in December. Reclassifying as a VM bug.


Removed the owner.
Removed Area-Language label.
Added Area-VM, Triaged labels.

@DartBot
Copy link

DartBot commented Nov 17, 2014

This comment was originally written by @mhausner


Set owner to @mhausner.
Added Accepted label.

@DartBot
Copy link

DartBot commented Dec 9, 2014

This comment was originally written by @mhausner


Added Started label.

@DartBot
Copy link

DartBot commented Dec 18, 2014

This comment was originally written by @mhausner


This is now supported in the VM.

$ dart ~/tmp/s.dart
{Symbol("A"): 0, Symbol("B"): 1}
$


Added Fixed label.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

7 participants