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

Missing properties are not enumerable when checking enumerability #416

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

whitlockjc
Copy link
Contributor

@whitlockjc whitlockjc commented Apr 5, 2018

When checking property enumerability (Object.prototype.propertyIsEnumerable) on a String, missing properties should return false instead of throwing a Property {0} not found. exception.

Fixes #415 (regression in 1.7.7.2)

When checking property enumerability
_(`Object.prototype.propertyIsEnumerable`)_ on a `String`, missing properties
should return `false` instead of throwing a `Property {0} not found.` exception.

Fixes mozilla#415
@gbrail
Copy link
Collaborator

gbrail commented Apr 7, 2018

Thank you for looking at this and fixing it -- it's a real regression.

I'd like to take a little time and figure out if there's a way to fix this without catching the exception and checking the error message on it. Is it just the String class that's throwing an exception from "has"? Perhaps we should fix that, or even fix the implementation of "has" in ScriptableObject so it can't leak out an exception.

@whitlockjc
Copy link
Contributor Author

whitlockjc commented Apr 8, 2018

Fair enough. I just did it this way since the method throwing is used in other places. This approach makes the special handling (false instead of Exception) isolated the changes to the lowest place without affecting other callers. If you'd rather just not try/catch, I can find all consumers and put the Exception throwing logic in there.

@gbrail
Copy link
Collaborator

gbrail commented Apr 9, 2018

Given how many things out there extend ScriptableObject and may have the same undesirable behavior for "has," I think that this is a fine fix.

However I think we have more to fix, as I see that V8 and Rhino differ on the enumerability of string properties and other stuff. So I may open an issue to fix the underlying problem, which requires that classes that override "has" should also override "getAttributes" to return the proper attributes for these properties, as NativeArray already does.

@gbrail gbrail merged commit f3201b5 into mozilla:master Apr 9, 2018
@whitlockjc
Copy link
Contributor Author

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants