Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for completion value of function / class declaration statements #1012

Merged
merged 1 commit into from
May 4, 2017

Conversation

JosephPecoraro
Copy link
Contributor

Check completion value of more declarations are empty:

eval(`1; function f(){}`) === 1
eval(`1; function* f(){}`) === 1
eval(`1; async function f(){}`) === 1
eval(`1; class C {}`) === 1

Browsers:

  • Firefox passes all
  • Chrome fails for class - bug
  • WebKit fails for class - bug
  • Edge fails for class - bug

@littledan
Copy link
Member

Thanks for the contribution! Keep them coming.

It looks from the specification that the completion value of class declarations is the constructor, rather than empty. Because of this, I think Firefox is the engine with a bug, rather than the other three.

@anba
Copy link
Contributor

anba commented Apr 29, 2017

It looks from the specification that the completion value of class declarations is the constructor, rather than empty.

No, an empty completion value is correct -> https://tc39.github.io/ecma262/#sec-class-definitions-runtime-semantics-evaluation.

@littledan
Copy link
Member

Oh sorry, I was looking at ClassExpression.

Copy link
Member

@leobalter leobalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are looking good for me. I appreciate you're following the file naming pretty well.

There's a small detail to be fixed in the frontmatter (see the other comments).

You may also add a test for async generator declaration (adding the features: [async-iteration] frontmatter tag, or we can work on that as a follow up.

// Copyright (C) 2017 Apple Inc. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
es6id: 14.6.13
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use esid instead of es6id, please:

esid: sec-async-function-definitions-runtime-semantics-evaluation

// Copyright (C) 2017 Apple Inc. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
es6id: 14.5.15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use esid instead of es6id, please:

esid: sec-class-definitions-runtime-semantics-evaluation

// Copyright (C) 2017 Apple Inc. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
es6id: 14.1.21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use esid instead of es6id, please:

esid: sec-function-definitions-runtime-semantics-evaluation

// Copyright (C) 2017 Apple Inc. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
es6id: 13.1.8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use esid instead of es6id, please:

esid: sec-statement-semantics-runtime-semantics-evaluation

@rwaldron rwaldron added this to Needs Review in Q2 Bocoup & Google May 3, 2017
@JosephPecoraro
Copy link
Contributor Author

You may also add a test for async generator declaration (adding the features: [async-iteration] frontmatter tag, or we can work on that as a follow up.

I would rather someone address this in a follow-up. Its not in the current ES spec, and it looks like it does not behave like other declarations, and instead returns the function instead of an empty completion like all of these declarations. See https://tc39.github.io/proposal-async-iteration/#prod-AsyncGeneratorDeclaration

AsyncGeneratorDeclaration : async [no LineTerminator here] function* BindingIdentifier ( FormalParameters ) { AsyncGeneratorBody }

...
3. Let F be ! AsyncGeneratorFunctionCreate(Normal, FormalParameters, AsyncGeneratorBody, scope, strict).
...
7. Return F.

@leobalter
Copy link
Member

I would rather someone address this in a follow-up

This is fine.

Its not in the current ES spec, ...

This is tricky. Test262 capture tests not only for the current spec draft, but also stage 3 proposals. The motivation is allowing runtimes to implement correct features before Stage 4.

Anyway, that doesn't make any requirement to land everything together, but it opens an reminder we should cover async generators as well. I'll create an issue to keep track.

Thanks for the tests!

@leobalter leobalter merged commit 19eb1d2 into tc39:master May 4, 2017
@littledan
Copy link
Member

littledan commented May 4, 2017

@JosephPecoraro I see that text, but it looks like it's under the heading Runtime Semantics: InstantiateFunctionObject.

If you look at the 8th bullet of this section, it mentions adding a clause for evaluation analogous to GeneratorDeclaration, which evaluates to NormalCompletion(empty). So I believe async generators should have an empty completion value just like other types of declarations.

Anyway, putting this test in a follow-on patch sounds good to me.

EDIT: I was unclear on the specification at first; the above comment is more relevant.

@JosephPecoraro
Copy link
Contributor Author

Test262 capture tests not only for the current spec draft, but also stage 3 proposals.

I didn't realize that. Thanks for the clarification.

If you look at the 8th bullet of this section, it mentions adding a clause for evaluation analogous to GeneratorDeclaration, which evaluates to NormalCompletion(empty). So I believe async generators should have an empty completion value just like other types of declarations.

Ahh, I see. Thanks.

@rwaldron rwaldron moved this from Needs Review to Done in Q2 Bocoup & Google May 8, 2017
@JosephPecoraro JosephPecoraro deleted the cptn-decl branch June 1, 2017 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Q2 Bocoup & Google
Done (Authored, Reviewed, etc)
Development

Successfully merging this pull request may close these issues.

None yet

4 participants