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

[strict-boolean-expressions] False negative for generic parameters #3644

Closed
3 tasks done
RunDevelopment opened this issue Jul 20, 2021 · 3 comments · Fixed by #3981
Closed
3 tasks done

[strict-boolean-expressions] False negative for generic parameters #3644

RunDevelopment opened this issue Jul 20, 2021 · 3 comments · Fixed by #3981
Labels
enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@RunDevelopment
Copy link

  • 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/strict-boolean-expressions": "warn"
  }
}
function test<S>(): void {
	const foo = getEmptyArray<S>();
	if (foo.pop()) {
		throw new Error();
	}

	const bar = getEmptyArray<boolean>();
	if (bar.pop()) {
		throw new Error();
	}
}
function getEmptyArray<S>(): S[] {
	return [];
}

Expected Result

Both if (foo.pop()) and if (bar.pop()) should produce warnings. Both are potentially undefined and most likely not booleans. foo.pop() has the return type S | undefined and bar has number | undefined.

Actual Result

if (foo.pop()) does not produce a warning despite almost certainly not being a boolean.

I found that if the condition is a union with a type parameter (S | anything), then strict-boolean-expressions won't report anything. This is a problem because that means that the rule cannot check all conditions inside generic functions.

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 4.28.4
@typescript-eslint/parser 4.23.0
TypeScript 4.2.4
ESLint 7.26.0
node 14.15.4
@RunDevelopment RunDevelopment added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jul 20, 2021
@bradzacher
Copy link
Member

The rule sees the type S and doesn't know what type it is (because it's an unconstrained generic) - so it just treats it as if it were the object type.
So the return type of foo.pop() is essentially seen as object | undefined.

By the default configuration of the rule, allowNullableObject is turned on - meaning this case is ignored by the rule.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Jul 20, 2021
@RunDevelopment
Copy link
Author

so it just treats it as if it were the object type.

That's a big assumption especially because allowNullableObject is enabled by default. I fixed a bug today that could have been prevented had had this rule not made that assumption.

allowNullableObject is too useful to disable. Could we add something like an option or setting to tell the rule to treat S as unknown instead?

@bradzacher
Copy link
Member

bradzacher commented Jul 20, 2021

I'd be happy if the rule were updated to do the following:

  1. Before calling inspectVariantTypes (

    const types = inspectVariantTypes(tsutils.unionTypeParts(type));
    ), run each type through checker.getBaseConstraintOfType in order to attempt to refine a generic type to its constraint.

  2. In inspectVariantTypes. If one of the types is a generic param (the TypeFlags.TypeVariable flag is set) then handle it appropriately -- eg exclude it from this check, and push a new "variant type" to the set

  3. add handling to checkNode so that it appropriately handles the new "variant type".

no need for an option or anything here, as I think this is just a logical improvement to the rule's logic.

@bradzacher bradzacher added enhancement New feature or request and removed awaiting response Issues waiting for a reply from the OP or another party labels Jul 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants