Skip to content

[restrict-plus-operands] False positive reported when adding two numbers #624

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

Closed
TabithaLarkin opened this issue Jun 18, 2019 · 22 comments
Closed
Labels
awaiting response Issues waiting for a reply from the OP or another party package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@TabithaLarkin
Copy link

TabithaLarkin commented Jun 18, 2019

Repro

{
  "rules": {
    "@typescript-eslint/restrict-plus-operands": "error"
  }
}
namespace Other {
  export abstract class ILegend
  {
    protected legendWidth: number = 244;

    public get width(): number { return this.legendWidth; }
  }

  export class Canvas
  {
    protected width: number;
  }

  export class FoLegend extends ILegend
  {
  }

  export class Legend extends ILegend
  {
  }


  export abstract class Chart extends Canvas
  {
    protected static readonly svgcMargin: number = 10;

    protected legend: ILegend;

    constructor(foreignObjectSupported: boolean)
    {
      super();

      if (foreignObjectSupported)
        this.legend = new FoLegend();
      else
        this.legend = new Legend();
    }

    // This is the line it should error on.
    protected get legendXPosition(): number { return this.width - (Chart.svgcMargin + this.legend.width); }
  }

  export class PieChart extends Chart
  {
    public getAvailableWidth(): number
    {
      // This is the line it should error on.
      return this.width - (this.legend.width + Chart.svgcMargin);
    }
  }

  export class BarChart extends Chart
  {
    public getAvailableWidth(): number
    {
      // This is the line it should error on.
      return this.width - (this.legend.width + Chart.svgcMargin);
    }
  }

  export class PointChart extends Chart
  {
    public getAvailableWidth(): number
    {
      // This is the line it should error on.
      return this.width - (this.legend.width + Chart.svgcMargin);
    }
  }
}

Expected Result
No error

Actual Result
this.legend.width + Chart.svgcMargin reports a false positive

Versions

package version
@typescript-eslint/eslint-plugin 1.10.2
@typescript-eslint/parser 1.10.2
TypeScript 3.4.3
ESLint 5.16.0
@TabithaLarkin TabithaLarkin added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jun 18, 2019
@bradzacher
Copy link
Member

I updated your example to include the line protected readonly legend: Legend; in the Chart class.
Before it was not defined, which was throwing errors.
If this was incorrect, please correct your example accordingly.

If there are type errors in your codebase, typescript will essentially resolve the type to any.
So in the case of your original example, this.legend was not defined, which meant that its type was any, meaning the type of this.legend.width was any, meaning the sum was resolving to any + number which is why the rule was flagged.

Running the updated example against master works fine.

for posterity, this was the code I ran against master
export abstract class Legend
{
  protected legendWidth: number = 244;
  public get width(): number { return this.legendWidth; }
}

export abstract class Chart
{
  protected static readonly svgcMargin: number = 10;
  protected readonly legend: Legend;
}

// Code that is failing in a class that extends Chart
export class PieChart extends Chart
{
  protected fn(): void
  {
      const thing = this.legend.width + Chart.svgcMargin;
  }
}

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Jun 18, 2019
@TabithaLarkin
Copy link
Author

Sorry about that I was trying to manually cut down the example code and missed something out. I've updated with the code that I think should repro my issue, though in this example I can't get it to error for some reason. But I am most definitely seeing the issue in my code, despite both properties being explicitly defined as numbers.

eslinterror

As I'm working with a private repo I am not able to share the full code that is giving me issues. It's quite large if I were to include everything.

I have a theory that it might be something to do with the generated mapping file creating links to different cased files, and somehow this breaks some sort of type checking. As in some files refer to:

/// <reference path="Chart.ts" />

And others refer to:

/// <reference path="chart.ts" />

I'm not sure if this makes a difference, but I'll try updating my references and then seeing what happens.

@TabithaLarkin
Copy link
Author

Yeah, that didn't fix it...

@TabithaLarkin
Copy link
Author

I've just done a check of our full codebase, and this is not the only set of variables that this is occurring with.

ESlintStringExample

In the above case you can clearly see that the encodeUriComponent function is returning a string, but the type of one of the parameters is clearly not being interpreted correctly.

@bradzacher
Copy link
Member

bradzacher commented Jun 19, 2019

There must be something misconfigured with your codebase.

I just tested this against master and it worked fine:

declare const UrlHelper : { encodeUriComponent(v: string): string }
const url = 'foo' + UrlHelper.encodeUriComponent('thing');

I suspect that when typescript is launched via the eslint run, it can't find all of your types, meaning it types all of those things are typed as any.

Do you have multiple tsconfig files? Is your code in a monorepo (i.e. are there different package.jsons aroudn the place?
Your lint run isn't hard failing, so it means that eslint can at least successfully find + load your tsconfig file.

@TabithaLarkin
Copy link
Author

TabithaLarkin commented Jun 19, 2019

It's a monorepo, that's set up to use Typescript projects. The tsconfig.json file that is referenced contains a reference to all of the tsconfig.json files in our repo. Though the neither of the places that I have previously mentioned contained any references to any code outside of its own tsconfig.json scope.

I don't see how it can be an issue with the way we've set up our codebase, as typescript picks up the types of each of these variables and functions correctly.

The weird thing about this issue is that it is not consistent. it is happening in multiple places, but not for all places where strings or numbers are being added together.

Is there any way of getting the output of estree to see what the type is being detected as?

@bradzacher
Copy link
Member

Sorry, I don't mean your codebase itself is misconfigured, I mean the eslint -> typescript integration is misconfigured somehow.

I don't have enough knowledge of how typescript projects work to know what the issue is exactly.
I assume your source code is private?

Could you try supplying each project's tsconfig to the projects setting in eslint? Instead of just supplying a single project: "path", you should be able to supply multiple projects: ["path", "path"].

@teoxoy
Copy link
Contributor

teoxoy commented Aug 2, 2019

I had the same problem with a yarn workspaces monorepo.
Here is how I solved it:

const glob = require('glob')
const configs = glob.sync('./packages/**/tsconfig.json')

module.exports = {
    parser: '@typescript-eslint/parser',
    parserOptions: {
        project: configs[0],
        projects: configs
    },
    ...
}

All the other tsconfigs extend a base config that is at the root of the project. BUT using the base tslint config as the project doesn't work.

@bradzacher I was planning to switch from using a JS eslint config to using the yaml alternative file but it's no longer possible because of the runtime component. It would be very helpful to add a glob pattern option to parserOptions. Would you accept a PR that does this or should I open another issue for this to gather more feedback?

@bradzacher
Copy link
Member

Are you able to try installing the 2.0.0 canary (npm i @typescript-eslint/eslint-plugin@canary @typescript-eslint/parser@canary)?

We've put a fix into the new version which should hard error for files that aren't included within your project config.
It'll help you discover when files aren't covered by the provided tsconfig(s).

You'll be able to add a single tsconfig, and then keep adding in more as required.


As an aside, part of me thinks that the easiest solution to this is to just create a tsconfig in the root of your project

tsconfig.eslint.json

{
  "compilerOptions": { "strict": true },
  "include": ["packages/**/*"],
  "exclude": ["node_modules"]
}

I'm not sure what is easier.


It would be very helpful to add a glob pattern option to parserOptions

It's probably a good idea. Maybe you could set it up for the projects option. I haven't done enough with globs but I assume you can just apply the glob to the provided projects strings?

@teoxoy
Copy link
Contributor

teoxoy commented Aug 2, 2019

I have tried v2 and with the eslint config I had before and the files in question are being included and eslint still complains about restrict-plus-operands where it shouldn't.

However, if I add a file tsconfig.eslint.json as per your suggestion with:

{
  "compilerOptions": { "strict": true },
  "include": ["packages/*"], // <-- packages/**/* doesn't seem to work (files are not being included)
  "exclude": ["node_modules"]
}

then the restrict-plus-operands errors go away

IMO the most elegant solution is to allow us to pass a glob pattern for the parserOptions.projects
I will start a PR for it - should be pretty straight forward

I got a question in regards to this: Why is it required to pass something to parserOptions.project when using parserOptions.projects? Could we make project optional in cases where projects is being used? Then internally pass the first string in the projects array to project.

@bradzacher
Copy link
Member

bradzacher commented Aug 2, 2019

cc @uniqueiniquity, who has more context on how the ts compiler piece works.

@uniqueiniquity
Copy link
Contributor

Hi all,

There is no projects option, only project. project can take either a string or an array. Currently, it can't take a glob, but I don't see any reason why that would be a bad thing.

@teoxoy
Copy link
Contributor

teoxoy commented Aug 2, 2019

There is no projects option

@uniqueiniquity
Copy link
Contributor

uniqueiniquity commented Aug 2, 2019

Yeah... Extra is the internal, cleaned up version of the public API. You can see on line 34 of that file that project is the public version. That's probably my fault :/

@teoxoy
Copy link
Contributor

teoxoy commented Aug 2, 2019

Ohh, so that means I can provide the array of strings to the project option.

it can't take a glob, but I don't see any reason why that would be a bad thing

Regarding this, the context was that I wanted to convert my eslint config from JS to YAML but I can't because of the runtime component (the glob that searches for all tsconfigs). Passing a glob would solve this.

@uniqueiniquity
Copy link
Contributor

Regarding this, the context was that I wanted to convert my eslint config from JS to YAML but I can't because of the runtime component (the glob that searches for all tsconfigs). Passing a glob would solve this.

Sorry, I wasn't clear - I fully support it taking a glob :)

@teoxoy
Copy link
Contributor

teoxoy commented Aug 2, 2019

No problem :)
I will start a PR for it then.

