Skip to content

Commit

Permalink
fix: no longer removing Map or Set type when they are needed (#1520)
Browse files Browse the repository at this point in the history 
<!-- 👋 Hi, thanks for sending a PR to TypeStat! 💖.
Please fill out all fields below and make sure each item is true and [x]
checked.
Otherwise we may not be able to review your PR. -->

## PR Checklist

- [x] Addresses an existing open issue: fixes #1512
- [x] That issue was marked as [`status: accepting
prs`](https://github.com/JoshuaKGoldberg/TypeStat/issues?q=is%3Aopen+is%3Aissue+label%3A%22status%3A+accepting+prs%22)
- [x] Steps in
[CONTRIBUTING.md](https://github.com/JoshuaKGoldberg/TypeStat/blob/main/.github/CONTRIBUTING.md)
were taken

## Overview

<!-- Description of what is changed and how the code change does that.
-->

I'm not sure that this is correct place to place this code. It's also
maybe too heavy handed. But on the other hand, I think it's better to
prevent cases where we introduce implicit anys when there was some type
earlier. Keeping types that could have been removed, is less bad in that
case.

I have tried to find what would be right place to do this check but I
did not have any luck.

---------

Co-authored-by: Josh Goldberg ✨ <[email protected]>
  • Loading branch information
@rubiesonthesky @JoshuaKGoldberg
rubiesonthesky and JoshuaKGoldberg authored Apr 23, 2024
1 parent 8985f8f commit d27679b
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 34 deletions.
122 changes: 92 additions & 30 deletions src/shared/comparisons.ts
View file
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 => {
// 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)) {
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" ||
// 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) {
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
View file
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
30 changes: 29 additions & 1 deletion test/cases/fixes/noInferableTypes/variableDeclarations/expected.ts
View file
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();
const anyArray: any[] = [];
})();
28 changes: 28 additions & 0 deletions test/cases/fixes/noInferableTypes/variableDeclarations/original.ts
View file
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[] = [];
})();

0 comments on commit d27679b

Please sign in to comment.