8000 Rule proposal: warn against unused array iterator method return values · Issue #2509 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Rule proposal: warn against unused array iterator method return values #2509

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
Open
PSeitz opened this issue Sep 7, 2020 · 10 comments
Open

Rule proposal: warn against unused array iterator method return values #2509

PSeitz opened this issue Sep 7, 2020 · 10 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@PSeitz
Copy link
PSeitz commented Sep 7, 2020 •
edited by Josh-Cena
Loading

There are some function calls where the return value should not be ignored (like some, indexOf, concat, map, etc.) or the function call doesn't make sense. Would be nice to cover this with the no-unused-expressions lint. I did test the snippets below on current master, they both don't fail.

function foo(arr) {
  if (arr) {
    arr.some(el == 'myval'); // should be: return arr.some(el == 'myval');
  }
  return undefined;
}
function foo(arr) {
  var merged = arr.reduce(function (a, b) {
    a.concat(b);  // should be: return a.concat(b);
  });
}
@PSeitz PSeitz added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Sep 7, 2020
@bradzacher
Copy link
Member

I have seen people do weird things like use some as a short-circuiting iterator (yes it's weird, yes I hate it, but people do do it).

I think that this would be better as a separate rule that's more generalised (for example #1314)

@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 Sep 7, 2020
@PSeitz
Copy link
Author
PSeitz commented Sep 8, 2020

Now you mention it, I also saw some used like this. I think it's bad style, because it's hard to understand but could be optionally accepted.
The main difference between #1314 is, that I would like to focus on builtin methods where ignoring the value is very likely a bug.

So I could imaging, that on #1314 we can control that normal return values functions are ignored, but that predefined list of function names are errors (which again could be extended or shortened, e.g. to cover some). What do you think?

@bradzacher
Copy link
Member

Configuring a rule like this is rather difficult, as it is with any type-aware lint rule.
Names are entirely contextual. So you have to design a system for specifying the type name + method name pairs such that you can uniquely resolve them to a single type (i.e. you don't want to false-positive on a type just because it has the same name).

This is why I usually don't allow simple ignore lists in type-aware lint rules.

As an aside, my only problem with rules like this (which is why I've never bothered to implement them myself) is that they are going to be super expensive to run.

TS types are lazily computed, so the more that a rule inspects, the slower that the rule runs. We already see this in rules like no-floating-promises - which inspects the type of every ExpressionStatement.

It's a high price to pay for a rarer bug vector.

@PSeitz
Copy link
Author
PSeitz commented Sep 26, 2020

Yes, since this is type-aware it should not be mixed with #1314, but be a seperate rule.

How should expensive functions handled? Flagged as expensive or moved to a seperate plugin?
I think it would make sense as part of a more extensive codescan, maybe on ci.

@bradzacher
Copy link
Member

We can flag it in the readme for the rule for sure.

@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed awaiting response Issues waiting for a reply from the OP or another party labels Feb 18, 2021
@rafaelss95 rafaelss95 mentioned this issue Sep 1, 2021
Open
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@bradzacher bradzacher changed the title [no-unused-expressions] extend to cover some functions Rule proposal: warn against unused array iterator method return values May 3, 2022
@jguddas
Copy link
Contributor
jguddas commented Jul 22, 2022

I like this 👍

If you think about it, there are a crazy amount of ways to have floating expressions.

function iContainFloatingEpressions() {
    'foo';
    550;
    [].slice(0);
    [].splice(0);
    (new Date()).getHours();
    (() => { console.log() });
}

@Josh-Cena
Copy link
Member

This should be part of https://typescript-eslint.io/rules/no-unused-expressions/, no? We are essentially special-casing a subset of call expressions to be flagged.

@bradzacher
Copy link
Member

It could be - it's just that it's quite a deviation from the rule.
It's a reasonably rare case that people leave behind such dead code - so I'm not sure if people would really want this turned on by default?

There's space here to track all function calls and report if the return value is unused.
The problem is that there is no way to track/declare side-effects in TS. Really the rule wants to report on "all function calls that are pure and their return value is not used" - but without a system for tracking purity that sort of rule would be full of false-positives

@Josh-Cena
Copy link
Member

Yes, I don't think there's a way to generically check function calls, but conceptually, this is using some prior knowledge of built-in methods to assume their purity. I'm not sure if putting it as an option makes the most sense—{ ignoreBuiltInPureFunctions: false } which can gradually expand in scope over time? A rule like no-unused-array-method-call certainly seems like an ad-hoc rule for a very specific subset of use cases.

@bradzacher
Copy link
Member

I would say that this still belongs as a separate rule for two reasons:

first it would require types and thus would be a breaking change to introduce it into no-unused-expressions.

second I think we want to leave no-unused-expressions open in case TS adds new syntax in future that could be an unused expression. For example #7748 - the rule should flag Foo<T>.
If we were to introduce type-aware behaviour into the extension rule then we would restrict people from having the basic checks for TS-specific syntax like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@PSeitz @JoshuaKGoldberg @bradzacher @jguddas @Josh-Cena
0