-
Notifications
You must be signed in to change notification settings - Fork 859
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
Disable Java Map accessing by properties #836
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is the next PR that we should merge as we make List and Map work better. Can you please take a look at a few improvements? Thanks!
@Override | ||
public boolean has(String name, Scriptable start) { | ||
if (map.containsKey(name)) { | ||
return true; | ||
Context cx = Context.getContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"getContext" will throw a RuntimeError if there is no Context on the stack. That's usually incorrect but I know that there's code out there that might do it. Could you please replace these tests with:
Context cx = Context.getCurrentContext();
if (cx != null && cx.hasFeature(Context.FEATURE_ENABLE_JAVA_MAP_ACCESS))
(or make a helper function in Context or ScriptRuntime since we do that ll the time!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, those changes help. Thanks!
Closes #820
This will have an impact on these Pull Requests. #824, #827
#713 had compatibility issues because entry access by properties took precedence over Java methods. If Java methods are prioritized, compatibility issues will probably disappear.
But here's what happens.
getMap
returnsjava.util.Map
, but an instance of it isjava.util.TreeMap
, so you can use thejava.util.TreeMap
method. If Java methods are prioritized, in order to access it by property, you need to avoid theTreeMap
method instead ofMap
. You always need to know what the instance is. Also, ifgetMap
returns a different Map implementation, your existing code may not work.Nashorn has probably extended the use of indirect-eval-call to distinguish between property access and method invocation. I'm also checking for behavior such as the distinction between
m[0]
andm['0']
, and that Java methods are not JavaScript Functions.