-
Notifications
You must be signed in to change notification settings - Fork 847
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
add Object.hasOwn (#1052) #1157
Conversation
Tnx for your contribution! I'm sure one of the committers will take a look at your PR as soon as they can. One thing to note though is that your PR doesn't come with a UnitTest, which normally wouldn't be a problem, as the Test262 testsuite would contain a test for this feature. However, as Object.hasOwn is a brand new thing in EcmaScript and atm we're not yet running against the latest version of Test262 yet (as Rhino is missing a couple of features that are used in the Test262 test suite), there's no test covering this feature. Not sure how we want to handle this, except for trying to implement the missing features in Rhino, so we can run the latest version of the Test262 test suite |
import org.mozilla.javascript.Context; | ||
import org.mozilla.javascript.ScriptableObject; | ||
|
||
public class NativeObjectTest { |
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.
- Is there a test for not following the prototype chain?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn
Object.hasOwn(example, 'toString'); // returns false
- How is
Symbol
handled in the spec?
The build is failing. Could you run $ ./gradlew spotlessApply |
Object arg = args.length < 1 ? Undefined.instance : args[0]; | ||
ScriptableObject obj = ensureScriptableObject(arg); | ||
Object str = args.length < 2 ? Undefined.instance : args[1]; | ||
boolean result; |
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 read the spec.
https://tc39.es/proposal-accessible-object-hasownproperty/
Object.hasOwn
calls the HasOwnProperty
operation in the same way as Object.prototype.hasOwnProperty
. Could you make this common? I think it is better to write it in ScriptRuntime
or ScriptableObject
.
rhino/src/org/mozilla/javascript/NativeObject.java
Lines 207 to 219 in a0a9fb9
boolean result; | |
Object arg = args.length < 1 ? Undefined.instance : args[0]; | |
if (arg instanceof Symbol) { | |
result = ensureSymbolScriptable(thisObj).has((Symbol) arg, thisObj); | |
} else { | |
StringIdOrIndex s = ScriptRuntime.toStringIdOrIndex(cx, arg); | |
if (s.stringId == null) { | |
result = thisObj.has(s.index, thisObj); | |
} else { | |
result = thisObj.has(s.stringId, thisObj); | |
} | |
} | |
return ScriptRuntime.wrapBoolean(result); |
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.
Good point!
The correct location to put it however is org.mozilla.javascript.AbstractEcmaObjectOperations, as the spec defines the HasOwnProperty operation as one of the Abstract Operations on Objects (section 7.3.13).
Do check the notes at the top of the file as to how to add stuff: https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/AbstractEcmaObjectOperations.java#L8
Also possibly see where else the new function ought to be used and see if you can refactor that existing code to use the new function as well
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 changed 'hasOwnProperty' and 'hasOwn' to call the function 'hasOwnPropertyInObject' with the ec9b150 commit. I'll have to find more codes to handle the same process here.
I'm so sorry! I didn't check the code formatting properly. I did code formatting with the e073746 commit. |
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.
looking good!
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.
Looking good to me as well
This seems good. Are you sure that there aren't tests in test262 that can be enabled with this change? You can check for this automatically by doing this (described in testsrc/README.md) ./gradlew test --tests org.mozilla.javascript.tests.Test262SuiteTest --rerun-tas This is fine with me to merge as soon as we verify this. Thanks! |
I couldn't find any. $ find test262/test -type f -exec grep -e Object\\.hasOwn\\b {} + | wc -l
0 |
f94fc66 There seems to be no test for |
Object.hasOwn got proposed and adopted after the version of the test262 suite we're running against got authored, so I don't expect any tests for it in the version we're running against
Wish we could, but if we update to just the next commit in test262 after the one we're on, running the testsuite bombs out because as of that point the test262 suite depends on JavaScript features that Rhino doesn't implement yet, like #958, #913 and #678. The later two of these issues might depend on #967, which is currently waiting for #1159, which in turn is waiting for this pr to get merged, among others :-) Hence those 3 issues are linked to the v1.7.15 milestone, as we should tackle them asap imho. |
OK, I think that we are as far as we're going to get with test262, so I'm going to merge this then. Thanks! |
This failed since mozilla#1157 with "TypeError: Expected argument of type object, but instead had type object"
This failed since mozilla#1157 with "TypeError: Expected argument of type object, but instead had type object"
This failed since mozilla#1157 with "TypeError: Expected argument of type object, but instead had type object"
This failed since mozilla#1157 with "TypeError: Expected argument of type object, but instead had type object"
This failed since #1157 with "TypeError: Expected argument of type object, but instead had type object"
add Object.hasOwn
Closes #1052