8000 [@typescript-eslint/restrict-plus-operands] False positive for type `0|1` · Issue #4689 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

[@typescript-eslint/restrict-plus-operands] False positive for type 0|1 8000 #4689

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.

Sign up for GitHub

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

Jump to bottom
Closed
3 tasks done
viceice opened this issue Mar 15, 2022 · 24 comments
Closed
3 tasks done

[@typescript-eslint/restrict-plus-operands] False positive for type 0|1 #4689

viceice opened this issue Mar 15, 2022 · 24 comments
Labels
external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@viceice
Copy link
viceice commented Mar 15, 2022 •
edited
Loading
  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/restrict-plus-operands": ["error"]
  }
}
export function getFutureVersion(
  policy: number,
  newVersion: string,
  baseVersion?: string
): number[] {
  const toRelease: number[] = [];
  const baseRelease: number[] = [];
  return baseRelease.map((_, index) => {
    const toPart = toRelease[index] || 0;
    if (index < policy) {
      return toPart;
    }
    if (index === policy) {
      return toPart + (baseVersion === undefined ? 0 : 1);
    }
    return 0;
  });
}

happens after update from v5.12.1to v5.13.0.

Expected Result

Should not cause an error.

Actual Result
image
The rule causes a false positive on code above.
https://github.com/renovatebot/renovate/pull/14597/files#diff-f6e993a4715dd7526818c5bb2cdfdaa3a139db2aa290ba8857b9d93f8fe03c4f

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 5.14.0
@typescript-eslint/parser 5.14.0
TypeScript 4.6.2
ESLint 8.10.0
node 14.19.0
@viceice viceice added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Mar 15, 2022
@viceice viceice mentioned this issue Mar 15, 2022
Merged
1 task
@bradzacher
Copy link
Member
bradzacher commented Mar 15, 2022 •
edited
Loading

I am unable to repro this against master.
playground

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party unable to repro issues that a maintainer was not able to reproduce and removed triage Waiting for team members to take a look labels Mar 15, 2022
@vpzomtrrfrt
Copy link

I'm also seeing a false positive with this rule that I can't reproduce in a new project. Seems to be related to the upgrade from typescript 4.5 to 4.6

@bradzacher
Copy link
Member

This rule hasn't changed in several months, definitely not so in the latest version. So it's doubtful that it was the latest release that broke something.

If you can create an isolated repro in either the playground or in a stand-alone git repo, we can definitely help out and take a closer look!

Without a repro - we unfortunately can't provide much help.

@viceice

This comment was marked as outdated.

Sign in to view
@bradzacher
Copy link
Member
bradzacher commented Mar 16, 2022 •
edited
Loading

Line 6 is the line I added to show the rule is working fine and correctly enabled. It purposely adds a string and a number, which is banned by the rule.

Lines 3 and 4 are from your example code. You reported that line 4 errored when it shouldn't. But as shown - I cannot repro. Line 4 does not error.

I'll close this for now as cannot repro.
If someone can provide an isolated repro in the playground or in a gh repo we can reopen and investigate.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 16, 2022
@viceice
Copy link
Author
viceice commented Mar 16, 2022

Updated repro code. Not sure how to reduce more. If you copy it to the playground, it will show the issue.

@viceice
Copy link
Author
viceice commented Mar 16, 2022

it seems toPart isn't correctly inferred
image

@viceice
Copy link
Author
viceice commented Mar 16, 2022

OK, verified. This works when explicit typing toPart
image

@viceice
Copy link
Author
viceice commented Mar 16, 2022

Reproduction playground added to issue

@JoshuaKGoldberg
Copy link
Member
JoshuaKGoldberg commented Mar 16, 2022 •
edited
Loading

Thanks for following up with more details @viceice! The issue in the playground code snippet is that toPart is type any. Confirmed that if we change the playground code snippet to const toPart: number there's no more issue updated playground link. Your issue is with TypeScript, not typescript-eslint.

By the way, as a general tip, instead of posting screenshots of code or CI reports, please post plain text and/or links to playground snippets. Screenshots are harder for us to work with: they can't be copy&pasted and aren't accessible to folks who rely on screenreaders unless they have very detailed alt text / captions.

@viceice
Copy link
Author
viceice commented Mar 16, 2022 •
edited
Loading

Sorry about the pictures, just want to show the issue.

I know that adding const toPart: number will fix it, but toPart should already be number.
vscode show's it as number:
image

Will add a git repo for reproduction too. https://github.com/renovate-reproductions/typescript-eslint-issue-4689/blob/0f5918e579a1126fa27e105d6ffab5955a423c2a/index.ts#L10

@bradzacher
Copy link
Member

This is a bug with TS.

You can repro it in the TS playground - TS intellisense shows that toPart is any in v4.6.2.

But at the same time TS knows that the type of toPart is a number:
image

However in TS v4.5.5 this does not repro - it shows that the type is number.

In TS 4.6.2 the API that we use to get the type (checker.getTypeAtLocation) returns the type as any, which is all we can base our linting off of.

This is a bug in TS.

@viceice
Copy link
Author
viceice commented Mar 17, 2022

We had ts 4.6.2 with both ts-eslint versions v5.12.1 and v5.13.0 and it started failing on v5.13.0. I see v5.14.0 has added ts v4.6 support.

So it's strange to me why it fails on v5.13.0 🤔

@viceice
Copy link
Author
viceice commented Mar 17, 2022

@bradzacher Can you help me to find or file a typescript bug? I'm not really sure how to do this, as the normal typescript compiler doesn't show any issue.

@viceice
Copy link
Author
viceice commented Mar 17, 2022 •
edited
Loading

Found another sample of failing type inference

/* eslint "@typescript-eslint/restrict-template-expressions": ["error"] */
const PREFIX_DOT = 'PREFIX_DOT';

const TYPE_NUMBER = 'TYPE_NUMBER';
const TYPE_QUALIFIER = 'TYPE_QUALIFIER';

export interface BaseToken {
  prefix: string;
  type: typeof TYPE_NUMBER | typeof TYPE_QUALIFIER;
  val: number | string;
  isTransition?: boolean;
}

export interface NumberToken extends BaseToken {
  type: typeof TYPE_NUMBER;
  val: number;
}

export interface QualifierToken extends BaseToken {
  type: typeof TYPE_QUALIFIER;
  val: string;
}

export type Token = NumberToken | QualifierToken;

// failed version
export function tokensToStr(tokens: Token[]): string {
  return tokens.reduce((result, token, idx) => {
    const prefix = token.prefix === PREFIX_DOT ? '.' : '-';
    return `${result}${idx !== 0 && token.val !== '' ? prefix : ''}${
      token.val
    }`;
  }, '');
}

// fixed version
function tokensToStr1(tokens: Token[]): string {
  return tokens.reduce((result:string, token:Token, idx) => {
    const prefix = token.prefix === PREFIX_DOT ? '.' : '-';
    return `${result}${idx !== 0 && token.val !== '' ? prefix : ''}${
      token.val
    }`;
  }, '');
}

repro added to https://github.com/renovate-reproductions/typescript-eslint-issue-4689

Playground

@bradzacher
Copy link
Member

Okay so in the playground the lib types aren't being loaded - so the array types aren't loaded properly - meaning the ESLint run sees .reduce as any, and thus result is any.

I knew that the Promise types didn't load - but I didn't realise we had this issue for arrays as well. Issue is tracked here #4493

If we manually add the array types it'll work as expected

Either way - I just re-tested your cases directly against the master tests (which aren't borked like the playground) and they passed just fine:
image
Which suggests the issue is within your project somewhere.

@bradzacher
Copy link
Member

So I simplified your repro further to this eslintrc
https://github.com/bradzacher/typescript-eslint-issue-4689/blob/patch-1/.eslintrc.js

The interesting thing is that if you remove the lint rule '@typescript-eslint/no-misused-promises': ['error'], from the config, the error in restrict-template-expressions goes away!!!

@viceice
Copy link
Author
viceice commented Mar 17, 2022

I've reduced the repo at https://github.com/renovate-reproductions/typescript-eslint-issue-4689 to be very minimal.

@JamieMagee is working to reduce it even futher and will open an upstream issue on typescript.

it seems this additional lint rules make a difference config

  rules: {
    // TODO: removing these two lines to remove the lint errors
    '@typescript-eslint/no-unsafe-return': 0,
    '@typescript-eslint/no-unsafe-argument': 0,

    '@typescript-eslint/restrict-template-expressions': [
      2,
      { allowNumber: true, allowBoolean: true },
    ],
    '@typescript-eslint/restrict-plus-operands': 2,
  },

https://github.com/renovate-reproductions/typescript-eslint-issue-4689/blob/3432b05c5700c04a132f41e092677da566bd7796/.eslintrc.js#L17-L28

@viceice
Copy link
Author
viceice commented Mar 17, 2022

So I simplified your repro further to this eslintrc https://github.com/bradzacher/typescript-eslint-issue-4689/blob/patch-1/.eslintrc.js

The interesting thing is that if you remove the lint rule '@typescript-eslint/no-misused-promises': ['error'], from the config, the error in restrict-template-expressions goes away!!!

So it seems there is some crossing between those rules, which cause the strange behavior. We do not even have any Promise reference in example code 😕

@bradzacher
Copy link
Member

None of our lint rules mutate any of the TS data - they all just read TS data.
I confirmed this by adding readonly to all of the TS types and I got no errors

All TS types are computed lazily - TS computes exactly what you ask for and nothing more! So if I had to hazard a guess, what's happening is that there's a bug in TS wherein it is not computing the correct type information for the variable based solely on the query in restrict-template-expressions. no-misused-promises must query for some additional types which then changes the types that TS returns when restrict-template-expressions runs after it.

I went through and commented out all of the code in no-misused-promises until it stopped erroring. I narrowed it down to the VariableDeclarator selector - if I comment this one out the errors disappear.

Next I commented out code from within its function. When I comment out this line the errors disappear.

Screen.Recording.2022-03-17.at.11.04.43.mov

@bradzacher
Copy link
Member
bradzacher commented Mar 17, 2022 •
edited
Loading

This will require some effort to create an isolated repro using just the TS APIs in order to submit a proper bug report to TS.

@bradzacher bradzacher reopened this Mar 17, 2022
@bradzacher bradzacher added external This issue is with another package, not typescript-eslint itself and removed unable to repro issues that a maintainer was not able to reproduce labels Mar 17, 2022
@bradzacher bradzacher mentioned this issue Mar 17, 2022
Closed
@bradzacher
Copy link
Member

I have raised microsoft/TypeScript#48313 which covers one of the issues and was easy to isolate just using the playground.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Jul 29, 2022
Closed
4 tasks
@bradzacher bradzacher mentioned this issue Oct 2, 2022
Closed
4 tasks
@stefanhamburger
Copy link

In case it helps, here are two more examples which trigger this bug.

interface Time {
  minutes?: number;
}
declare const time: Time;
 
// Throws @typescript-eslint/restrict-plus-operands
const sum1 = 3 + (time.minutes ?? 0);
 
if (time.minutes) {
  // Throws @typescript-eslint/restrict-plus-operands
  const sum2 = 3 + time.minutes;
}

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Jan 6, 2023
Merged
3 tasks
@Josh-Cena
Copy link
Member

On the latest TS version, this doesn't seem to be an error anymore.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@viceice @JoshuaKGoldberg @vpzomtrrfrt @bradzacher @stefanhamburger @Josh-Cena
0