Skip to content
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

no-implicit-coercion: <string expression> + "" triggers false positive #14623

Closed
martyfmelb opened this issue May 25, 2021 · 3 comments
Closed

no-implicit-coercion: <string expression> + "" triggers false positive #14623

martyfmelb opened this issue May 25, 2021 · 3 comments

Comments

@martyfmelb
Copy link

@martyfmelb martyfmelb commented May 25, 2021

Tell us about your environment

Node version: v10.15.0
npm version: v6.4.1
Local ESLint version: v7.25.0 (Currently used)
Global ESLint version: Not found
Operating System: win32 10.0.19042

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?

@typescript-eslint/parser

Please show your full configuration:

Configuration
"use strict";
const MAX_LINES_PER_FUNCTION = 128;
const MAX_PARAMS = 8;

const MAX_CODE_DEPTH = 8;
module.exports = {
  env: {
    amd: true,
    browser: true,
    node: true,
  },
  parser: "@typescript-eslint/parser",
  parserOptions: {
    ecmaFeatures: {
      jsx: true,
    },
    ecmaVersion: 2020,
    sourceType: "module",
  },
  plugins: ["prettier", "react-hooks", "prefer-arrow"],
  extends: ["eslint:recommended", "plugin:react/recommended", "prettier"],
  rules: {
    "accessor-pairs": ["error", { enforceForClassMembers: true, setWithoutGet: true }],
    "array-callback-return": ["error", { checkForEach: true }],
    "arrow-body-style": ["error", "as-needed"],
    "block-scoped-var": "error",
    "camelcase": "off",
    "class-methods-use-this": "error",
    "complexity": "error",
    "consistent-return": "error",
    "consistent-this": ["error", "self"],
    "curly": "error",
    "default-case": "error",
    "default-case-last": "error",
    "default-param-last": "error",
    "dot-notation": ["error", { allowKeywords: true }],
    "eqeqeq": ["error", "always"],
    "func-name-matching": "off",
    "func-names": "off",
    "func-style": ["error", "expression"],
    "grouped-accessor-pairs": "error",
    "guard-for-in": "error",
    "id-denylist": ["error", "data", "callback"],
    "id-length": "off",
    "id-match": "off",
    "init-declarations": "off",
    "line-comment-position": "off",comments
    "lines-between-class-members": ["error", "always", { exceptAfterSingleLine: true }],
    "max-classes-per-file": "error",
    "max-depth": ["error", MAX_CODE_DEPTH],
    "max-len": "off",
    "max-lines": "off",
    "max-lines-per-function": ["error", { max: MAX_LINES_PER_FUNCTION, skipBlankLines: true }],
    "max-nested-callbacks": "error",
    "max-params": ["error", MAX_PARAMS],
    "max-statements": "off",
    "max-statements-per-line": ["error", { max: 2 }],
    "multiline-comment-style": ["error", "starred-block"],
    "new-cap": "error",
    "no-alert": "error",
    "no-array-constructor": "error",
    "no-await-in-loop": "error",
    "no-bitwise": "error",
    "no-caller": "error",
    "no-confusing-arrow": "error",
    "no-console": ["error", { allow: ["warn", "error"] }],
    "no-constructor-return": "error",
    "no-continue": "off",
    "no-debugger": "error",
    "no-div-regex": "error",
    "no-dupe-class-members": "error",
    "no-duplicate-imports": "error",
    "no-else-return": "error",
    "no-empty": ["error", { allowEmptyCatch: true }],
    "no-empty-function": "error",
    "no-eq-null": "error",
    "no-eval": "error",
    "no-extend-native": "error",
    "no-extra-bind": "error",
    "no-extra-label": "error",
    "no-implicit-coercion": ["error", { allow: ["!!"] }],
    "no-implicit-globals": "error",
    "no-implied-eval": "error",
    "no-inline-comments": "off",
    "no-invalid-this": "error",
    "no-iterator": "error",
    "no-label-var": "error",
    "no-labels": ["error", { allowLoop: true }],
    "no-lone-blocks": "error",
    "no-lonely-if": "error",
    "no-loop-func": "error",
    "no-loss-of-precision": "error",
    "no-multi-assign": "off",
    "no-multi-str": "error",
    "no-negated-condition": "off",
    "no-nested-ternary": "off",
    "no-new": "error",
    "no-new-func": "error",
    "no-new-object": "error",
    "no-new-wrappers": "error",
    "no-nonoctal-decimal-escape": "error",
    "no-octal-escape": "error",
    "no-param-reassign": "error",
    "no-plusplus": "off",
    "no-promise-executor-return": "error",
    "no-proto": "error",
    "no-restricted-exports": "off",
    "no-restricted-globals": "off",
    "no-restricted-imports": "off",
    "no-restricted-properties": "off",
    "no-restricted-syntax": "off",
    "no-return-assign": "error",
    "no-return-await": "error",
    "no-script-url": "error",
    "no-self-compare": "error",
    "no-sequences": "error",
    "no-shadow": "off",
    "no-template-curly-in-string": "error",
    "no-ternary": "off",
    "no-throw-literal": "error",
    "no-trailing-spaces": "error",
    "no-undef-init": "off",
    "no-undefined": "off",
    "no-underscore-dangle": "error",
    "no-unmodified-loop-condition": "error",
    "no-unneeded-ternary": "error",
    "no-unreachable-loop": "error",
    "no-unsafe-optional-chaining": "error",
    "no-unused-expressions": "error",
    "no-use-before-define": ["error", { functions: false }],
    "no-useless-backreference": "error",
    "no-useless-call": "error",
    "no-useless-computed-key": "error",
    "no-useless-concat": "error",
    "no-useless-constructor": "error",
    "no-useless-rename": "error",
    "no-useless-return": "error",
    "no-var": "error",
    "no-void": ["error", { allowAsStatement: true }],
    "no-warning-comments": "off",
    "nonblock-statement-body-position": ["off"],
    "one-var": "off",
    "operator-assignment": ["error", "always"],
    "padding-line-between-statements": [
      "error",
      {
        blankLine: "always",
        next: "*",
        prev: ["block", "block-like", "cjs-export", "class", "export", "import"],
      },
      { blankLine: "any", next: ["export", "import"], prev: ["export", "import"] },
    ],
    "prefer-arrow-callback": "error",
    "prefer-exponentiation-operator": "error",
    "prefer-named-capture-group": "off",
    "prefer-numeric-literals": "error",
    "prefer-promise-reject-errors": "error",
    "prefer-regex-literals": "error",

    "prettier/prettier": ["error", { endOfLine: "auto" }],

    "radix": ["error", "as-needed"],

    "react-hooks/rules-of-hooks": "error",
    "react/display-name": "off",
    "react/no-unescaped-entities": "off",

    "require-atomic-updates": "error",
    "require-await": "error",
    "require-unicode-regexp": "off",
    "require-yield": "error",
    "rest-spread-spacing": ["error", "never"],
    "sort-imports": ["error", { ignoreCase: true, ignoreDeclarationSort: true }],
    "sort-keys": "off",
    "sort-vars": "off",
    "spaced-comment": ["error", "always"],
    "symbol-description": "error",
    "template-curly-spacing": "error",
    "valid-typeof": ["error", { requireStringLiterals: true }],
    "vars-on-top": "off",
    "yoda": "error",
  },
  overrides: [
    {
      env: {
        amd: true,
        browser: true,
        es2020: true,
        node: true,
      },
      files: ["*.ts", "*.tsx"],
      parserOptions: {
        project: "./tsconfig.json",
      },
      extends: [
        "eslint:recommended",
        "plugin:react/recommended",
        "plugin:@typescript-eslint/eslint-recommended",
        "plugin:@typescript-eslint/recommended",
        "plugin:@typescript-eslint/recommended-requiring-type-checking",
        "prettier",
      ],
      plugins: ["@typescript-eslint", "prettier", "react-hooks"],
      rules: {
        "@typescript-eslint/explicit-function-return-type": "off",
        "@typescript-eslint/explicit-member-accessibility": "off",
        "@typescript-eslint/explicit-module-boundary-types": "off",
        "@typescript-eslint/member-delimiter-style": [
          "error",
          {
            multiline: {
              delimiter: "semi",
              requireLast: true,
            },
            singleline: {
              delimiter: "semi",
              requireLast: false,
            },
          },
        ],
        "@typescript-eslint/no-duplicate-imports": "error",
        "@typescript-eslint/no-explicit-any": "error",
        "@typescript-eslint/no-invalid-this": "error",
        "@typescript-eslint/no-magic-numbers": [
          "error",
          {
            ignore: [-1, 0, 0.001, 0.01, 0.5, 1, 2, 100, 1000],
            ignoreEnums: true,
            ignoreNumericLiteralTypes: true,
          },
        ],
        "@typescript-eslint/no-namespace": "error",
        "@typescript-eslint/no-non-null-assertion": "error",
        "@typescript-eslint/no-this-alias": "error",
        "@typescript-eslint/no-unused-vars": ["error", { ignoreRestSiblings: true }],
        "@typescript-eslint/no-use-before-define": ["off"],
        "@typescript-eslint/no-var-requires": "error",
        "@typescript-eslint/triple-slash-reference": [
          "error",
          {
            lib: "never",
            path: "never",
            types: "never",
          },
        ],
        "@typescript-eslint/unified-signatures": "error",
        "default-param-last": "off",
        "no-duplicate-imports": "off",
        "no-invalid-this": "off",
        "no-new-symbol": "error",
        "no-use-before-define": "off",
        "object-shorthand": [
          "error",
          "properties",
          {
            avoidQuotes: true,
          },
        ],
        "prefer-arrow/prefer-arrow-functions": [
          "error",
          {
            classPropertiesAllowed: false,
            disallowPrototype: true,
            singleReturnOnly: false,
          },
        ],
        "prefer-destructuring": "error",
        "prefer-object-spread": "error",
        "prefer-template": "error",

        "quotes": ["error", "double", { avoidEscape: true }],

        "react/prop-types": "off",
      },
    },
    {
      files: ["*.test.ts", "*.test.tsx"],
      rules: {
        "@typescript-eslint/no-magic-numbers": "off",
        "@typescript-eslint/no-unsafe-assignment": "off",
        "@typescript-eslint/no-unsafe-call": "off",
        "@typescript-eslint/no-unsafe-member-access": "off",
        "@typescript-eslint/no-var-requires": "off",
      },
    },
    {
      files: ["**/styleguide/**/*.tsx", "**/styleguide/**/*.ts"],
      rules: {
        "@typescript-eslint/no-magic-numbers": ["off"],
      },
    },
    {
      env: {
        amd: true,
        browser: true,
        jquery: true,
        node: true,
      },
      files: ["*.js", ".prettierrc.js"],
      globals: {
        $: "readonly",
      },
      parserOptions: {
        ecmaVersion: 5,
        sourceType: "script",
      },
      rules: {
        "block-scoped-var": "error",
        "consistent-this": ["error", "self", "ctl", "exports"],
        "func-style": ["off"],
        "max-lines-per-function": ["off"],
        "max-params": "off",
        "new-cap": [
          "error",
          {
            capIsNewExceptions: [
              "CssMediaWatcher",
              "Deferred",
              "GenerateUuid",
              "GetPseudoElementContent",
              "ParseBool",
              "SmoothScrollTo",
            ],
          },
        ],
        "no-invalid-this": "off",
        "no-magic-numbers": ["error", { ignore: [-1, 0, 0.001, 0.01, 0.5, 1, 2, 100, 1000] }],
        "no-var": "off",
        "prefer-arrow-callback": "off",
        "prefer-arrow/prefer-arrow-functions": "off",
        "prefer-exponentiation-operator": "off",
        "sort-keys": "off",
        "strict": ["error", "function"],
      },
    },
    {
      env: {
        amd: true,
        browser: true,
        jquery: true,
        mocha: true,
        node: true,
      },
      files: ["*.spec.js", "test-helper.js"],
      parserOptions: {
        ecmaVersion: 5,
        sourceType: "script",
      },
      plugins: ["chai-friendly"],
      rules: {
        "block-scoped-var": "error",
        "chai-friendly/no-unused-expressions": 2,
        "func-style": ["off"],
        "max-lines-per-function": ["off"],
        "max-params": "off",
        "new-cap": ["error"],
        "no-magic-numbers": ["off"],
        "no-unused-expressions": 0,
        "no-var": "off",
        "prefer-arrow-callback": "off",
        "prefer-arrow/prefer-arrow-functions": "off",
        "sort-keys": "off",
        "strict": ["error", "function"],
      },
    },
    {
      env: {
        amd: true,
        node: true,
      },
      files: ["gulpfile.js", ".eslintrc.js"],
      parserOptions: {
        ecmaVersion: 2020,
        sourceType: "script",
      },
      plugins: ["sort-keys-fix"],
      rules: {
        "arrow-body-style": ["error", "as-needed"],
        "block-scoped-var": "error",
        "no-magic-numbers": ["error", { ignore: [-1, 0, 0.001, 0.01, 0.5, 1, 2, 100, 1000] }],
        "no-var": "error",
        "prefer-arrow-callback": "error",
        "prefer-arrow/prefer-arrow-functions": [
          "error",
          {
            classPropertiesAllowed: false,
            disallowPrototype: true,
            singleReturnOnly: false,
          },
        ],
        "sort-keys": "error",
        "strict": ["error", "global"],
      },
    },
  ],
  settings: {
    react: {
      version: "detect",
    },
  },
};

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

Here are some example statements which trigger it with our particular eslintrc setup:

var foo = { x: "a" }.x + "";

ESLint: use String({ x: "a" }.x) instead. (no-implicit-coercion)

var foo = ["a"][0] + "";

ESLint: use String(["a"][0]) instead. (no-implicit-coercion)

var foo = String("a") + "";

ESLint: use String(String("a")) instead. (no-implicit-coercion)

Swapping the operands of + makes no difference.

Changing the empty string to any other value in all of these cases removes this error. I've tried "a", the number 0, false, true, null, empty array [], all cases for which I'd expect to find a message about implicit coercion — they all, counterintuitively, remove the error.

I am running it from MINGW64 (i.e., bash on Windows):

./node_modules/.bin/eslint "/C/Projects/Client Website/client-site/Client.Site/Client/Site/Client.Squad.Web.UI/Assets/Client/Site/js/ng/directives/domain/domain-feature.js"

What did you expect to happen?

No error should result from the cases above.

For example, for the very first example above I would expect the output:

  448:15  error  'foo' is assigned a value but never used  no-unused-vars

✖ 1 problem (1 error, 0 warnings)

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

  448:15  error  'foo' is assigned a value but never used  no-unused-vars
  448:21  error  use `String({ x: "a" }.x)` instead        no-implicit-coercion

✖ 2 problems (2 errors, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

See the ESLint messages above.

@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage May 25, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage May 27, 2021
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented May 27, 2021

Hi @martyfmelb, thanks for the issue!

var foo = { x: "a" }.x + "";

ESLint: use String({ x: "a" }.x) instead. (no-implicit-coercion)

var foo = ["a"][0] + "";

ESLint: use String(["a"][0]) instead. (no-implicit-coercion)

While it's true that there will be no string coercion, because all operands evaluate to string values, fixing <expr> + "" to String(<expr>) doesn't really change anything - both versions have the same behavior and both repesent intention to convert <expr> to a string value. I think this doesn't warrant adding a complex logic that would be able to determine the type of operands.

var foo = String("a") + "";

ESLint: use String(String("a")) instead. (no-implicit-coercion)

We could add this particular check, to mirror the behavior of this rule for numbers:

/* eslint no-implicit-coercion: "error" */

var foo = { x: 2 }.x * 1; // error

var foo = [2][0] * 1; // error

var foo = Number(a) * 1; // no error!

Demo

Loading

@mdjermanovic mdjermanovic moved this from Triaging to Feedback Needed in Triage May 27, 2021
@btmills
Copy link
Member

@btmills btmills commented May 29, 2021

ESLint doesn't infer types, so the rule doesn't differentiate between string coercions of non-string values and redundant operations on values that are already strings. One could build two type-aware @typescript-eslint rules, one to replace the built-in no-implicit-coercion rule that doesn't have these false positives and a second no-redundant-coercion rule that instead flags these cases as redundant coercions.

I'd be fine detecting the explicit String() case for consistency with Number() since that seems within the rule's purview.

Loading

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented May 30, 2021

ESLint doesn't infer types, so the rule doesn't differentiate between string coercions of non-string values and redundant operations on values that are already strings. One could build two type-aware @typescript-eslint rules, one to replace the built-in no-implicit-coercion rule that doesn't have these false positives and a second no-redundant-coercion rule that instead flags these cases as redundant coercions.

Completely agree with @btmills, this is something that could be done in @typescript-eslint/eslint-plugin rather than in the core.

I'd be fine detecting the explicit String() case for consistency with Number() since that seems within the rule's purview.

I marked this issue as accepted to fix this part only, and prepared PR #14641.

Loading

@mdjermanovic mdjermanovic moved this from Feedback Needed to Pull Request Opened in Triage May 30, 2021
Triage automation moved this from Pull Request Opened to Complete Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Triage
Complete
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants