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

Stage 3 reviews #7

Closed
2 tasks done
tschneidereit opened this issue Dec 19, 2017 · 13 comments
Closed
2 tasks done

Stage 3 reviews #7

tschneidereit opened this issue Dec 19, 2017 · 13 comments

Comments

@tschneidereit
Copy link
Member

tschneidereit commented Dec 19, 2017

At the last f2f, @keithamus and I agreed to review the proposal for stage 3. Opening this issue to track the reviews.

@tschneidereit
Copy link
Member Author

The spec changes look good to me, no changes requested.

@keithamus
Copy link
Member

Reading through the specification, I'm happy. I have a clarifying question around throwing multiple times (it looks possible to do something like 0 || throw throw throw throw 0) but I'm pretty sure the runtime semantics mean this is a non-issue.

@ljharb
Copy link
Member

ljharb commented Dec 19, 2017

We certainly should ensure that throw throw can never appear; it would make sense that multiple throws would just end up throwing the inner value, but allowing that to be valid in the grammar seems a bit nonsensical to me.

I'm not that sharp on spec syntax grammar; does it allow this currently?

@keithamus
Copy link
Member

As far as I can see it does; as throw is a UnaryExpression which allows a UnaryExpression as its operand. At first I thought the lookahead token for ExpressionStatement may disallow this but I'm not sure which is why I commented. I'm sure that the semantics indicate multiple throws become a no-op. Certainly other duplicate expressions are allowed like typeof typeof {}, delete delete x.y, and void void 0, so maybe throw throw isn't that much of a problem?

@ljharb
Copy link
Member

ljharb commented Dec 19, 2017

typeof typeof makes sense rationally; delete delete doesn't make sense to me but is probably a legacy artifact of how delete behaves; void void also makes sense to me as a value coercion operator. But throw doesn't coerce one value to another; it's an abrupt completion. We don't allow return return either, and throw is most like return, expression context or no.

@rbuckton
Copy link
Collaborator

As mentioned, throw throw x isn't any different than typeof typeof x, void void x or delete delete x and arbitrarily restricting it will just add confusion. Consider a case where a developer concatenates multiple strings to then pass to eval to evaluate an expression dynamically. If we arbitrarily disallow throw throw x, then they have to special case that specific scenario compared to other unary operators.

Even if we created a grammar restriction, what about throw (throw x) or throw (0, throw x)? A grammar restriction here only prevents a single case.

We don't allow return return purely because return is not an expression (at least, not yet anyways). I imagine that return return would also be allowed for the above reasons if return were to have an expression form.

If you want to disallow throw throw x, a lint rule would be ideal. Linter's can enforce code quality on the code you author, but don't have a downstream effect on scenarios like eval.

@ljharb
Copy link
Member

ljharb commented Dec 20, 2017

It's a fair point that it would make things more difficult for users of eval, but I don't think that the ergonomics of eval use is something anyone on the committee (or realistically, anyone in the community) tends to care about.

I think return return would also be problematic to allow for the same reasons.

However, since a grammar restriction wouldn't be sufficient (beyond simply blocking just throw throw, altho I think that would still be beneficial alone), and the only other action that the spec could take is "throw an exception", which it was in the process of doing anyways, it does seem like the only thing that is even possible for us to do in a practical sense is "block throw throw in the grammar", which probably wouldn't be compelling enough to complicate the spec with. I'd still like to hear the committee's thoughts on it; if return is ever to become an expression, the same decision would affect it too.

@ljharb
Copy link
Member

ljharb commented Dec 20, 2017

(Regardless, throw is very different from typeof, void, delete and in fact every other unary operator, conceptually, even if grammatically they'd be treated the same)

@rbuckton
Copy link
Collaborator

throw is very different from typeof, void, delete

I disagree, at least in part. delete can result in an abrupt completion just as throw will, though throw is more obvious:

> var p = new Proxy({}, { deleteProperty() { throw new Error(); } }); 
> delete delete p.a;
Error
    at Object.deleteProperty (repl:1:50)
    at repl:1:83
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:433:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)
    at REPLServer.Interface._onLine (readline.js:278:10)

Also, the runtime semantics of throw throw x is no different than throw x() if x() itself throws an exception.

That said, I am fine with bringing this up in committee. Did you find any other concerns with the spec text?

@ljharb
Copy link
Member

ljharb commented Dec 20, 2017

"can result in an abrupt completion" isn't the same as "is designed to solely produce an abrupt completion" :-)

The rest of the spec text looks straightforward.

@keithamus
Copy link
Member

Did you find any other concerns with the spec text?

No other concerns from me.

@rbuckton
Copy link
Collaborator

rbuckton commented Jan 7, 2018

@bterlson, I believe Stage 3 also requires a review by the current ECMAScript editor. Can you take a look?

@rbuckton
Copy link
Collaborator

I've created #9 for the open question about throw throw x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants