ES6 classes. #1048
ES6 classes. #1048
Conversation
Conflicts: tests/stable/unit/parser.js Fixed by keeping both tests.
There is some overlap with reserved var checking, so I'm not sure how useful the 'eval' and 'arguments' checks are.
This https://github.com/jshint/jshint/pull/1048/files#L1R3256 looks right to me :) I haven't tested it yet, but the strategy and tests look solid. Also, this is super exciting :) For everyone watching from home... The static semantics 13.5 @usrbincc refers to are these lines: ClassDeclaration : class BindingIdentifier ClassTail Which just means that BindingIdentifier cannot be "eval" or "arguments", eg. the tests that @usrbincc wrote here: https://github.com/jshint/jshint/pull/1048/files#L3R3134 |
| var run = TestRun(test) | ||
| .addError(1, "Expected an identifier and instead saw 'eval' (a reserved word).") | ||
| .addError(1, "A class cannot be named 'eval'.") | ||
| .addError(2, "Expected an identifier and instead saw 'arguments' (a reserved word).") |
There was a problem hiding this comment.
The only issue I can see here is that the error isn't quite right, the names eval and arguments are prohibited for the same reason they are prohibited in strict mode. Looking at the spec, it seems that Allen just extended the implicit strict mode static semantics of ClassBody all the way out to ClassDeclaration and ClassExpression.
non-strict mode:
function eval() {} // no errorstrict mode:
"use strict";
function eval() {} // syntax errornon-strict mode:
class eval {} // syntax errorstrict mode:
"use strict";
class eval {} // syntax errorI'm not really sure what the error message should be; since the ClassDeclaration and ClassExpression aren't necessarily in strict mode themselves—even though their ClassBody is.
cc @allenwb
Allen, do you have any thoughts on the language that should be used to describe these kinds of syntax errors?
There was a problem hiding this comment.
Actually, I' not sure that the non-strict mode restriction on class names should be specified. When you consider that
"don't use strict";
let eval = class {};
is legal, then for consistency
"don't use strict";
class eval {};
should also be legal. I think I'll change the spec. draft such that eval and arguments are restricted as class names only if the class definition appears in strict code.
Regarding the message, "eval" and "arguments" shouldn't be called "reserved words" as they don't actually belong to that lexical category. You might call them "restricted identifiers" but the best linting message for this case is probably "A class defined in strict mode cannot be named 'eval' (or 'arguments')".
Allen
On May 2, 2013, at 7:12 AM, Rick Waldron wrote:
In tests/stable/unit/parser.js:
+};
+
+exports["class and method naming"] = function (test) {
- var code = [
"class eval {}","class arguments {}","class C {"," get constructor() {}"," set constructor(x) {}"," prototype() {}","}"- ];
- var run = TestRun(test)
.addError(1, "Expected an identifier and instead saw 'eval' (a reserved word).").addError(1, "A class cannot be named 'eval'.").addError(2, "Expected an identifier and instead saw 'arguments' (a reserved word).")function eval() {} // no error
"use strict";
function eval() {} // syntax error
class eval {} // syntax error
"use strict";
class eval {} // syntax error
I'm not really sure what the error message should be; since the ClassDeclaration and ClassExpression aren't necessarily in strict mode themselves—even though their ClassBody is.cc @allenwb
Allen, do you have any thoughts on the language that should be used to describe these kinds of syntax errors?
—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
I think I'll change the spec. draft such that eval and arguments are restricted as class names only if the class definition appears in strict code.
There was a problem hiding this comment.
Regarding the message, "eval" and "arguments" shouldn't be called "reserved words" as they don't actually belong to that lexical category.
Agreed. (The "(a reserved word)" error messages happen even without this patch, btw.) JSHint's reserved word and var handling for strict and non-strict modes definitely needs some tweaking in this regard.
I'm thinking (optimistically) that it might just need a few simple changes localized to the reservevar and W024 areas.
You might call them "restricted identifiers" but the best linting message for this case is probably "A class defined in strict mode cannot be named 'eval' (or 'arguments')".
More specific error messages would definitely be nice, in general. Currently JSHint has the same generic error message for var, let, function params, and so on. Passing in an optional context argument to identifier() would probably work with minimal source changes.
Of course, there's only one way to be sure just how messy this solution ultimately turns out to be.
How about extends? Here is my current understanding:
"use strict";
// I'm thinking this is legal, if strange, but just to make sure.
var A = class extends eval {};
// syntax error
var B = class extends public {};I just realized that extends takes an AssignmentExpression, not a BindingIdentifier. Now that's prototype-based languages for you. I'll fix this first.
@rwldrn I know intellectually that the spec does change, and that there are people behind each change, but this is the first time I've seen it happen right before me. Very neat.
There was a problem hiding this comment.
@usrbincc :) thanks for making this happen!!
Examples:
class C extends func() {}
class C extends arr[0] {}
class C extends "str" {}
This isn't needed if we adjust the code in `expression()` to only warn about future reserved words if it really is a label.
- Make the class declaration and class expression tests read more parallel.
|
Okay, looks like there are at least four categories of words, and of course, several categories of places where they can be (illegally) bound -- cat > reserved.js <<"END"
// jshint maxerr: 640
// missing semicolons to make it easier to keep track of which errors go where.
(function() {
// strict-only keywords.
var implements, interface, package, private, protected, public, static
// always keywords
var class, enum, export, extends, import, super
var function, new, void, delete
// strict-only restricted ids
var arguments, eval
// always restricted ids
var false, Infinity, null, this, true, undefined
}());
(function() {
"use strict";
// strict-only keywords.
var implements, interface, package, private, protected, public, static
// always keywords
var class, enum, export, extends, import, super
var function, new, void, delete
// strict-only restricted ids
var arguments, eval
// always restricted ids
var false, Infinity, null, this, true, undefined;
}());
END
# currently everything gets the same generic error message.
./bin/jshint reserved.jsI did a reduced version of this before I realized how many categories needed to be handled. I'll have another go at it, though it might be too big to tastefully shoehorn into this pull.
But a consequence of this is that it's harder to report errors for things like This might still be possible, but it requires some more fundamental changes to the handling of restricted words. It's somewhat related to the change discussed in the previous section. In terms of ES6, it looks like jshint is still missing template literals and modules (of course, since they're still in flux). Possibly extended regexps too? Octal literals? Unicode strings? Probably some other things. The spec is scary big. That's the trouble with extending syntax; the old tools need to be upgraded to work. Same thing goes for lint tools, macro transformers, browsers (simple polyfills no longer feasible), etc Though in the other direction lies |
|
My apologies for bitrotting this patch. No excuses besides the usual "too many projects too little time". I skimmed over the actual code and it looks fine. Given the pretty good test coverage and no regressions I think it's good to go. Thanks! |
|
Ah, shit, I think this patch introduced some lint errors. I'll fix them shortly in a followup patch, no biggie. |
|
Lint errors fixed in f25c27e. |
|
Oops, sorry about the lint errors. I was getting the #1052 "TypeError: Cannot read property 'use strict' of undefined" crash running It turns out that
I know the feeling, though with me, the projects usually aren't making much progress, so they don't work too well as excuses. Batch processing is an efficient way to process things (trading off latency for throughput), so lag in processing pulls is understandable. All the more reason for everyone to have lots of projects — ensuring maximum CPU utilization even if a thread gets blocked by a system call. (Okay, I may be mixing metaphors slightly here (batch + threads), but it'd get a little too involved to talk about adjusting time-slices, quantums, etc.) I just checked, and it looks like this pull would have merged without conflicts, so it doesn't seem to have bitrotted too much. |
Support for ES6 class syntax.
I've been meaning to learn about Pratt parsers, so this was a good excuse (#1024). Much of the heavy lifting was done by reusing the object literal code (slightly modified). I'm not good at writing tests, so thanks to rwldrn for providing the source material. Wait, did I do anything at all? Unfortunately, my secretary was out, so I had to do the typing myself.
I added some of the static semantic checks, but to be honest, I don't understand most of what the spec is saying, so I'm almost certain that I misinterpreted the part where it says:
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-13.5
I mean, how can a
BindingIdentifier"contain" anything?I'm almost certainly missing some important static semantic checks. I'm hoping someone better versed in the spec can spot the holes.
I followed the grammar pretty much verbatim, so hopefully everything that should parse correctly, does. And vice versa.