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

fix: no longer removing Map or Set type when they are needed #1520

Merged
122 changes: 92 additions & 30 deletions src/shared/comparisons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@ import * as tsutils from "ts-api-utils";
import ts from "typescript";

import { FileMutationsRequest } from "./fileMutator.js";
import { isIntrinsicNameType, isTypeArgumentsType } from "./typeNodes.js";
import {
isIntrinsicNameType,
isOptionalTypeArgumentsTypeNode,
} from "./typeNodes.js";
import { getTypeAtLocationIfNotError } from "./types.js";
getTypeAtLocationIfNotError,
getTypeAtLocationIfNotErrorWithChecker,
isTypeBuiltIn,
} from "./types.js";

export const declaredInitializedTypeNodeIsRedundant = (
request: FileMutationsRequest,
declaration: ts.TypeNode,
initializer: ts.Node,
) => {
): boolean | undefined => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder should this method always return boolean? From the usage, it's always used in if check, so it does not really matter that it also returns undefined. But on the other hand, it could return false in those cases.

Copy link
Owner

Choose a reason for hiding this comment

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

🤷 I don't have a hard preference either way 😄

// Most literals (e.g. `""`) have a corresponding keyword (e.g. `string`)
switch (declaration.kind) {
case ts.SyntaxKind.BooleanKeyword:
Expand All @@ -36,14 +37,8 @@ export const declaredInitializedTypeNodeIsRedundant = (
}

// `RegExp`s are also initializers that one should never reassign
if (
ts.isTypeReferenceNode(declaration) &&
ts.isIdentifier(declaration.typeName)
) {
switch (declaration.typeName.text) {
case "RegExp":
return initializer.kind === ts.SyntaxKind.RegularExpressionLiteral;
}
if (ts.isRegularExpressionLiteral(declaration)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not related to the issue, but I think it's easier to read and understand this way.

Copy link
Owner

Choose a reason for hiding this comment

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

No preference on my end 😄

return ts.isRegularExpressionLiteral(initializer);
}

// Other types are complex enough to need the type checker...
Expand All @@ -52,23 +47,48 @@ export const declaredInitializedTypeNodeIsRedundant = (
return undefined;
}

const declaredText = declaredType.getSymbol()?.getEscapedName();

// ReadonlySet is type-only, it can't be initialized. It should be never be removed.
// eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
if (declaredText === "ReadonlySet" && isTypeBuiltIn(declaredType)) {
return false;
}

const initializedType = getTypeAtLocationIfNotError(request, initializer);
if (initializedType === undefined) {
return undefined;
}

return declaredTypeIsEquivalent(
request.services.program.getTypeChecker(),
declaredType,
initializedType,
);
const typeChecker = request.services.program.getTypeChecker();

// This is brute-force way to keep Map<string, number> comparison to Map without type arguments...
// This will allow many type declarations that could be removed.
if (
// eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
(declaredText === "Map" ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
declaredText === "Set") &&
isTypeBuiltIn(initializedType) &&
typeChecker.typeToString(declaredType) !=
typeChecker.typeToString(initializedType)
) {
return false;
}

// eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
if (declaredText === "Set" && isTypeBuiltIn(initializedType)) {
return typeIsEquivalentForSet(typeChecker, declaration, initializer);
}

return declaredTypeIsEquivalent(typeChecker, declaredType, initializedType);
};

const declaredTypeIsEquivalent = (
typeChecker: ts.TypeChecker,
declaredType: ts.Type,
initializedType: ts.Type,
) => {
): boolean => {
// Most types, such as `string[]` / `[""]`, are generally found by this intersection...
if (
typeChecker.isTypeAssignableTo(declaredType, initializedType) &&
Expand Down Expand Up @@ -105,20 +125,13 @@ const typeSymbolsAndArgumentsAreEquivalent = (
typeChecker: ts.TypeChecker,
declaredType: ts.Type,
initializedType: ts.Type,
) => {
): boolean => {
const declaredSymbol = declaredType.getSymbol();
const initializedSymbol = initializedType.getSymbol();
if (
declaredSymbol !== initializedSymbol ||
!isOptionalTypeArgumentsTypeNode(declaredType) ||
declaredType.typeArguments === undefined ||
!isOptionalTypeArgumentsTypeNode(initializedType) ||
initializedType.typeArguments === undefined
) {
return false;
}

if (
!isTypeArgumentsType(declaredType) ||
!isTypeArgumentsType(initializedType) ||
declaredType.typeArguments.length !== initializedType.typeArguments.length
) {
return false;
Expand Down Expand Up @@ -148,7 +161,7 @@ const typeSymbolsAndArgumentsAreEquivalent = (
const intrinsicNamesAreEquivalent = (
declaredName: string,
initializedName: string,
) => {
): boolean => {
switch (declaredName) {
case "boolean":
return initializedName === "false" || initializedName === "true";
Expand All @@ -159,3 +172,52 @@ const intrinsicNamesAreEquivalent = (

return false;
};

/**
* TypeChecker completes types for some nodes. So we need to check the equivalence
* from the node level, not from their types.
*/
const typeIsEquivalentForSet = (
typeChecker: ts.TypeChecker,
declaration: ts.TypeNode,
initializer: ts.Node,
) => {
const declarationTypeArguments = ts.isTypeReferenceNode(declaration)
? declaration.typeArguments
: undefined;
const initializerTypeArguments = ts.isNewExpression(initializer)
? initializer.typeArguments
: undefined;

if (
!declarationTypeArguments?.length ||
declarationTypeArguments.length !== initializerTypeArguments?.length
) {
return false;
}

for (let i = 0; i < declarationTypeArguments.length; i += 1) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There should be only one type argument in Set, but I copied this code from above.

const declaredTypeArgument = getTypeAtLocationIfNotErrorWithChecker(
typeChecker,
declarationTypeArguments[i],
);
const initializedTypeArgument = getTypeAtLocationIfNotErrorWithChecker(
typeChecker,
initializerTypeArguments[i],
);

if (
!declaredTypeArgument ||
!initializedTypeArgument ||
!declaredTypeIsEquivalent(
typeChecker,
declaredTypeArgument,
initializedTypeArgument,
)
) {
return false;
}
}

return true;
};
18 changes: 15 additions & 3 deletions src/shared/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,21 @@ export const getTypeAtLocationIfNotError = (
return undefined;
}

const type = request.services.program
.getTypeChecker()
.getTypeAtLocation(node);
return getTypeAtLocationIfNotErrorWithChecker(
request.services.program.getTypeChecker(),
node,
);
};

export const getTypeAtLocationIfNotErrorWithChecker = (
typeChecker: ts.TypeChecker,
node: ts.Node | undefined,
): ts.Type | undefined => {
if (node === undefined) {
return undefined;
}

const type = typeChecker.getTypeAtLocation(node);

return isIntrinsicNameType(type) && type.intrinsicName === "error"
? undefined
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@
let letInferableNullOrStrings = [null, ""];
let letInferableNumberOrRegExps = [0, /./];

// Non-inferable const arrays
const constNonInferableStringArray: string[] = [];

// Non-inferable const multi-type arrays
const constNonInferableNullOrStrings: (null | string)[] = [null];
const constNonInferableNumberOrRegExps: (number | RegExp)[] = [0];
Expand Down Expand Up @@ -87,7 +90,19 @@
// should keep their types
// map
type TypeSummariesPerNodeByName = Map<string, number>;
const incompleteTypes = new Map();
const incompleteTypes: TypeSummariesPerNodeByName = new Map();
const mapWithoutRightSide: Map<string, string> = new Map();
const stringSet: Set<string> = new Set();
const stringReadonlySet: ReadonlySet<string> = new Set();
const stringSetWithInitialValue: Set<string> = new Set([""]);
const stringSetWithInitialValueAndTypes = new Set<string>([""]);
const stringOrNumberSet: Set<string | number> = new Set();
const stringOrNumberSet2: Set<string | number> = new Set<number>();
const copySet: ReadonlySet<Parent> = new Set<Parent>();
let letStringMapTyped: Map<string, string | number> = new Map<
string,
number
>();
// array
interface Mutation {
readonly range: number;
Expand All @@ -102,4 +117,17 @@
const fixIncompleteImplicitClassGenerics: FileMutator = (
request: FileMutationsRequest,
) => undefined;
class MyMap<K, V> {
//
}

// should lose their types
const incompleteTypes2: TypeSummariesPerNodeByName = new Map<
string,
number
>();
const stringMapTyped = new Map<string, number>();
const anyMap = new Map();
const anySet: Set<any> = new Set();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could lose it's type declaration. But I think it would need it's own custom logic for this special case.

const anyArray: any[] = [];
})();
28 changes: 28 additions & 0 deletions test/cases/fixes/noInferableTypes/variableDeclarations/original.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@
let letInferableNullOrStrings: (null | string)[] = [null, ""];
let letInferableNumberOrRegExps: (number | RegExp)[] = [0, /./];

// Non-inferable const arrays
const constNonInferableStringArray: string[] = [];

// Non-inferable const multi-type arrays
const constNonInferableNullOrStrings: (null | string)[] = [null];
const constNonInferableNumberOrRegExps: (number | RegExp)[] = [0];
Expand Down Expand Up @@ -88,6 +91,18 @@
// map
type TypeSummariesPerNodeByName = Map<string, number>;
const incompleteTypes: TypeSummariesPerNodeByName = new Map();
const mapWithoutRightSide: Map<string, string> = new Map();
const stringSet: Set<string> = new Set();
const stringReadonlySet: ReadonlySet<string> = new Set();
const stringSetWithInitialValue: Set<string> = new Set([""]);
const stringSetWithInitialValueAndTypes: Set<string> = new Set<string>([""]);
const stringOrNumberSet: Set<string | number> = new Set();
const stringOrNumberSet2: Set<string | number> = new Set<number>();
const copySet: ReadonlySet<Parent> = new Set<Parent>();
let letStringMapTyped: Map<string, string | number> = new Map<
string,
number
>();
// array
interface Mutation {
readonly range: number;
Expand All @@ -102,4 +117,17 @@
const fixIncompleteImplicitClassGenerics: FileMutator = (
request: FileMutationsRequest,
) => undefined;
class MyMap<K, V> {
//
}

// should lose their types
const incompleteTypes2: TypeSummariesPerNodeByName = new Map<
string,
number
>();
const stringMapTyped: Map<string, number> = new Map<string, number>();
const anyMap: Map<any, any> = new Map();
const anySet: Set<any> = new Set();
const anyArray: any[] = [];
})();
Loading