Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: [no-unnecessary-condition] use assignability APIs in no-unnecessary-condition (POC)#10378

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
kirkwaiblinger wants to merge6 commits intotypescript-eslint:main
base:main
Choose a base branch
Loading
fromkirkwaiblinger:nuc-assignability-3
Draft
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -107,7 +107,7 @@ function assert(condition: unknown): asserts condition {

assert(false); // Unnecessary; condition is always falsy.

const neverNull = {};
const neverNull = { someProperty: 'someValue'};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That's due to TS's (questionable?) decision to giveneverNull type{} instead of typeobject, even when initializing aconst variable.

😩 this again... is there a TypeScript issue filed that we can reference? It feels like a bug to me.

I also don't mind it very much. It's not often that folks use{}. This docs change kind of makes it more realistic to me.

kirkwaiblinger reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

😩 this again... is there a TypeScript issue filed that we can reference? It feels like a bug to me.

That's a good question. I'll take an action item to investigate this.

I also don't mind it very much. It's not often that folks use{}. This docs change kind of makes it more realistic to me.

Agreed that the docs change is probably an improvement. But, the false negative here makes the rule a little less understandable to me, so I'd lean on the side of wanting it to be correct if we can get it to be.

JoshuaKGoldberg reacted with thumbs up emoji
Copy link
MemberAuthor

@kirkwaiblingerkirkwaiblingerNov 24, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh, dear, the moment I start playing around with this, I immediately find infuriating behavior around{}....

functiontakesObject(x:object){if(x){console.log('should always be true')}else{console.error('this should be unreachable');}}consto:{}=0;// no error here!?!?!?!?takesObject(o);

I hate the{} type so much.

JoshuaKGoldberg reacted with laugh emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Regardingconst o = {}; // has type {}, started discord conversation athttps://discord.com/channels/508357248330760243/508357638677856287/1310419206260654081.

Regarding{} ->object assignments, filedmicrosoft/TypeScript#60582... and immediately learned this is a duplicate ofmicrosoft/TypeScript#56205, in which this behavior is deemed intentional 🫤.

Reminder to self: I'm pretty sure we'll have some good info to add to the motivation forhttps://typescript-eslint.io/rules/no-empty-object-type/ as an outcome of these discussions 😆

JoshuaKGoldberg reacted with laugh emoji
assert(neverNull); // Unnecessary; condition is always truthy.

function isString(value: unknown): value is string {
Expand Down
102 changes: 73 additions & 29 deletionspackages/eslint-plugin/src/rules/no-unnecessary-condition.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,41 +41,36 @@ const getValueOfLiteralType = (
return type.value;
};

const isFalsyBigInt = (type: ts.Type): boolean => {
return (
tsutils.isLiteralType(type) &&
valueIsPseudoBigInt(type.value) &&
!getValueOfLiteralType(type)
);
};
const isTruthyLiteral = (type: ts.Type): boolean =>
tsutils.isTrueLiteralType(type) ||
(type.isLiteral() && !!getValueOfLiteralType(type));

const isPossiblyFalsy = (type: ts.Type): boolean =>
const isPossiblyFalsy = (
checker: ts.TypeChecker,
type: ts.Type,
falsyTypes: ts.Type[],
): boolean =>
tsutils
.unionTypeParts(type)
// Intersections like `string & {}` can also be possibly falsy,
// requiring us to look into the intersection.
.flatMap(type => tsutils.intersectionTypeParts(type))
// PossiblyFalsy flag includes literal values, so exclude ones that
// are definitely truthy
.filter(t => !isTruthyLiteral(t))
.some(type => isTypeFlagSet(type, ts.TypeFlags.PossiblyFalsy));
.some(type =>
falsyTypes.some(falsyType => checker.isTypeAssignableTo(falsyType, type)),
);

const isPossiblyTruthy = (type: ts.Type): boolean =>
const isPossiblyTruthy = (
checker: ts.TypeChecker,
type: ts.Type,
falsyTypes: ts.Type[],
): boolean =>
tsutils
.unionTypeParts(type)
.map(type => tsutils.intersectionTypeParts(type))
.map(unionPart => tsutils.intersectionTypeParts(unionPart))
.some(intersectionParts =>
// It is possible to define intersections that are always falsy,
// like `"" & { __brand: string }`.
intersectionParts.every(
type =>
!tsutils.isFalsyType(type) &&
// below is a workaround for ts-api-utils bug
// see https://github.com/JoshuaKGoldberg/ts-api-utils/issues/544
!isFalsyBigInt(type),
// It is possible to define intersections that are always falsy,
// like `"" & { __brand: string }`.
intersectionPart =>
!falsyTypes.some(falsyType =>
checker.isTypeAssignableTo(intersectionPart, falsyType),
),
),
);

Expand DownExpand Up@@ -167,6 +162,49 @@ function booleanComparison(
}
}

function getFalsyTypes(checker: ts.TypeChecker) {
return [
{
type: checker.getNullType(),
value: null,
},
{
type: checker.getUndefinedType(),
value: undefined,
},
{
type: checker.getFalseType(),
value: false,
},
{
type: checker.getStringLiteralType(''),
value: '',
},
{
type: checker.getNumberLiteralType(0),
value: 0,
},
// i honestly don't think this matters, but idk :shrug:
{
type: checker.getNumberLiteralType(-0),
value: -0,
},
// this doesn't seem to have any effect. I don't think you can create a NaN literal type in TS.
{
type: checker.getNumberLiteralType(NaN),
value: NaN,
},
// not available in all supported versions; see https://github.com/microsoft/TypeScript/issues/58563
{
type: checker.getBigIntLiteralType({ base10Value: '0', negative: false }),
value: 0n,
},
] as const satisfies {
type: ts.Type;
value: unknown;
}[];
}

// #endregion

export type Options = [
Expand DownExpand Up@@ -287,6 +325,8 @@ export default createRule<Options, MessageId>({
});
}

const falsyTypes = getFalsyTypes(checker).map(({ type }) => type);

function nodeIsArrayType(node: TSESTree.Expression): boolean {
const nodeType = getConstrainedTypeAtLocation(services, node);
return tsutils
Expand DownExpand Up@@ -399,9 +439,9 @@ export default createRule<Options, MessageId>({

if (isTypeFlagSet(type, ts.TypeFlags.Never)) {
messageId = 'never';
} else if (!isPossiblyTruthy(type)) {
} else if (!isPossiblyTruthy(checker,type, falsyTypes)) {
messageId = !isUnaryNotArgument ? 'alwaysFalsy' : 'alwaysTruthy';
} else if (!isPossiblyFalsy(type)) {
} else if (!isPossiblyFalsy(checker,type, falsyTypes)) {
messageId = !isUnaryNotArgument ? 'alwaysTruthy' : 'alwaysFalsy';
}

Expand DownExpand Up@@ -655,13 +695,17 @@ export default createRule<Options, MessageId>({
if (returnTypes.some(t => isTypeAnyType(t) || isTypeUnknownType(t))) {
return;
}
if (!returnTypes.some(isPossiblyFalsy)) {
if (
!returnTypes.some(type => isPossiblyFalsy(checker, type, falsyTypes))
) {
return context.report({
node: callback,
messageId: 'alwaysTruthyFunc',
});
}
if (!returnTypes.some(isPossiblyTruthy)) {
if (
!returnTypes.some(type => isPossiblyTruthy(checker, type, falsyTypes))
) {
return context.report({
node: callback,
messageId: 'alwaysFalsyFunc',
Expand Down

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -735,6 +735,7 @@ if (x) {
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: true,
},
],
skip: true,
},
`
interface Foo {
Expand DownExpand Up@@ -982,6 +983,44 @@ isString('falafel');
`,
options: [{ checkTypePredicates: true }],
},
{
// {} can be falsy, so this is fine
code: `
declare let foo: {};
foo &&= 1;
`,
},
{
// { toFixed(): string } can be falsy since 0 is assignable, so this is fine
code: `
declare let foo: { toFixed(): string };
foo &&= 1;
`,
},
{
code: `
function usesEmptyObject(x: {}) {
if (x) {
}
}
`,
},
{
code: `
function usesEmptyObject(x: { toFixed(): string }) {
if (x) {
}
}
`,
},
{
code: `
function usesEmptyObject(x: { toString(): string }) {
if (x) {
}
}
`,
},
],

invalid: [
Expand DownExpand Up@@ -1033,7 +1072,8 @@ switch (b1) {
unnecessaryConditionTest('"always truthy"', 'alwaysTruthy'),
unnecessaryConditionTest(`undefined`, 'alwaysFalsy'),
unnecessaryConditionTest('null', 'alwaysFalsy'),
unnecessaryConditionTest('void', 'alwaysFalsy'),
// generated code is a TS error (void cannot be tested for truthiness)
// unnecessaryConditionTest('void', 'alwaysFalsy'),
unnecessaryConditionTest('never', 'never'),
unnecessaryConditionTest('string & number', 'never'),
// More complex logical expressions
Expand DownExpand Up@@ -1645,7 +1685,7 @@ if (arr.filter) {
function truthy() {
return [];
}
function falsy() {}
function falsy(): undefined {}
[1, 3, 5].filter(truthy);
[1, 2, 3].find(falsy);
[1, 2, 3].findLastIndex(falsy);
Expand DownExpand Up@@ -2318,6 +2358,7 @@ if (x) {
tsconfigRootDir: path.join(rootPath, 'unstrict'),
},
},
skip: true,
},
{
code: `
Expand DownExpand Up@@ -2442,8 +2483,8 @@ foo ??= null;
},
{
code: `
declare let foo: {};
foo ||=1;
declare let foo: { bar: null};
foo ||={ bar: null };
`,
errors: [
{
Expand DownExpand Up@@ -2472,8 +2513,8 @@ foo ||= null;
},
{
code: `
declare let foo: {};
foo &&=1;
declare let foo: { bar: string};
foo &&={ bar: 'none' };
`,
errors: [
{
Expand DownExpand Up@@ -2766,5 +2807,18 @@ isString('fa' + 'lafel');
'((string & { __brandA: string }) | (number & { __brandB: string })) & ("foo" | 123)',
'alwaysTruthy',
),
{
code: `
const neverNull: object = {};
if (neverNull) {
}
`,
errors: [
{
line: 3,
messageId: 'alwaysTruthy',
},
],
},
],
});
Loading
[8]ページ先頭
©2009-2025 Movatter.jp