We should also mention here that project can accept an array of strings (and a glob pattern when the PR get's merged). I went hunting for the projects option and now I find it's actually an internal option. lol

@teoxoy
Copy link
Contributor

teoxoy commented Aug 3, 2019

@uniqueiniquity I got a few more questions:

Does the parser have any IO functionality? I have looked at the source and it doesn't seem to be the case. If the current implementation is not dependent on IO, I think the glob stuff should only be implemented for the parseForESLint function

export function parseForESLint(


Does parseForESLint run on every file? I think so because of the code arg but not sure. If it does run on every file, I think from a performance standpoint it's necessary to cache the output of the glob function (it's IO bound so it adds overhead)


On a side-note, I am planning to use is-glob to find out if parserOptions.project is a glob pattern or not. I imagine the majority of people don't need to provide globs so this should take care of that.

@uniqueiniquity
Copy link
Contributor

@teoxoy It does depend on IO in the sense that if you provide one or more tsconfig.json files, it reads files from disk in order to construct the corresponding TypeScript program, via the TypeScript Compiler API. I'm not sure I follow how that affects the way we choose to accept globs, however.

parseForESLint does run on every file in a given lint operation, sometimes more than once.

Could you open a new issue to track this work? In case the original issue reported here continues to be a problem, I don't want to get too far away from the original topic.

@teoxoy
Copy link
Contributor

teoxoy commented Aug 5, 2019

I'm not sure I follow how that affects the way we choose to accept globs, however.

I was wondering because some parsers don't have any IO operations and can be used in other environments other than node. I only wanted to make sure this was out of scope for this project.

Could you open a new issue to track this work? In case the original issue reported here continues to be a problem, I don't want to get too far away from the original topic.

Sure


Regarding parseForESLint, where should the glob functionality be implemented?

In the parseForESLint function (which is in the parser package) or here:

if (typeof options.project === 'string') {

(in the typescript-estree package)?

@uniqueiniquity
Copy link
Contributor

I was wondering because some parsers don't have any IO operations and can be used in other environments other than node. I only wanted to make sure this was out of scope for this project.

I see. I think this project is specific to Node - @bradzacher can confirm.

I'd suggest the glob functionality be in the typescript-estree project.

@bradzacher
Copy link
Member

If you provide project, then it requires node because of the typescript compiler.
Otherwise it doesn't require node.
Hence astexplorer.net works without a project.

We'd have to do some work to make it work fine in the browser with a project.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

4 participants