Skip to content

Need API for creating private symbols w/r/t a given library #13355

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
rmacnak-google opened this issue Sep 16, 2013 · 13 comments
Closed

Need API for creating private symbols w/r/t a given library #13355

rmacnak-google opened this issue Sep 16, 2013 · 13 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-mirrors

Comments

@rmacnak-google
Copy link
Contributor

Symbol LibraryMirror.createSymbol(String)?

@peter-ahe-google
Copy link
Contributor

This is somewhat iffy, but we do like to use constant symbols, so we could say:

Symbol LibraryMirror.createSymbol(Symbol)

If the given symbol doesn't start with an underscore, it is added.

So this would work:

myLibraryMirror.createSymbol(#_privateName);
myLibraryMirror.createSymbol(#privateName);
myLibraryMirror.createSymbol(new Symbol('privateName'));

@rmacnak-google
Copy link
Contributor Author

Our current thinking is LibraryMirror.createSymbol() should take a string and emit the same warning(hint?) as new Symbol.

@anders-sandholm
Copy link
Contributor

Added Area-Library label.

@peter-ahe-google
Copy link
Contributor

In order for my suggestion to make sense, the method should be renamed to createPrivateSymbol.

I discussed this method with Ryan, and we both agreed it would be nicer if you could pass in strings. However, then dart2js needs to warn (as it does for new Symbol). However, dart2js cannot reliably warn about instance methods (due to optional types). So I would prefer a static method on MirrorSystem.

@floitschG
Copy link
Contributor

wrt comment #­1: createPrivateSymbol(#_privateName) and createPrivateSymbol(#privateName) should not return the same symbol.

It is perfectly legal to have private variables "___foo", and it should be clear how to construct a symbol for it.

wrt comment #­4: I currently don't see the big advantage of using strings (after all it's just a new Symbol(string) away), but even with symbols we would need to track the symbols or always assume that the private version of a symbol exists too.

Personally I prefer instance methods, but I can understand if that won't work here.

@jmesserly
Copy link

Marked this as blocking #13559.

@jmesserly
Copy link

Marked this as blocking #13881.

@rmacnak-google
Copy link
Contributor Author

MirrorSystem.getSymbol added in r28551


Added Fixed label.

@jmesserly
Copy link

is there a dart2js bug for this?
just in case, I filed:
https://code.google.com/p/dart/issues/detail?id=14231

(when only one side is fixed, we can't use it yet from library code, so I need a new bug # to mark things blocked against.)

@DartBot
Copy link

DartBot commented Nov 18, 2013

This comment was originally written by [email protected]


MirrorSystem.getSymbol("foo") will always throw an ArgumentError, I assume line 49 should be:

if ((library != null && library is! LibraryMirror) ||

48 /* patch */ static Symbol getSymbol(String name, [LibraryMirror library]) {
49 if (library is! LibraryMirror ||
50 ((name[0] == '_') && (library == null))) {
51 throw new ArgumentError(library);
52 }
53 if (library != null) name = _mangleName(name, library._reflectee);
54 return new _symbol_dev.Symbol.unvalidated(name);
55 }

@peter-ahe-google
Copy link
Contributor

We shouldn't check the type, that's what checked mode is for.

@rmacnak-google
Copy link
Contributor Author

Greg, yes.

Peter, I still want to check the type before handing things off to a primitive where it would hit an assertion error and crash rather than throw a runtime error. (Say you passed a ClassMirror instead of a LibraryMirror, which also has a reflectee which is a MirrorReference, but which has the wrong VM object inside.)

https://codereview.chromium.org/61753019/

@peter-ahe-google
Copy link
Contributor

I can implement LibraryMirror myself. You need to find a different way to protect native code.

@rmacnak-google rmacnak-google added Type-Defect library-mirrors area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Nov 19, 2013
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. library-mirrors
Projects
None yet
Development

No branches or pull requests

7 participants