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

Disabling warning for W100 not working in 2.9.3 #3013

Closed
damyanpetev opened this Issue Aug 19, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@damyanpetev
Copy link

damyanpetev commented Aug 19, 2016

See IgniteUI/ignite-ui#243:

First build passed with 2.9.2, consecutive builds with 2.9.3 update fail, with a line wrapped with /*jshint -W100 */

Don't see anything related in the update blog, but has something changed in the way those are handled?

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Aug 19, 2016

Thanks for the report! This is indeed a regression.

Not that it helps you much, but it looks like the new release has surfaced a previously-existing bug--in the process of reducing the input you shared, I was able to experience this problem in version 2.9.2, as well.

I need to step away for a bit, but I hope to have a patch ready this evening (EST).

@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Aug 20, 2016

Here's the story:

Upon encountering an open parenthesis character (() in an expression
position, a ES2015-aware JavaScript parser like JSHint's has to determine
whether it marks the beginning of a grouping operator or an arrow function.

JSHint does this by tokenizing all the input that follows until it finds a
"matching" close parenthesis character ()). At that point, it can make a
determination about the nature of the production in question--if it's followed
by the => token, it's an arrow function; otherwise, it's a grouping operator.

While it is looking ahead, it does not apply in-line directives it encounters
such as -W100. However, the lexer is responsible for emitting certain linting
errors including W100. So as soon as JSHint begins "looking ahead", its
behavior in response to these potentially-dangerous characters is "locked in."
(That suggests a short-term fix for anyone suffering from this regression: disable
W100 before the IIFE. Not ideal, I know, but note that the relevance of that
warning is disputed
.)

This regression came about because of a seemingly-unrelated patch: gh-3003.
Previously, the lookahead would be cancelled if a semicolon token were
encountered, as in:

(function() {
;//<-- lookahead ends at this semicolon token
}());

...however, that was found to be an invalid heuristic because arrow function
parameters may themselves contain that token:

(x = function() {
;//<-- lookahead should *not* end at this semicolon token
}) => x;

So we removed it in order to properly recognize arrow functions in those cases.

With that in mind, the fully-reduced test case may be a little more clear (that
is an unprintable \u200f character in the string literal):

(function() {
    ;

    /*jshint -W100 */
    "";
})();

Prior to gh-3003 (e.g. JSHint at version 2.9.2), the semicolon would stop the
lookahead, and the unprintable character would not be lexed until after the
JSHint directive was applied. With the fix applied, JSHint lexes the character
before applying the directive and proceeds to warn.

This also demonstrates how the fundamental problem exists even in JSHint 2.9.2.
The following input incorrectly triggers a warning even in that version of
JSHint:

(function() {
    /*jshint -W100 */
    "";
})();

Because here, the parenthesis-triggered lookahead is not interrupted in either
version.

The ideal fix would be to apply directives during lookahead. Unfortunately, due
to the highly flexible nature of JSHint's in-line directives, this is not
possible. Users expect that directives have "function scope," meaning the lexer
would need to be aware of the surrounding semantics of the tokens it produces.

I've submitted a fix here: gh-3016. In the long term, I think we ought to consider
other improvements to the lookahead heuristics--without the (invalid) semicolon
check, the common practice of wrapping entire programs in an IIFE will cause
JSHint to initially lex all input.

@damyanpetev

This comment has been minimized.

Copy link
Author

damyanpetev commented Aug 22, 2016

Thanks for the detailed explanation!
I see how tricky this can be without adding an immense amount of extra work for scopes.
We'll stick to 2.9.2 for a while as it works for our files and will look into breaking down the problematic parts into blocks we can manage in the future :)

WMDE-Fisch pushed a commit to wmde/DeepCat-Gadget that referenced this issue Aug 24, 2016

WMDE-Fisch

WMDE-Fisch pushed a commit to wmde/DeepCat-Gadget that referenced this issue Aug 24, 2016

WMDE-Fisch

rwaldron added a commit that referenced this issue Sep 22, 2016

Fix for gh-3013 (#3016)
* [[CHORE]] Remove `lex.start` method

This method simply proxies `lex.nextLine` to scan a single line of the
program. It is currently only used to scan the very first line of input,
which is problematic for two reasons:

1. Linting options have not yet been applied via the `assume` function,
   so linting options that effect the parser's behavior are not honored
   for this first line (see the unit test corrected in this patch for an
   example)
2. So-called "checks" for asynchronously-reported warnings on the first
   line cannot be associated with a token. This makes it impossible to
   issue warnings asynchronously for the first line.

Because the `lex.token` method itself invokes `lex.nextLine`, these
problems may be avoided by removing `lex.start` and deferring the
invocation of `nextLine` until the first token is requested.

* [[FIX]] Allow W100 to be ignored during lookahead

Defer the reporting of warning W100 until parse time (using
`triggerAsync`) so that it may be disabled via in-line directives even
in contexts where a "lookahead" is taking place.

* fixup! [[FIX]] Allow W100 to be ignored during lookahead

jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 18, 2016

Fix for jshintgh-3013 (jshint#3016)
* [[CHORE]] Remove `lex.start` method

This method simply proxies `lex.nextLine` to scan a single line of the
program. It is currently only used to scan the very first line of input,
which is problematic for two reasons:

1. Linting options have not yet been applied via the `assume` function,
   so linting options that effect the parser's behavior are not honored
   for this first line (see the unit test corrected in this patch for an
   example)
2. So-called "checks" for asynchronously-reported warnings on the first
   line cannot be associated with a token. This makes it impossible to
   issue warnings asynchronously for the first line.

Because the `lex.token` method itself invokes `lex.nextLine`, these
problems may be avoided by removing `lex.start` and deferring the
invocation of `nextLine` until the first token is requested.

* [[FIX]] Allow W100 to be ignored during lookahead

Defer the reporting of warning W100 until parse time (using
`triggerAsync`) so that it may be disabled via in-line directives even
in contexts where a "lookahead" is taking place.

* fixup! [[FIX]] Allow W100 to be ignored during lookahead
@jugglinmike

This comment has been minimized.

Copy link
Member

jugglinmike commented Oct 20, 2016

The fix for this bug is now available in the newly-published JSHint version 2.9.4:

http://jshint.com/blog/2016-10-20/release-2-9-4/

Please let us know if you're still having trouble with that version!

WMDE-Fisch pushed a commit to wmde/DeepCat-Gadget that referenced this issue Oct 24, 2016

WMDE-Fisch
use newest jshint version again
The bug with -W100 was fixed in 2.9.4. See
jshint/jshint#3013 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment