no-restricted-modules and TemplateLiteral

[pieh]

Tell us about your environment

• ESLint Version: 5.16.0 (in production), but issue exist also in 6.8.0
• Node Version: 10.16.3
• npm Version: 6.13.4

What parser (default, Babel-ESLint, etc.) are you using?

babel-eslint

Please show your full configuration:

Configuration

Pasted full config below - but relevant part is in overrides configuration for packages/gatsby-cli/src/**/*.js

const TSEslint = require("@typescript-eslint/eslint-plugin")

const gatsbyCLILocalReactImportPath = `packages/gatsby-cli/src/reporter/loggers/ink/react-instance-used-by-ink`

module.exports = {
  parser: "babel-eslint",
  extends: [
    "google",
    "eslint:recommended",
    "plugin:flowtype/recommended",
    "plugin:react/recommended",
    "prettier",
    "prettier/flowtype",
    "prettier/react",
  ],
  plugins: ["flowtype", "prettier", "react", "filenames"],
  parserOptions: {
    ecmaVersion: 2016,
    sourceType: "module",
    ecmaFeatures: {
      jsx: true,
    },
  },
  env: {
    browser: true,
    es6: true,
    node: true,
    jest: true,
  },
  globals: {
    before: true,
    after: true,
    spyOn: true,
    __PATH_PREFIX__: true,
    __BASE_PATH__: true,
    __ASSET_PREFIX__: true,
  },
  rules: {
    "arrow-body-style": [
      "error",
      "as-needed",
      { requireReturnForObjectLiteral: true },
    ],
    "no-unused-expressions": [
      "error",
      {
        allowTaggedTemplates: true,
      },
    ],
    "consistent-return": ["error"],
    "filenames/match-regex": ["error", "^[a-z-\\d\\.]+$", true],
    "no-console": "off",
    "no-inner-declarations": "off",
    "prettier/prettier": "error",
    quotes: ["error", "backtick"],
    "react/display-name": "off",
    "react/jsx-key": "warn",
    "react/no-unescaped-entities": "off",
    "react/prop-types": "off",
    "require-jsdoc": "off",
    "valid-jsdoc": "off",
  },
  overrides: [
    {
      files: [
        "packages/**/gatsby-browser.js",
        "packages/gatsby/cache-dir/**/*",
      ],
      env: {
        browser: true,
      },
      globals: {
        ___loader: false,
        ___emitter: false,
      },
    },
    {
      files: ["**/cypress/integration/**/*", "**/cypress/support/**/*"],
      globals: {
        cy: false,
        Cypress: false,
      },
    },
    {
      files: ["*.ts", "*.tsx"],
      parser: "@typescript-eslint/parser",
      plugins: ["@typescript-eslint/eslint-plugin"],
      rules: {
        ...TSEslint.configs.recommended.rules,
        // This rule ensures that typescript types do not have semicolons
        // at the end of their lines, since our prettier setup is to have no semicolons
        // e.g.,
        // interface Foo {
        // -  baz: string;
        // +  baz: string
        // }
        "@typescript-eslint/member-delimiter-style": [
          "error",
          {
            multiline: {
              delimiter: "none",
            },
          },
        ],
        // This ensures all interfaces are named with an I as a prefix
        // e.g.,
        // interface IFoo {}
        "@typescript-eslint/interface-name-prefix": [
          "error",
          { prefixWithI: "always" },
        ],
        "@typescript-eslint/no-empty-function": "off",
        // This ensures that we always type the return type of functions
        // a high level focus of our TS setup is typing fn inputs and outputs.
        "@typescript-eslint/explicit-function-return-type": "error",
        // This forces us to use interfaces over types aliases for object defintions.
        // Type is still useful for opaque types
        // e.g.,
        // type UUID = string
        "@typescript-eslint/consistent-type-definitions": [
          "error",
          "interface",
        ],

        // Allows us to write unions like `type Foo = "baz" | "bar"`
        // otherwise eslint will want to switch the strings to backticks,
        // which then crashes the ts compiler
        quotes: "off",
        "@typescript-eslint/quotes": [
          2,
          "backtick",
          {
            avoidEscape: true,
          },
        ],
      },
    },
    {
      files: [`packages/gatsby-cli/src/**/*.js`],
      rules: {
        "no-restricted-imports": [
          "error",
          {
            paths: [
              {
                name: "react",
                message: `Please import React from ${gatsbyCLILocalReactImportPath} (using relative local path) instead.`,
              },
            ],
          },
        ],
        "no-restricted-modules": [
          "error",
          {
            paths: [
              {
                name: "react",
                message: `Please import React from ${gatsbyCLILocalReactImportPath} (using relative local path) instead.`,
              },
            ],
          },
        ],
      },
    },
  ],
  settings: {
    react: {
      version: "16.4.2",
    },
  },
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

// notice using TemplateLiteral as argument for `require`
// eslint-disable-next-line no-unused-vars, quotes
const React = require(`React`)

yarn eslint packages/gatsby-cli/src/reporter/loggers/ink/test.js

What did you expect to happen?

For no-restricted-modules to raise linting error

What actually happened? Please include the actual, raw output from ESLint.

No linting error

Are you willing to submit a pull request to fix this bug?

Yes!

Extra information / motivation:

The no-restricted-modules works well when using regular single or double quoted strings as argument for require calls - it results in expected linting error then.

However gatsbyjs/gatsby repository uses quotes rule with backticks ( https://github.com/gatsbyjs/gatsby/blob/e720d8efe58eba0f6fae9f26ec8879128967d0b5/.eslintrc.js#L53 ) which automatically will turn all require call arguments to use backticks, which makes it so the no-restricted-modules rule is ineffective.

Code in rule:

eslint/lib/rules/no-restricted-modules.js

Lines 159 to 175 in 41de9df

	// node has arguments and first argument is string
	if (node.arguments.length && isString(node.arguments[0])) {
	const name = node.arguments[0].value.trim();
	// check if argument value is in restricted modules array
	if (isRestrictedPath(name)) {
	reportPath(node);
	}
	if (restrictedPatterns.length > 0 && ig.ignores(name)) {
	context.report({
	node,
	messageId: "patternMessage",
	data: { name }
	});
	}
	}

only handle cases of using Literal as argument, but it doesn't have handling for TemplateLiteral - see AST for:

• require("react") (Literal case):

  {
   "type": "ExpressionStatement",
   "start": 0,
   "end": 16,
   "expression": {
     "type": "CallExpression",
     "start": 0,
     "end": 16,
     "callee": {
       "type": "Identifier",
       "start": 0,
       "end": 7,
       "name": "require"
     },
     "arguments": [
       {
         "type": "Literal",
         "start": 8,
         "end": 15,
         "value": "react",
         "raw": "\"react\""
       }
     ]
   }
  }

• require(`react`) (TemplateLiteral case):

  {
   "type": "ExpressionStatement",
   "start": 17,
   "end": 33,
   "expression": {
     "type": "CallExpression",
     "start": 17,
     "end": 33,
     "callee": {
       "type": "Identifier",
       "start": 17,
       "end": 24,
       "name": "require"
     },
     "arguments": [
       {
         "type": "TemplateLiteral",
         "start": 25,
         "end": 32,
         "expressions": [],
         "quasis": [
           {
             "type": "TemplateElement",
             "start": 26,
             "end": 31,
             "value": {
               "raw": "react",
               "cooked": "react"
             },
             "tail": true
           }
         ]
       }
     ]
   }
  }

As mentioned in one of questions - I'm happy to provide pull request to adjust rule and tests to handle TemplateLiteral case (if it is desirable for the rule to handle it as well).

[kaicataldo]

I have verified this using our demo.

I think this is worth fixing, though please note that we are in the process of deprecating this rule (see #12835 for more info).

[pieh]

Thanks for quick response!

I don't want to add more work for you folks as I see there are already deprecation pull requests and migration of rules into eslint-plugin-node pull request. I personally can work around this (with vendored rule), but if you think it's still worthwile to make pull request here - I'll do it.

[kaicataldo]

Thanks for being willing! I think it would be great to get this in before we deprecate it, since at that point we won't be making bug fixes (though the rule will live on in eslint-plugin-node). We can then either update the eslint-plugin-node PR or follow up with a second PR to add this to the plugin :) Please let us know if you have any questions while working on this!

[pieh]

I opened pull request ( #12927 ) and only questions I have are

Does this change needs any documentation changes?
Are added test fixtures enough?

But I included them in the PR description so maybe those could be answered there