1467052 - Re-design Scope object and frontend in order to distinguish vars and top-level functions not with offset

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

from https://github.com/binast/ecmascript-binary-ast/issues/40

Asserted*Scope in BinAST spec doesn't distinguish between "var" and "function",
so we cannot create Scope object at the point of compiling Asserted*Scope itself, while streaming compilation,
because it's unknown that which "var" binding is actually "function", until hitting function declaration which may appear very later.

We should either:
  * gradually create Scope object, and emit bytecode with dummy slot offset,
    and fixup bytecode's slot offset once offset is known
  * re-design Scope object to allow "var" and "functions" mixed in the single range

the distinction between let and const is going to be handled in the spec and we can use it.
(note. we still need a way to know which "let" binding is class, for bug 1428672)

Comment 1

[Tooru Fujisawa [:arai]]

The ultimate consumer of it is only BytecodeEmitter::EmitterScope::{enterGlobal,enterEval}, which checks the range by BindingIter::isTopLevelFunction and avoid emitting DEFVAR for top level function, which we will emit DEFFUN into prologue here, when hitting function node:
https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/js/src/frontend/BytecodeEmitter.cpp#8108-8110

so, it's specific to frontend and once the bytecode is generated, that information is no more necessary,
which means it's better removing function range from Scope data (that reduces the size of Scope data by 4bytes).

we could store the function vs var info separately and handle it in enterGlobal and enterEval.

Comment 2

[Tooru Fujisawa [:arai]]

oops, I overlooked WrappedPtrOperations<BindingIter, Wrapper> :P

which is used by CheckGlobalDeclarationConflicts and CheckEvalDeclarationConflicts that is used at runtime.
so, turns out we should store the information in Scope.

Comment 3

[Tooru Fujisawa [:arai]]

I'll use BindingName's tag to represent top-level function,
which can be reused in bug 1428672 for let vs class.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f23baad8e5d686c7fe173d5a43c127ff0cfc0a0

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Use BindingName tag to distinguish between var and top-level function, instead of offset range.

* Merged "top-level funcs" and "vars" ranges in:
   * BindingIter
   * GlobalScope::Data
   * EvalScope::Data
 * Added TopLevelFunctionFlag tag to BindingName, which is set for top-level function
   inside "vars" range
 * Changed BindingIter::isTopLevelFunction to check the tag,
   with an extra asswertion that the binging is in "vars" range if it's true
   (when we reuse the tag bit for other range (let vs class), we should check the kind as well, instead of just an assertion)

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> oops, I overlooked WrappedPtrOperations<BindingIter, Wrapper> :P
> 
> which is used by CheckGlobalDeclarationConflicts and
> CheckEvalDeclarationConflicts that is used at runtime.
> so, turns out we should store the information in Scope.

Yes, exactly. If a distinction wasn't necessary beyond the frontend, it should definitely be removed from the Scope binding stuff.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8984026 [details] [diff] [review]
Use BindingName tag to distinguish between var and top-level function, instead of offset range.

Review of attachment 8984026 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.

I'm mainly worried about the scalability and maintainability of this scheme going forward -- that some binding kinds are determined by sorting order, and some are by a tag, not to mention running out of tag bits to use.

I've mostly convinced myself that this can be organized more sensibly by having two tiers of binding kinds:

1. Sorting order determines the main kind (parameter, var, let, or const)
2. Bits determine the subkind, which may affect semantics but in some lesser way, like errors (e.g. body-level functions, classes in the future). What these bits mean is determined by the main kind.

I wonder if this can be reflected in code somehow.

::: js/src/vm/Scope.h
@@ +134,5 @@
>      bool closedOver() const {
>          return bits_ & ClosedOverFlag;
>      }
>  
> +    bool isTopLevelFunction() const {

Given that the result of isTopLevelFunction is only meaningful if the BindingName happens to be a var, I'd like this to be made private, and a friend declaration added for BindingIter.

@@ +757,5 @@
>      // Parser<FullParseHandler>::newGlobalScopeData.
>      struct Data
>      {
>          // Bindings are sorted by kind.
> +        // `vars` includes top-level functions.

I'd add comments, here and below, that top-level functions are distinguished by a bit on the BindingName instead of by sorted ranges.

Comment 7

[Shu-yu Guo [:shu]]

As a heads up, the parameter logic around positional parameters with and without parameter expressions will probably need refactoring for binary AST as well.

The semantics is that if a parameter list has parameter expressions (e.g. destructuring, default value), then parameters are lexical bindings and have TDZ, etc. If a parameter list doesn't have expressions, it's considered "simple", and parameters are accessed via arg slots. I've added "isSimpleParameterList" to AssertedParameterScopes for this reason.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dee28e17417eb3fcb043027393abc7b4f506d78
Bug 1467052 - Use BindingName tag to distinguish between var and top-level function, instead of offset range. r=shu

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3dee28e17417