1298640 - test262 regressions caused by the frontend rewrite

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

First: Big congrats on landing, this actually decreased our test262 failures from 3352 to 2881.

There seem to be only 4 tests that regressed by landing that huge patch:

language/eval-code/direct/non-definable-function-with-variable.js
language/eval-code/indirect/non-definable-function-with-variable.js
language/eval-code/indirect/non-definable-function-with-variable.js
language/statements/try/scope-catch-block-lex-open.js
language/statements/try/scope-catch-block-lex-open.js

Comment 1

[Shu-yu Guo [:shu]]

(In reply to Tom Schuster [:evilpie] from comment #0)
> First: Big congrats on landing, this actually decreased our test262 failures
> from 3352 to 2881.
> 
> There seem to be only 4 tests that regressed by landing that huge patch:
> 
> language/eval-code/direct/non-definable-function-with-variable.js
> language/eval-code/indirect/non-definable-function-with-variable.js
> language/eval-code/indirect/non-definable-function-with-variable.js
> language/statements/try/scope-catch-block-lex-open.js
> language/statements/try/scope-catch-block-lex-open.js

Thanks for running test262 and tracking the regressions. I see 2 dupes in that list, so I guess it's just the 3?

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Track top-level functions in eval/global bodies for all-or-nothing redeclaration checks.

This language is the worst.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Always give catch bodies their own lexical scope.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8785638 [details] [diff] [review]
Always give catch bodies their own lexical scope.

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

Feh, I maybe could have seen this.

::: js/src/frontend/Parser.cpp
@@ +205,5 @@
>          return;
>  
> +    for (DeclaredNameMap::Range r = catchParamScope.declared_->all(); !r.empty(); r.popFront()) {
> +        DeclaredNamePtr p = declared_->lookup(r.front().key());
> +        MOZ_ASSERT(p && DeclarationKindIsCatchParameter(r.front().value()->kind()));

Separate asserts for &&.

::: js/src/frontend/Parser.h
@@ +153,5 @@
>          // Remove all VarForAnnexBLexicalFunction declarations of a certain
>          // name from all scopes in pc's scope stack.
>          static void removeVarForAnnexBLexicalFunction(ParseContext* pc, JSAtom* name);
>  
> +        // And and remove a catch parameter names. Used to implement the odd

s/a catch/catch/

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8785636 [details] [diff] [review]
Track top-level functions in eval/global bodies for all-or-nothing redeclaration checks.

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

Either I'm misreading this, or it has some basic-ish bugs in it.  Probably the former; tell me how I'm wrong?

::: js/src/frontend/Parser.cpp
@@ -1384,5 @@
>          BindingName* start = bindings->names;
>          BindingName* cursor = start;
>  
> -        // Keep track of what vars are functions. This is only used in BCE to omit
> -        // superfluous DEFVARs.

lol :-(

::: js/src/vm/EnvironmentObject.cpp
@@ +3194,5 @@
> +    // ES 8.1.14.16 CanDeclareGlobalFunction.
> +
> +    // Step 4.
> +    if (!desc.object())
> +        return true;

This assumes that globals are always extensible, correct?  Is that actually true?  I know it's true in the browser, but I can perfectly well call |Object.preventExtensions(this)| in the shell and |Object.isExtensible| claims it was respected.  So I think you need to return IsExtensible(cx, global) here.

@@ +3198,5 @@
> +        return true;
> +
> +    // Step 6.
> +    if (desc.hasConfigurable() && desc.configurable())
> +        return true;

I believe it's the case that descriptors the engine *hands out* are complete.  So just assert hasConfigurable().

@@ +3201,5 @@
> +    if (desc.hasConfigurable() && desc.configurable())
> +        return true;
> +
> +    // Step 7.
> +    if (!desc.isAccessorDescriptor() &&

Why not isDataDescriptor?

@@ +3203,5 @@
> +
> +    // Step 7.
> +    if (!desc.isAccessorDescriptor() &&
> +        desc.hasWritable() && desc.writable() &&
> +        desc.hasEnumerable() && desc.enumerable())

The has-checks aren't needed once isDataDescriptor's verified, either.

@@ +3325,5 @@
> +    if (varObj->is<GlobalObject>()) {
> +        Handle<GlobalObject*> global = varObj.as<GlobalObject>();
> +        RootedPropertyName name(cx);
> +        for (Rooted<BindingIter> bi(cx, BindingIter(script)); bi; bi++) {
> +            if (bi.isTopLevelFunction()) {

Maybe I'm reading this too quickly, but why doesn't this fail the |kind() == Var| assert in that function?  Looks like it would if the global had any global let/const declarations.  If so, please fix (I guess by adding a should-continue test that aborts when the first non-top-level function is reached?  seeing as there are no imports/formals, so top-level functions come first up until the first genunine var) and add a test -- if not, explain how I are dumb.

::: js/src/vm/Interpreter.cpp
@@ +4391,5 @@
>          if (shape->configurable()) {
>              if (!DefineProperty(cx, parent, name, rval, nullptr, nullptr, attrs))
>                  return false;
>          } else {
> +            MOZ_ASSERT(!shape->isAccessorDescriptor() && shape->writable() && shape->enumerable());

Three separate assertions, please.

::: js/src/vm/Scope.h
@@ +1134,5 @@
>  
> +    bool isTopLevelFunction() const {
> +        MOZ_ASSERT(!done());
> +        MOZ_ASSERT(kind() == BindingKind::Var);
> +        return index_ >= topLevelFunctionStart_ && index_ < varStart_;

You could have one fewer test/mathop

  return index < varStart_;

and

  MOZ_ASSERT(index_ >= topLevelFunctionStart_)

because |kind() == BindingKind::Var| assured that.  I'd do that -- better to learn early if things were to go awry than to test more than necessary and cover up some mistake.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Track top-level functions in eval/global bodies for all-or-nothing redeclaration checks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1194f91ff50
Always give catch bodies their own lexical scope. (r=Waldo)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/701075b5e63c
Track top-level functions in eval/global bodies for all-or-nothing redeclaration checks. (r=Waldo)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/701075b5e63c