Closed Bug 1523791 Opened 6 years ago Closed 6 years ago

Add "name" property for classes as part of ClassDefinitionEvaluation

Categories

(Core :: JavaScript Engine, enhancement, P2)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(
URL
)

Details

Attachments

(1 file, 2 obsolete files)

bug1523791.patch
(deleted), patch
anba
: review+
Details | Diff | Splinter Review
Attached patch bug1523791.patch (obsolete) (deleted) — — Splinter Review

Overview:

BytecodeEmitter.{h,cpp}

  • Add two helper methods to emit anonymous functions, emitAnonymousFunctionWithName and emitAnonymousFunctionWithComputedName.
  • If these helpers are called with a function parse-node, we're simply generating the function bytecode through emitTree and then set the name via setFunName or JSOP_SETFUNNAME.
  • But when called with a class parse-node, we're directly invoking emitClass to pass the correct ClassNameKind mode.
  • Move BytecodeEmitter::emitSetClassConstructorName to ObjectEmitter::emitSetEmptyClassConstructorNameForDefaultCtor, because it's now only used for the special case when an implicitly declared class constructor has the empty string as its inferred name. When option 2 in https://github.com/tc39/ecma262/issues/1049#issuecomment-452923011 gets accepted, we can remove this special case.
  • BytecodeEmitter::emitPropertyList and the PropertyEmitter class needed a few more changes, because we now no longer need to pass any isPropertyAnonFunctionOrClass flags through the various emitter methods.
  • BytecodeEmitter::emitClass and the ClassEmitter class now take care of setting the correct inferred name for anonymous classes.

ObjectEmitter.{h,cpp}

  • Remove ClassEmitter::setName and instead handle the empty string hack directly in ClassEmitter::emitInitDefaultConstructor.

JSFunction.{h,cpp}

  • Rename js::SetFunctionNameIfNoOwnName to js::SetFunctionName now that the own "name" property check is no longer needed.
  • Update js::SetFunctionName to remove the own "name" property check and adjust some comments accordingly.

Interpreter.cpp, BaselineCompiler.cpp, CodeGenerator.cpp

  • Update to use new function name.

jit-test/tests/auto-regress/bug1448582-6.js

  • The "name" property is now added before the static "name" method is added, which means the class will always get an inferred name. Update the test to expect the new behaviour.

tests/non262/Function/function-toString-discard-source-name.js

  • Statically known inferred names are now directly passed to JSOP_CLASSCONSTRUCTOR and JSOP_DERIVEDCONSTRUCTOR, which means if we're discarding the class' source code, the name is still present in the fallback source representation.

Tests:

  • https://github.com/tc39/test262/pull/2057 was already merged, but it's not really relevant for us, because the spec change is only detectable through the property enumeration order for classes, which doesn't work for us because of the JSFunction resolve hook. (The resolve hook adds the "name" property lazily, so our property enumeration order for classes doesn't match the order defined in the spec.) The spec change is also detectable when the "name" property is queried in static class fields initialisers, but we don't support those yet.
Attachment #9040429 - Flags: review?(jorendorff)
Priority: -- → P2
Comment on attachment 9040429 [details] [diff] [review] bug1523791.patch Review of attachment 9040429 [details] [diff] [review]: ----------------------------------------------------------------- This is nice. Thank you. I'm sorry for the slow review. ::: js/src/frontend/BytecodeEmitter.cpp @@ +7671,3 @@ > // [stack] CTOR? OBJ CTOR? KEY? > > + if (propVal->isDirectRHSAnonFunction()) { Maybe we should get rid of the bit that stores this and recompute it each time this is called (at most once per node, I think). Let me know if you think so, I can file a followup bug... ::: js/src/vm/JSFunction.cpp @@ +2469,5 @@ > RootedValue idv(cx, IdToValue(id)); > return NameToFunctionName(cx, idv, prefixKind); > } > > +bool js::SetFunctionName(JSContext* cx, HandleFunction fun, HandleValue name, A few comments in JSFunction.h and .cpp mention SetFunctionNameIfNoOwnName. Please update them. @@ +2475,4 @@ > MOZ_ASSERT(name.isString() || name.isSymbol() || name.isNumber()); > > // An inferred name may already be set if this function is a clone of a > + // singleton function; clear the inferred name in any case. ```cpp // `fun` is a newly cloned function, so normally it can't already have an // inferred name. The rare exception is when `fun` was created by cloning // a singleton function; see comment in CloneFunctionObject. In that case, // the inferred name is bogus, so clear it out. ```
Attachment #9040429 - Flags: review?(jorendorff) → review+

