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

Allow use of ES3 FutureReservedWord in dot position with es3:true #2231

Closed
Krinkle opened this issue Mar 3, 2015 · 12 comments
Closed

Allow use of ES3 FutureReservedWord in dot position with es3:true #2231

Krinkle opened this issue Mar 3, 2015 · 12 comments

Comments

@Krinkle
Copy link

Krinkle commented Mar 3, 2015

This re-opens #674. (Regression.)

It seems code using obj.static no longer passes JSHint with es3: true. As discussed at #674 and as was fixed once I believe, FutureReservedWords are allowed in ES3 engines. The spec wasn't too clear on them, but actual implementations were.

It seems JSHint source code still reads as if this works, but in practice it is actually failing:

jshint/src/jshint.js

Lines 136 to 142 in 96a97d0

function isReserved(token) {
if (!token.reserved) {
return false;
}
var meta = token.meta;
if (meta && meta.isFutureReservedWord && state.option.inES5()) {

  function isReserved(token) {
    if (!token.reserved) {
      return false;
    }
    var meta = token.meta;

    if (meta && meta.isFutureReservedWord && state.option.inES5()) {

However the following code still produces warnings on JSHint 2.6.1 as well as on jshint.com:

/*jshint es3: true */
var obj = {};
obj.static = 1; // Expected an identifier and instead saw 'static' (a reserved word).
obj.throws = 2;
@nicolo-ribaudo
Copy link
Contributor

The regression was introduced in 9782fc8

@lukeapage
Copy link
Member

Added Needs Discussion since it was done on purpose - it wasn't a straight forward regression.

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2015

I'm having the same problem with t['throws'] triggering an error when es3 is true - t.throws is a syntax error in older IE.

@Krinkle
Copy link
Author

Krinkle commented Jul 22, 2015

@ljharb No, it isn't a syntax error in older IE. See also #674, #674 (comment), and qunitjs/qunit#267 (comment).

http://codepen.io/Krinkle/debug/KpxGRR works fine in WinXP/IE6.

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2015

hm, so you're saying that .throws always works in older browsers? I was pretty certain there was at least one browser it failed in - I'll have to track down which one.

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2015

#1000 points out that class has this problem (which I verified in IE 8), so some FutureReservedWords definitely break. It'd be useful to verify exactly which ones work and which don't. It seems import, enum, and a few others also break.

For safety, I still think it would be a good idea when es3 is "true" to require (or at least allow) FutureReservedWords to be quoted.

FutureReservedWords, for reference, from http://www.ecma-international.org/publications/files/ECMA-ST-ARCH/ECMA-262,%203rd%20edition,%20December%201999.pdf:

abstract enum int boolean export interface byte extends long char
final native class float package const goto private debugger
implements protected double import public short static super
synchronized throws transient volatile

@WebReflection
Copy link

if I might add something to this discussion, iOS5 which never made it to 6 and others might silently die using .super so if there's a list of words that should always be wrapped, that might be one.

@jdforrester
Copy link

if I might add something to this discussion, iOS5 which never made it to 6 and others might silently die using .super so if there's a list of words that should always be wrapped, that might be one.

Do you have a link that discusses which versions of iOS will syntax-error on using which future reserved words?

@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2015

@jdforrester I think we should be clear though - if a keyword in dot notation syntax errors on any version of any browser, then it seems to me like jshint absolutely needs a way to cover that case - since the purpose of a linter is to prevent runtime errors at compile time.

@ljharb
Copy link
Contributor

ljharb commented Dec 27, 2015

fwiw, the related eslint issue is here.

@Krinkle Krinkle closed this as completed Mar 16, 2017
@ljharb
Copy link
Contributor

ljharb commented Mar 16, 2017

@Krinkle why close this? it's still an issue.

@Krinkle
Copy link
Author

Krinkle commented Mar 16, 2017

@ljharb Mainly because active projects I'm involved with no longer require ES3 syntax compatibility. And either way, the majority was migrated to use ESLint instead of JSHint+JSCS. Lower priority bugs should generally remain open, but there is a certain point where the priority is so low it is very unlikely to get resolved quicker than the bug itself becoming obsolete (e.g. JSHint 3.x dropping support for ES3 or something like that, or, JSHint merging with ESLint).

Most likely anyone this bug will ever apply to has already worked around it. And that group will only keep getting smaller.

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

No branches or pull requests

7 participants