Parse error in isSemicolonToken

[edemaine]

My coding style seems to routinely trigger some edge case of the parser that causes a crash. Here's a minimal reproduction:

f(hi)
  key: value
  #comment

If I remove the second or third line, the parser works fine. But with the code above, I get the following error:

ESLint: 7.20.0

TypeError: Cannot read property 'value' of undefined
Occurred while linting C:\...\test.coffee:1
    at Object.isSemicolonToken (C:\...\node_modules\eslint\lib\rules\utils\ast-utils.js:561:18)
    at checkForSemicolon (C:\...\node_modules\eslint\lib\rules\semi.js:274:37)
    at C:\...\node_modules\eslint\lib\linter\safe-emitter.js:45:58
    at Array.forEach (<anonymous>:null:null)
    at Object.emit (C:\...\node_modules\eslint\lib\linter\safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (C:\...\node_modules\eslint\lib\linter\node-event-generator.js:256:26)
    at NodeEventGenerator.applySelectors (C:\...\node_modules\eslint\lib\linter\node-event-generator.js:285:22)
    at NodeEventGenerator.enterNode (C:\...\node_modules\eslint\lib\linter\node-event-generator.js:299:14)
    at CodePathAnalyzer.enterNode (C:\...\node_modules\eslint-plugin-coffee\lib\code-path-analysis\code-path-analyzer.js:577:21)
    at CodePathAnalyzer.dynamicallyDelegatingMonkeypatch (C:\...\node_modules\eslint-plugin-coffee\lib\patch-code-path-analysis.js:15:48)
    at CodePathAnalyzer.ESLintCodePathAnalyzer.<computed> (C:\...\node_modules\eslint-plugin-coffee\lib\patch-code-path-analysis.js:28:49)
    at C:\...\node_modules\eslint\lib\linter\linter.js:954:32
    at Array.forEach (<anonymous>:null:null)
    at runRules (C:\...\node_modules\eslint\lib\linter\linter.js:949:15)
    at Linter._verifyWithoutProcessors (C:\...\node_modules\eslint\lib\linter\linter.js:1175:31)
    at Linter._verifyWithConfigArray (C:\...\node_modules\eslint\lib\linter\linter.js:1273:21)
    at Linter.verify (C:\...\node_modules\eslint\lib\linter\linter.js:1228:25)
    at Linter.verifyAndFix (C:\...\node_modules\eslint\lib\linter\linter.js:1418:29)
    at verifyText (C:\...\node_modules\eslint\lib\cli-engine\cli-engine.js:239:48)
    at CLIEngine.executeOnFiles (C:\...\node_modules\eslint\lib\cli-engine\cli-engine.js:807:28)
    at ESLint.lintFiles (C:\...\node_modules\eslint\lib\eslint\eslint.js:521:23)
    at Object.execute (C:\...\node_modules\eslint\lib\cli.js:294:36)
    at main (C:\...\node_modules\eslint\bin\eslint.js:142:52)
    at Object.<anonymous> (C:\...\node_modules\eslint\bin\eslint.js:146:2)
    at Module._compile (node:internal/modules/cjs/loader:1108:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
    at Module.load (node:internal/modules/cjs/loader:973:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47

CoffeeScript 2.5.1 eslint-plugin-coffee 0.1.14 ESLint: 7.20.0

[helixbass]

@edemaine ok it looks like the specific cause of the crash is that ESLint's "token cursors" hardcode the assumption that comment tokens are never the last token corresponding to an AST node (eg here)

So I think the general pattern I've followed is for any existing ESLint rules that call into ESLint functionality like that that's incompatible with Coffeescript, to override that rule with a Coffeescript-specific rule that avoids that functionality

However in this case the rule that caused the crash was semi, which is a rule that doesn't make sense to run against Coffeescript code

If you use any of the existing "preset configs" that this plugin exposes (eg "plugin:coffee/eslint-recommended"), those non-Coffeescript-compatible rules are automatically disabled (so this crash would be avoided)

However, not everyone uses those preset configs (including your setup it seems)

So I think the correct solution here is to encourage everyone who uses this plugin to at least turn on a basic preset config that disables all rules that are incompatible with Coffeescript

So I exposed a new "plugin:coffee/disable-incompatible" preset config and updated the README to include turning on this config as part of the basic usage instructions (in #52)

So tl;dr can you try:

yarn add --dev github:helixbass/eslint-plugin-coffee#eslint-plugin-coffee-v0.1.15-dev.1-gitpkg

And add to your ESLint config:

  "extends": [
    "plugin/coffee:disable-incompatible"
  ]

and let me know if that fixes the crash?

[edemaine]

Ah, I hadn't realized the incompatibility was at that level. Sad that eslint doesn't even give me a nice error message, though now I see from the stack trace where it's the rule failing not e.g. the parser.

It turns out I had an explicit semi rule in my eslintrc rules list — oops. Removing that fixed the issue. (Before that, I tried adding your new coffee:disable-incompatible list, but that didn't help, presumably because rules takes precedent.)

The next issue I'm having is with eslint-plugin-react's jsx-no-target-blank:

TypeError: Cannot read property 'type' of null
Occurred while linting C:\...\MessagePDF.coffee:220
    at attributeValuePossiblyBlank (C:\...\node_modules\eslint-plugin-react\lib\rules\jsx-no-target-blank.js:33:26)

The disable-incompatible extension doesn't help with this one.

Is there an extends rule that encompasses the long list in the README? (Also for the JSX rules.) Then I could switch to that from plugin:react/recommended.

plugin:meteor/recommended seems to work fine, so I think I just need this React replacement; commenting it out, I get everything to lint! (though many new errors that I didn't have with @fellow/eslint-plugin-coffee, still need to sort through them)

[helixbass]

Is there an extends rule that encompasses the long list in the README? (Also for the JSX rules.) Then I could switch to that from plugin:react/recommended.

It looks like it was an oversight that plugin:react/all didn't turn on all of the rules in that list, I just fixed via #56 and pushed a dev version which you can try via:

yarn add --dev github:helixbass/eslint-plugin-coffee#eslint-plugin-coffee-v0.1.15-dev.3-gitpkg

(depending on the order that you see my replies, these are "cumulative" dev versions so just pick the latest one and it should include all fixes)

[helixbass]

The next issue I'm having is with eslint-plugin-react's jsx-no-target-blank

Can you provide a Coffeescript snippet that causes the crash you're describing?

[edemaine]

Sorry for missing this. Here's some code that fails, actually during jsx-no-bind (TypeError: Cannot read property 'type' of null):

<a href={if user? then path}>My Posts</a>

Oops, I misunderstood your statement. I thought you were saying that I could just extends: - plugin:react/recommended and your plugin would somehow modify that. But now I see I should use `extends: - plugin:coffee/react-recommended. Could you update the documentation? (Or I can submit a PR.)

[edemaine]

Now that I'm using the correct rules, here's some code that fails jsx-no-target-blank:

<a href={annotation.url or '#'}
target={if annotation.url then '_blank'}
rel={if annotation.url then 'noreferrer'}
onClick={onClick}/>

I also notice that <span dangerouslySetInnerHTML={__html: formattedFile.description}/> produces a no-undef error ('__html' is not defined).

[helixbass]

Oops, I misunderstood your statement. I thought you were saying that I could just extends: - plugin:react/recommended and your plugin would somehow modify that. But now I see I should use `extends: - plugin:coffee/react-recommended. Could you update the documentation? (Or I can submit a PR.)

Ok sorry looks like I wrote plugin:react/all above when I meant plugin:coffee/react-all

But I don't see anywhere in the README that references plugin:react/*? But ya let me know or feel free to open a PR

Sorry for missing this. Here's some code that fails, actually during jsx-no-bind (TypeError: Cannot read property 'type' of null):

I'm able to reproduce this one (using coffee/jsx-no-bind) and have a fix incoming

Now that I'm using the correct rules, here's some code that fails jsx-no-target-blank:

Ok I'm able to reproduce this by upgrading to the latest version of eslint-plugin-react. Let me plan on putting up a separate PR for fixing this and maybe trying to wrap my head a little more legitimately around the relevant versioning considerations for these dependencies

I also notice that <span dangerouslySetInnerHTML={__html: formattedFile.description}/> produces a no-undef error ('__html' is not defined).

Hmm I'm not able to reproduce that in the eslint-plugin-coffee testing environment. Not sure off the top of my head what would be causing that, that rule basically just delegates to the scope manager. And seems strange that it would be deciding that __html is a declared name there

Can you double-check (if you're not already doing this) that a file containing just that snippet produces that no-undef error?

Thanks for reporting all of these, great that you're "kicking the tires" on the React rules!

[helixbass]

The coffee/jsx-no-bind crash should be fixed if you install:

yarn add --dev github:helixbass/eslint-plugin-coffee#eslint-plugin-coffee-v0.1.15-dev.5-gitpkg

[edemaine]

Somehow I just totally missed the eslint-plugin-react section. Ah, I guess it's because the Table of Contents has a link to eslint-plugin-react rules (and not the other section), and I just assumed it would be there. Maybe more Table of Contents entries would help? But my bad, it's totally documented.

You're right about the <span> not triggering on its own. Here's a minimal repro:

import React from 'react'

inner =
  <>
    <a href="##{message._id}" data-id={message._id}>
      <span dangerouslySetInnerHTML={__html: formattedTitle}/>
    </a>
  </>

[helixbass]

Somehow I just totally missed the eslint-plugin-react section. Ah, I guess it's because the Table of Contents has a link to eslint-plugin-react rules (and not the other section), and I just assumed it would be there. Maybe more Table of Contents entries would help? But my bad, it's totally documented.

That makes sense, opened #62 to track it

Here's a minimal repro

Ok this one should be fixed by #61, it was a semi-known issue with JSX fragments. You can use a version including the fix via:

yarn add --dev github:helixbass/eslint-plugin-coffee#eslint-plugin-coffee-v0.1.15-dev.6-gitpkg

[edemaine]

I finally got around to trying the v0.1.15-dev.6 tag. Now the following code is causing a parse error for me:

<a target={if annotation.url then '_blank'}/>

TypeError: Cannot read property 'type' of null
Occurred while linting C:\...\MessagePDF.coffee:1
    at attributeValuePossiblyBlank (C:\...\node_modules\eslint-plugin-react\lib\rules\jsx-no-target-blank.js:33:26)

Hope this helps!

[helixbass]

Ok this is the same jsx-no-target-blank error you described above, which I just hadn't gotten around to fixing yet (thanks for bumping)

Per above, this is sort of due to a version misalignment (where a newer version of eslint-plugin-react (that was still valid according to this package's eslint-plugin-react dependency version) introduced a change that made that rule no longer work "out of the box" against Coffeescript code)

So what I did was upgrade the dependency to the newest version of eslint-plugin-react and fix that exact dependency version in order to try and prevent this type of issue from happening again in the future (I guess the tradeoff there is that eslint-plugin-coffee users wouldn't get compatible incremental upgrades to eslint-plugin-react rules until I explicitly bump the eslint-plugin-react dependency version here, but that seems acceptable)

And then I created a Coffeescript-specific overriding version of jsx-no-target-blank

So can you try using a new dev version and let me know if it resolves the issue?

yarn add --dev github:helixbass/eslint-plugin-coffee#eslint-plugin-coffee-v0.1.15-dev.7-gitpkg

If that looks good, I can probably go ahead and bump to a beta -> full release

[edemaine]

Thanks for tackling this! Given how fragile these dependencies are, I think it makes sense to pin versions like this. Just means more work in dependency maintenance...

Unfortunately, I'm still getting an error:

TypeError: Cannot read property 'type' of null
Occurred while linting C:\Users\edemaine\Projects\coauthor\client\MessagePDF.coffee:224
    at attributeValuePossiblyBlank (C:\Users\edemaine\Projects\coauthor\node_modules\eslint-plugin-react\lib\rules\jsx-no-target-blank.js:33:26)
    at hasTargetBlank (C:\Users\edemaine\Projects\coauthor\node_modules\eslint-plugin-react\lib\rules\jsx-no-target-blank.js:46:50)
    at JSXOpeningElement (C:\Users\edemaine\Projects\coauthor\node_modules\eslint-plugin-react\lib\rules\jsx-no-target-blank.js:127:14)
    at C:\Users\edemaine\Projects\coauthor\node_modules\eslint\lib\linter\safe-emitter.js:45:58

From the paths, it looks like it's calling the rule eslint-plugin-react\lib\rules\jsx-no-target-blank.js but not by way of eslint-plugin-coffee. I feel like I must be enabling this rule, but I don't see where that might be in my eslintrc. Any ideas?

P.S. I confirmed via eslint --print-config that react/jsx-no-target-blank is included and coffee/jsx-no-target-blank somehow isn't... Here's a partial excerpt:

    "coffee/display-name": [
      "error"
    ],
    "react/display-name": [
      "off"
    ],
    "coffee/jsx-key": [
      "error"
    ],
    "react/jsx-key": [
      "off"
    ],
    "react/jsx-no-comment-textnodes": [
      2
    ],
    "react/jsx-no-duplicate-props": [
      2
    ],
    "react/jsx-no-target-blank": [
      2
    ],
    "react/jsx-no-undef": [
      2
    ],

[following by a lot of `react/` rules]

[helixbass]

Ok ya I see it needs to be specified as part of react-recommended in order to override eslint-plugin-react's recommended config

Can you try:

yarn add --dev github:helixbass/eslint-plugin-coffee#eslint-plugin-coffee-v0.1.15-dev.8-gitpkg

[edemaine]

It's working! I just got this project to fully lint with eslint-plugin-coffee for the first time!

I'll try some other projects and see if I run into any other bugs.