(In reply to Jason Orendorff [:jorendorff] from comment #2)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7671,3 @@

   //            [stack] CTOR? OBJ CTOR? KEY?

  •  if (propVal->isDirectRHSAnonFunction()) {

Maybe we should get rid of the bit that stores this and recompute it each
time this is called (at most once per node, I think). Let me know if you
think so, I can file a followup bug...

We need to have a separate bit, because constant folding may have changed the parse tree, so it's no longer possible to decide if function name inference has to applied when we're in BytecodeEmitter → bug 883377 comment #41.

Attached patch bug1523791.patch (obsolete) (deleted) — — Splinter Review

Updated per review comments, carrying r+.

Attachment #9040429 - Attachment is obsolete: true
Attachment #9043061 - Flags: review+

Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cab6219f4db
Set "name" property as part of ClassDefinitionEvaluation. r=jorendorff

Keywords: checkin-needed

Backed out changeset 7cab6219f4db (bug 1523791) for failing at src/js/src/frontend/ObjectEmitter.cpp on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/738860a7d63307eee5ce83a7e066b7faa224cd16

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=227751698&revision=7cab6219f4dbda4b239b701f33a8aa729207c247

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227751698&repo=mozilla-inbound&lineNumber=5572

Log snippet:

21:57:48 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/js/src/wasm'
21:57:48 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/modules/zlib/src'
21:57:48 INFO - modules/zlib/src/host_trees.obj
21:57:48 INFO - z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.8.4/VC/bin/Hostx64/x64/cl.exe -Fohost_trees.obj -c -nologo -DXP_WIN32 -DXP_WIN -DWIN32 -D_WIN32 -D_CRT_SECURE_NO_WARNINGS -D_AMD64_ -O2 -DDEBUG=1 -Iz:/build/build/src/modules/zlib/src -Iz:/build/build/src/obj-firefox/modules/zlib/src -Iz:/build/build/src/obj-firefox/dist/include -deps.deps/host_trees.obj.pp -Iz:/build/build/src/obj-firefox/dist/include/nspr z:/build/build/src/modules/zlib/src/trees.c
21:57:48 INFO - trees.c
21:57:48 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/modules/zlib/src'
21:57:48 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/js/src/frontend'
21:57:48 INFO - z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.8.4/VC/bin/HostX64/x86/cl.exe -FoUnified_cpp_js_src_frontend3.obj -c -DDEBUG=1 -DENABLE_BINARYDATA -DENABLE_WASM_BULKMEM_OPS -DENABLE_WASM_REFTYPES -DENABLE_WASM_GENERALIZED_TABLES -DENABLE_WASM_GC -DWASM_PRIVATE_REFTYPES -DJS_CACHEIR_SPEW -DJS_STRUCTURED_SPEW -DJS_HAS_CTYPES -DFFI_BUILDING -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -Iz:/build/build/src/js/src/frontend -Iz:/build/build/src/obj-firefox/js/src/frontend -Iz:/build/build/src/obj-firefox/js/src -Iz:/build/build/src/js/src -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -MD -FI z:/build/build/src/obj-firefox/js/src/js-confdefs.h -DMOZILLA_CLIENT -utf-8 -fcrash-diagnostics-dir=z:/build/public/build -TP -nologo -wd4800 -wd4595 -D_CRT_SECURE_NO_WARNINGS -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -Gw -wd4244 -wd4267 -wd4251 -wd4065 -we4553 -GR- -Z7 -O2 -Oy- -WX -wd4805 -wd4661 -wd4146 -wd4312 -deps.deps/Unified_cpp_js_src_frontend3.obj.pp z:/build/build/src/obj-firefox/js/src/frontend/Unified_cpp_js_src_frontend3.cpp
21:57:48 INFO - Unified_cpp_js_src_frontend3.cpp
21:57:48 INFO - z:/build/build/src/js/src/frontend/ObjectEmitter.cpp(682): error C2446: ':': no conversion from 'js::ImmutablePropertyNamePtr' to 'JS::Rooted<JSAtom *>'
21:57:48 INFO - z:/build/build/src/js/src/frontend/ObjectEmitter.cpp(682): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
21:57:48 INFO - cl : Command line warning D9002 : ignoring unknown option '-fcrash-diagnostics-dir=z:/build/public/build'
21:57:48 INFO - z:/build/build/src/config/rules.mk:812: recipe for target 'Unified_cpp_js_src_frontend3.obj' failed
21:57:48 INFO - mozmake.EXE[4]: *** [Unified_cpp_js_src_frontend3.obj] Error 2
21:57:48 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/js/src/frontend'
21:57:48 INFO - mozmake.EXE[4]: *** Waiting for unfinished jobs....
21:57:48 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/modules/zlib/src'
21:57:48 INFO - modules/zlib/src/host_uncompr.obj
21:57:48 INFO - z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.8.4/VC/bin/Hostx64/x64/cl.exe -Fohost_uncompr.obj -c -nologo -DXP_WIN32 -DXP_WIN -DWIN32 -D_WIN32 -D_CRT_SECURE_NO_WARNINGS -D_AMD64_ -O2 -DDEBUG=1 -Iz:/build/build/src/modules/zlib/src -Iz:/build/build/src/obj-firefox/modules/zlib/src -Iz:/build/build/src/obj-firefox/dist/include -deps.deps/host_uncompr.obj.pp -Iz:/build/build/src/obj-firefox/dist/include/nspr z:/build/build/src/modules/zlib/src/uncompr.c
21:57:48 INFO - uncompr.c

Flags: needinfo?(andrebargull)
Attached patch bug1523791.patch (deleted) — — Splinter Review

Updated patch to replace conditional expression with if-else statement to workaround what looks like a MSVC bug. Carrying r+.

Attachment #9043061 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #9043247 - Flags: review+

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09e3e887cdf
Set "name" property as part of ClassDefinitionEvaluation. r=jorendorff

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/d09e3e887cdf

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1528057
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: