fix(runtime): built-in prototype methods are non-enumerable (spec descriptor) - #2554
Merged
Merged
Conversation
…criptor)
`install_proto_method` stored every built-in prototype method
(Array/String/Number/Object/TypedArray/... .prototype.*) via the ordinary
field-set path, which defaults to { writable:true, enumerable:true,
configurable:true }. Per spec these are non-enumerable, so:
Object.getOwnPropertyDescriptor(Array.prototype, 'map').enumerable // was true
for (k in Array.prototype) ... // yielded 42 methods
Object.keys(Array.prototype) // listed them
all disagreed with Node and failed Test262's pervasive verifyProperty checks.
Record a non-enumerable data-property descriptor at install time via a new
set_builtin_property_attrs helper that, like the #2060 builtin-accessor path,
inserts into PROPERTY_DESCRIPTORS WITHOUT flipping the process-wide
GLOBAL_DESCRIPTORS_IN_USE hot-path gate. getOwnPropertyDescriptor, Object.keys
and for-in each read PROPERTY_DESCRIPTORS per-object and unconditionally, so all
three now observe enumerable:false — while the object get/set hot path keeps
skipping the descriptor table for every program. Now matches Node:
`for (k in Array.prototype)` yields 0; `...map` descriptor is
{ writable:true, enumerable:false, configurable:true }.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Built-in prototype methods (
Array/String/Number/Object/TypedArray/….prototype.*) were installed via the ordinary field-set path, which defaults to{ writable: true, enumerable: true, configurable: true }. Per spec they are non-enumerable, so Perry disagreed with Node on all three observation paths:This fails Test262's pervasive
verifyPropertychecks (every built-in method test verifies the property is non-enumerable, via bothgetOwnPropertyDescriptorand afor-inscan).Fix
Record a non-enumerable data-property descriptor at install time, via a new
set_builtin_property_attrshelper. Like the existing #2060 built-in accessor path, it inserts intoPROPERTY_DESCRIPTORSwithout flipping the process-wideGLOBAL_DESCRIPTORS_IN_USEhot-path gate.This works because
getOwnPropertyDescriptor,Object.keys, andfor-in(which lowers toObject.keys) each readPROPERTY_DESCRIPTORSper-object and unconditionally — so they now observeenumerable:false— while the object get/set hot path keeps skipping the descriptor table for every program (no perf regression).Testing
Now matches Node exactly:
User-object
for-in/Object.keysunaffected ({a,b,c}→['a','b','c']).cargo fmtclean.Scope
Foundational spec-correctness for the reflective layer. On its own it moves the Test262 conformance % only marginally (each
verifyPropertytest also gates onwritable-via-write,configurable-via-delete, exactvalue, and.name/.lengthown-properties — all of which must pass together), but the non-enumerable descriptor is a prerequisite that every built-in-method conformance test needs.Version bump + CHANGELOG entry omitted — please fold in at merge.