no-unused-modules does not work with shebangs

[ehmicky]

index.js:

#!/usr/bin/env node
import { example } from './foo.js'
export { example }

foo.js:

export const example = true

.eslintrc.yml:

parserOptions:
  ecmaVersion: 2019
  sourceType: module
plugins: [import]
rules:
  import/no-unused-modules: [2, {unusedExports: true}]

Then:

$ eslint
/home/user/example/foo.js
  1:1  error  exported declaration 'example' not used within other modules  import/no-unused-modules

✖ 1 problem (1 error, 0 warnings)

However this works when removing #!/usr/bin/env node.

eslint: 5.16.0
eslint-plugin-import: 2.17.3
node: 12.3.1
OS: Ubuntu 19.04

[ljharb]

It doesn't really make any sense to have an export in the same file as a shebang; overwhelming convention is to separate the library part from the bin part of a package.

That said, we should be able to strip the shebang.

[ehmicky]

Yes I agree. I only added an export in index.js so it does not trigger an ESLint warning, to make the issue simpler.

However even without that export, the bug is still present. I.e. foo.js example is reported as not imported, even though index.js imports it.

[rfermann]

It's not the rule having a problem with the shebang, it's the default parser used by ESLint:

Switching to another parser (e.g. babel-eslint or @typescript-eslint/parser solves this problem in my test case. Can you please verify it also fixes the error in your project, @ehmicky

[ehmicky]

Yes it does, thanks! However I think some users might not consider switching ESLint parser to get this rule to work.

Not sure if this is solvable by eslint-plugin-import though if the underlying parser does not cooperate.

[rfermann]

@ehmicky great to hear, thanks for the feedback.

I just had a look at import/named, which also parses the code to do some analysis. This rule returns a lint error (Parse errors in imported module './foo.js': Unexpected character '#') in case the file couldn't be parsed (see here).

We should do the same for import/no-unused-modules. I will prepare a PR for this later this week.

[ljharb]

Shebangs will soon become a regular part of the language, at which point eslint will parse then.

If the file doesn’t parse, these rules should probably bail out.

[rfermann]

Bailing out is what already happens. The rule basically fails silently, unfortunately for the wrong file, namely the importing file, not the exporting file.

I think, telling the user that a parsing error occurred at least lets the user know that something unexpected happened. In addition to returning the parsing error as a lint error, I would also explain this error on the readme. What do you think about that?

[ljharb]

As long as there's a workaround besides "remove the shebang" or "change the parser".

[rfermann]

I don't think this will be a good workaround, as it only tells the user that something unexpected happened. Technically seen there is no way of catching this issue and still reporting the correct results.

The only good workaround would include listening for that parsing error, programmatically and temporarily remove the shebang and re-parse the file. According to how the parsing works, this might be complex, as we can only pass file names to the parser, but no file content (in this case it would be far more easy to just strip out the shebang).

[ljharb]

That might actually be tenable if the parser can be made to accept string content, but if not, i'm not really sure what could be done.

[rfermann]

I did some quick tests on this. The parsers do accept a string (at least espree does, and the other parsers have to be compatible), and eslint-plugin-import is also already supplying this string.
I did a simple .replace('#!/usr/bin/env node', '') on the supplied string and got the expected linting result (no report about the export from foo.js), without parser errors.

Technically seen, we could strip out the shebang here (only until it is an official part of the language, of course). I can't evaluate, if this is a clean solution or just some quick-and-dirty solution, though.

[rfermann]

@ljharb what do you think about my last comment? Would that be a clean solution for this issue?

[ljharb]

@rfermann unless node itself exposes a helper function to strip the shebang (which it actually might), i don't think a string replace is reliable, especially because the shebang doesn't have to call into env, doesn't have to be called "node" (even "iojs" would work when installed), etc.

[silverwind]

It seems at least the rules import/default and import/no-named-as-default can also not deal with shebangs in imported files. They error out with Unexpected character '#' (1:1). Interestingly this issue seems only present when importing from ESM files, not from CJS ones.

[techmunk]

Would it not be possible to do what eslint does?

const textToParse = stripUnicodeBOM(text).replace(astUtils.shebangPattern, (match, captured) => `//${captured}`);

That's from the parse function in linters/linter.js in the eslint package.

[ljharb]

if eslint itself does it, then perhaps this is no longer an issue?

[silverwind]

if eslint itself does it, then perhaps this is no longer an issue?

Bug is still valid using latest versions of eslint and this plugin. I've set up a test repo at https://github.com/silverwind/eslint-hashbang-bug which you can clone and run npm install && npm test to reproduce the bug:

child.js
  1:1  error  exported declaration 'foo' not used within other modules  import/no-unused-modules

[silverwind]

Issue is probably that espree is still not able to parse shebangs. I think the fix will be to just do what eslint does and replace it with a line comment before handing it to the parser.

Maybe it's possible to even re-use the linked parse function in place of parser.parseForESLint.

[silverwind]

#2431