Skip to content

symbols should be canonicalized #15455

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
peter-ahe-google opened this issue Dec 4, 2013 · 7 comments
Closed

symbols should be canonicalized #15455

peter-ahe-google opened this issue Dec 4, 2013 · 7 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue

Comments

@peter-ahe-google
Copy link
Contributor

Symbols should be canonicalized to ensure that it is crystal clear they can be used as map keys.

@peter-ahe-google
Copy link
Contributor Author

Added Area-Library label.

@DartBot
Copy link

DartBot commented Dec 4, 2013

This comment was originally written by @mhausner


Aren't they canonicalized implicitly because they are allocated with const Symbol(...)?

@rmacnak-google
Copy link
Contributor

identical(const Symbol('foo'), new Symbol('foo')) ?

@lrhn
Copy link
Member

lrhn commented Dec 5, 2013

This is only a problem with "new Symbol(x)" where x is not a compile time constant.
If x is a compile time constant, it is easily replacable by "const Symbol(x)" at the source level (and the compilers can do that).

However, for "new Symbol(x)" where x is a dynamic value, it would require caching all created symbols, which is a potential memory leak.
We could make the cache weak, so that when there is no references left to the symbol, we collect it, but that would still risk exposing a new hash code. Example:
 var s = someStringExpression;
 var s1 = new Symbol(s).hashCode;
 // do something that causes a GC
 var s2 = new Symbol(s).hashCode;
 Expect.equals(s1, s2); // shouldn't be able to fail, but can.

So, we might need to keep the symbols alive forever. That's something we have avoided in other cases, where we did not want to force canonicalization because of the risk of a memory leak.

We do warn that using "new Symbol" can cause your program to bloat, but I don't think we should do it blindly.

Maybe we could just canonicalize symbols that occur as constants in the program already, and create other symbols dynamically as a subclass that overrides operator==. That way, you will always get identical(#a, new Symbol("a")) , but not necessarily identical(new Symbol("b"), new Symbol("b")) if #b does not occur as a constant. That would allow using the constants as map keys and switch cases, and require retaining anything but the constants that are already there.

It needs to work for private symbols too, so MirrorSystem.getSymbol needs to be in on the trick. It should also return something that has the same class as for public symbols, or you can't use them in a switch, which is an artificial problem.

This is all about the library Symbol class(es). It should not affect user classes implementing Symbol (which are allowed).

@lrhn
Copy link
Member

lrhn commented Dec 16, 2013

I'll change the documentation if the implementations will promise to implement the canonicalization - that's the hard part, after all.


cc @floitschG.
cc @iposva-google.

@iposva-google
Copy link
Contributor

Symbols allocated with "new Symbol('k')" should not be required to be canonicalized.

@lrhn
Copy link
Member

lrhn commented Jan 29, 2014

I believe the current plan is to special-case symbols in switch cases at the spec level (like we do for String and int).


cc @gbracha.
Added NotPlanned label.

@peter-ahe-google peter-ahe-google added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue labels Jan 29, 2014
@peter-ahe-google peter-ahe-google self-assigned this Jan 29, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue
Projects
None yet
Development

No branches or pull requests

6 participants