Fix for gh-3013 #3016

jugglinmike · 2016-08-20T18:35:07Z

@rwaldron This is a fix for gh-3013 in two parts. You can see my response to
that issue for more detail about the problem. Please review carefully because
I'm messing with some relatively fundamental behavior here.

One specific question I have for you: I do not think it's possible to test for
this behavior at every call site of nextLine. (The situation we're interested
in here is when the lexer is tokenizing input and may pass over a -W100
directive.) I've still updated the call sites for consistency, and in case some
future change can use checks from different call sites. Does that seem
right to you?

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.

coveralls · 2016-08-20T18:37:21Z

Coverage decreased (-0.0006%) to 97.718% when pulling 7b93cc5 on jugglinmike:gh-3013 into f00091c on jshint:master.

jugglinmike · 2016-08-20T18:41:51Z

@rwaldron Also note that a much simpler fix would be to remove W100 altogether--I still don't know exactly what that warning is for. Check out gh-2741 for an unresolved discussion along these lines.

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.

coveralls · 2016-08-20T20:53:07Z

Coverage decreased (-0.0006%) to 97.718% when pulling 4580e9e on jugglinmike:gh-3013 into f00091c on jshint:master.

jugglinmike · 2016-08-27T23:16:47Z

This needs work--W101 is also suffering the same problem.

coveralls · 2016-09-04T20:49:52Z

Coverage decreased (-0.07%) to 97.65% when pulling 72209ba on jugglinmike:gh-3013 into f00091c on jshint:master.

coveralls · 2016-09-04T20:49:52Z

Coverage increased (+0.004%) to 97.723% when pulling 72209ba on jugglinmike:gh-3013 into f00091c on jshint:master.

jugglinmike · 2016-09-10T18:03:05Z

@rwaldron Thanks to gh-3017, I have a better understanding of the scope of this problem.
The bug concerns all warnings that are detected by the lexer, namely W048,
W049, W100, W101, W113, and W119. An unrelated bug fix that was released with
version 2.9.3 exposed the problem to a wider category of input. (I suppose that
still qualifies as a "regression," but we may be splitting hairs there.)

The context that triggers this error is the lookahead operations JSHint
performs to disambiguate between multiple valid parsing strategies:

the { token in an expression position (either an object literal or a
destructuring assignment)
the [ token (either an array literal or a destructuring assignment)
the ( token (either a grouping operator or an arrow function parameter
list)

This latest version of the patch addresses all lexer-triggered warnings in all
relevant contexts. In the interest of transparency, I've pushed up the new
version of the patch as a fixup! commit. Before this lands, I'd like to
squash with a new commit message that correctly describes the expanded scope of
the fix.

[[FIX]] Honor "ignore" directives during lookahead

When the lexer emits a token, it may not be aware of the complete set of
in-line linting directives because these are activated by the parser. This
is notably the case during parsing lookahead operations (e.g. when selecting
between multiple parsing strategies that may follow the {, [, and (
tokens).

In these contexts, the lexer should not immediately report token-related
warnings. Reporting must be deferred until the parser has visited all preceding
comments and fully applied all linting directives present in the source
code.

I've also included the above text as a comment within the test's fixture file.
That file necessarily contains a few instances of non-printable characters,
which has the unfortunate side-effect of suppressing its output in patch
summaries from git and GitHub.com.

@damyanpetev @batista would either of you mind testing this branch of JSHint
against your code base and verifying that it resolves your issue?

batista · 2016-09-11T07:38:22Z

Sure, I'm unable to do it now, but I'll test it and get back to you
tomorrow.

jugglinmike · 2016-09-11T15:23:48Z

Thanks!

rwaldron · 2016-09-11T17:15:55Z

I can review tomorrow—please nudge me if I haven't reported by noon

batista · 2016-09-12T09:33:45Z

@jugglinmike can confirm that 72209ba fixes #3017 for me

jugglinmike · 2016-09-12T13:28:09Z

Thanks, Sérgio!

rwaldron · 2016-09-12T19:40:26Z

@jugglinmike I'm confused about the relationship to #2741

jugglinmike · 2016-09-12T22:27:40Z

@rwaldron I made that comment when I believed that the bug reported in gh-3013 was limited to W100. From that perspective, another viable fix was to remove W100 altogether--I suggested that as a possibility in gh-2741.

While I still don't understand the intent of W100, it's clear that the underlying problem is more far-reaching, so I'm happy to let the conversation about removing W100 proceed separately.

damyanpetev · 2016-09-13T13:38:05Z

@jugglinmike A bit late, but I can also verify 72209ba honored the ignore for W100 warning with as initially logged in #3013 (massive IIFE). Also tested with this bit:

(function() {
    var test;
    /*jshint -W100 */
    test = "‏س";
})();

And it worked out okay as well.

rwaldron · 2016-09-13T14:01:49Z

Thanks @damyanpetev and @batista, you're testing and confirmation is helpful here

rwaldron

Mike,

Thanks for walking me through this changeset, and I apologize for the amount of time that it took for me to fully understand the context.

rwaldron · 2016-09-22T19:09:25Z

Create work here, thanks @jugglinmike

* [[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 mentioned this pull request Aug 20, 2016

Disabling warning for W100 not working in 2.9.3 #3013

Closed

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

4580e9e

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.

jugglinmike force-pushed the gh-3013 branch from 7b93cc5 to 4580e9e Compare August 20, 2016 20:48

jugglinmike mentioned this pull request Aug 27, 2016

/* jshint -W101 */ not working when withing IIFE #3017

Closed

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

72209ba

rwaldron approved these changes Sep 22, 2016

View reviewed changes

rwaldron merged commit a2b3881 into jshint:master Sep 22, 2016

BryanDonovan mentioned this pull request Jan 26, 2017

Update dependency jshint to version 2.9.4 jaredwray/cache-manager#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for gh-3013 #3016

Fix for gh-3013 #3016

jugglinmike commented Aug 20, 2016

coveralls commented Aug 20, 2016 •

edited

jugglinmike commented Aug 20, 2016

coveralls commented Aug 20, 2016 •

edited

jugglinmike commented Aug 27, 2016

coveralls commented Sep 4, 2016

coveralls commented Sep 4, 2016 •

edited

jugglinmike commented Sep 10, 2016

batista commented Sep 11, 2016 •

edited

jugglinmike commented Sep 11, 2016

rwaldron commented Sep 11, 2016

batista commented Sep 12, 2016

jugglinmike commented Sep 12, 2016

rwaldron commented Sep 12, 2016

jugglinmike commented Sep 12, 2016

damyanpetev commented Sep 13, 2016

rwaldron commented Sep 13, 2016

rwaldron left a comment

rwaldron commented Sep 22, 2016

Fix for gh-3013 #3016

Fix for gh-3013 #3016

Conversation

jugglinmike commented Aug 20, 2016

coveralls commented Aug 20, 2016 • edited

jugglinmike commented Aug 20, 2016

coveralls commented Aug 20, 2016 • edited

jugglinmike commented Aug 27, 2016

coveralls commented Sep 4, 2016

coveralls commented Sep 4, 2016 • edited

jugglinmike commented Sep 10, 2016

batista commented Sep 11, 2016 • edited

jugglinmike commented Sep 11, 2016

rwaldron commented Sep 11, 2016

batista commented Sep 12, 2016

jugglinmike commented Sep 12, 2016

rwaldron commented Sep 12, 2016

jugglinmike commented Sep 12, 2016

damyanpetev commented Sep 13, 2016

rwaldron commented Sep 13, 2016

rwaldron left a comment

Choose a reason for hiding this comment

rwaldron commented Sep 22, 2016

coveralls commented Aug 20, 2016 •

edited

coveralls commented Aug 20, 2016 •

edited

coveralls commented Sep 4, 2016 •

edited

batista commented Sep 11, 2016 •

edited