Closed
Bug 1286757
Opened 8 years ago
Closed 8 years ago
CallResolveOp shouldn't be invoked for global scope lets
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
INVALID
People
(Reporter: manishearth, Unassigned)
Details
Attachments
(1 file)
1.18 KB,
patch
|
shu
:
feedback-
|
Details | Diff | Splinter Review |
`let x;` in the global scope triggers CallResolveOp on the global. This is unnecessary; `let x` cannot touch or define properties on the global. The root of the problem seems to be `js::CheckLexicalNameConflict` calling GetOwnPropertyDescriptor. Removing this seems to work.
Reporter | ||
Comment 1•8 years ago
|
||
This patch seems to work. `let x` no longer calls GetOwnPropertyDescriptor. bz gave an interesting example, `<script>Object.defineProperty(this, "cat", { value: 5 });</script><script>let cat</script>` -- this is supposed to throw an error since let can't shadow. This patch preserves this behavior -- when `cat` has been defined by `defineProperty`, it still shows up in the `varObj->as<NativeObject>().lookup(cx, name)` check, so it still errors. I'm very unsure about this fix, however. It seems to work but I'm really not familiar with this codebase.
Attachment #8770835 -
Flags: feedback?(shu)
Comment 2•8 years ago
|
||
Your timing is impeccably unfortunate. :-) This code is being modified/rewritten/changed as part of bug 1263355, e.g.: https://github.com/syg/gecko-dev/pull/11 https://github.com/syg/gecko-dev/pull/18 https://github.com/syg/gecko-dev/commit/48a85bf54ae4df492c2368846f46904cdac0b797 Any changes here will get overwritten -- or require merge-pain -- when that branch lands. Given there doesn't *seem* to be any bad consequence to this behavior except inefficiency (at least with the current set of global resolve hooks), I'd say we should wait til that branch has landed to do anything here. I'd guess that branch lands at the start of the next cycle or the one after. One or two more cycles with extra resolve-hook-calling seems very tolerable.
Reporter | ||
Comment 3•8 years ago
|
||
Works for me.
Comment 4•8 years ago
|
||
Comment on attachment 8770835 [details] [diff] [review] Proposed fix This is wrong. Calling the resolve op is necessary on the global because of ES 15.1.11 step 5.c-d [1]. |let undefined|, for instance, is supposed to throw a SyntaxError because 'undefined' is a non-configurable property on the global, and this property along with many other builtins properties like NaN and Infinity that are also considered "restricted global properties" are lazily resolved via the global object's resolve hook. Did you notice this causing performance problems? [1] https://tc39.github.io/ecma262/#sec-globaldeclarationinstantiation
Attachment #8770835 -
Flags: feedback?(shu) → feedback-
Reporter | ||
Comment 5•8 years ago
|
||
No performance problems that I notice, just seemed unnecessary. I hadn't thought of `let undefined` :)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•