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.

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

fix: streamline type collection by removing 'flags' #2025

Merged
merged 2 commits into from
Nov 30, 2024
Merged

fix: streamline type collection by removing 'flags' #2025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c5c747
fix: streamline type collection by removing 'flags'
JoshuaKGoldberg Nov 30, 2024
45087d8
chore: fix linting
JoshuaKGoldberg Nov 30, 2024
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
40 changes: 0 additions & 40 deletions src/mutations/aliasing/aliases.ts
View file
Open in desktop

This file was deleted.

9 changes: 1 addition & 8 deletions src/mutations/aliasing/joinIntoType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,15 @@
import ts from "typescript";

import { isNotUndefined, uniquify } from "../../shared/arrays.js";
import { uniquify } from "../../shared/arrays.js";
import { FileMutationsRequest } from "../../shared/fileMutator.js";
import { getApplicableTypeAliases } from "./aliases.js";

export const joinIntoType = (
flags: ReadonlySet<ts.TypeFlags>,
types: ReadonlySet<ts.Type>,
request: FileMutationsRequest,
) => {
const alias = getApplicableTypeAliases(request);

return uniquify(
...Array.from(types)
.map((type) => request.services.printers.type(type))
.map((type) => (type.includes("=>") ? `(${type})` : type)),
...Array.from(flags)
.map((flag) => alias.get(flag))
.filter(isNotUndefined),
).join(" | ");
};
88 changes: 32 additions & 56 deletions src/mutations/collecting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,21 @@ import ts from "typescript";
import { FileMutationsRequest } from "../shared/fileMutator.js";
import { isKnownGlobalBaseType } from "../shared/nodeTypes.js";
import { setSubtract } from "../shared/sets.js";
import { getApplicableTypeAliases } from "./aliasing/aliases.js";
import {
findMissingFlags,
isTypeFlagSetRecursively,
} from "./collecting/flags.js";
import { isTypeFlagSetRecursively } from "./collecting/flags.js";

/**
* Collects assigned and missing flags and types, recursively accounting for type unions.
* Collects assigned and missing types, recursively accounting for type unions.
* @param request Metadata and settings to collect mutations in a file.
* @param declaredType Original type declared on a node.
* @param allAssignedTypes All types immediately or later assigned to the node.
*/
export const collectUsageFlagsAndSymbols = (
export const collectUsageSymbols = (
request: FileMutationsRequest,
declaredType: ts.Type,
allAssignedTypes: readonly ts.Type[],
) => {
// Collect which flags are later assigned to the type
const [assignedFlags, assignedTypes] = collectFlagsAndTypesFromTypes(
request,
allAssignedTypes,
);
// Collect which types are later assigned to the type
const assignedTypes = collectRawTypesFromTypes(request, allAssignedTypes);

// If the declared type is the general 'any', then we assume all are missing
// Similarly, if it's a plain Function or Object, we'll want to replace its contents
Expand All @@ -34,81 +27,52 @@ export const collectUsageFlagsAndSymbols = (
isKnownGlobalBaseType(declaredType)
) {
return {
assignedFlags,
assignedTypes,
missingFlags: assignedFlags,
missingTypes: assignedTypes,
};
}

// Otherwise, collect which flags and types are declared (as a type annotation)...
const [declaredFlags, declaredTypes] = collectFlagsAndTypesFromTypes(
request,
[declaredType],
);
// Otherwise, collect which types are declared (as a type annotation)...
const declaredTypes = collectRawTypesFromTypes(request, [declaredType]);

// Subtract the above to find any flags or types assigned but not declared
// Subtract the above to find any types assigned but not declared
return {
assignedFlags,
assignedTypes,
missingFlags: findMissingFlags(declaredType, assignedFlags, declaredFlags),
missingTypes: findMissingTypes(request, assignedTypes, declaredTypes),
};
};

/**
* Separates raw type node(s) into their contained flags and types.
* Separates raw type node(s) into their contained types.
* @param request Metadata and settings to collect mutations in a file.
* @param allTypes Any number of raw type nodes.
* @param allowStrictNullCheckAliases Whether to allow `null` and `undefined` aliases regardless of compiler strictness.
* @returns Flags and types found within the raw type nodes.
* @returns Types found within the raw type nodes.
*/
export const collectFlagsAndTypesFromTypes = (
export const collectRawTypesFromTypes = (
request: FileMutationsRequest,
allTypes: readonly ts.Type[],
allowStrictNullCheckAliases?: boolean,
): [Set<ts.TypeFlags>, Set<ts.Type>] => {
const foundFlags = new Set<ts.TypeFlags>();
): Set<ts.Type> => {
const foundTypes = new Set<ts.Type>();
const applicableTypeAliases = getApplicableTypeAliases(
request,
allowStrictNullCheckAliases,
);

// Scan each type for undeclared type additions
for (const type of allTypes) {
// For any simple type flag we later will care about for aliasing, add it if it's in the type
for (const [typeFlag] of applicableTypeAliases) {
if (isTypeFlagSetRecursively(type, typeFlag)) {
foundFlags.add(typeFlag);
}
}

// If the type is a rich type (has a symbol), add it in directly
if (type.getSymbol() !== undefined) {
foundTypes.add(type);
continue;
}

// If the type is a union, add any flags or types found within it
// If the type is a union, add any types found within it
if (tsutils.isUnionType(type)) {
const subTypes = recursivelyCollectSubTypes(type);
const [subFlags, deepSubTypes] = collectFlagsAndTypesFromTypes(
request,
subTypes,
);

for (const subFlag of subFlags) {
foundFlags.add(subFlag);
}
const deepSubTypes = collectRawTypesFromTypes(request, subTypes);

for (const deepSubType of deepSubTypes) {
foundTypes.add(deepSubType);
}

continue;
}

// Otherwise, it's likely either an intrinsic, primitive, or a shape
foundTypes.add(type);
}

return [foundFlags, foundTypes];
return foundTypes;
};

export const recursivelyCollectSubTypes = (type: ts.UnionType): ts.Type[] => {
Expand Down Expand Up @@ -149,7 +113,14 @@ const findMissingTypes = (
return false;
}

// For each potential missing type:
for (const potentialParentType of declaredTypes) {
// If the potential parent type is unknown, then ignore it
if (potentialParentType.flags === ts.TypeFlags.Unknown) {
continue;
}

// If the assigned type is assignable to it, then it's a no
if (
request.services.program
.getTypeChecker()
Expand All @@ -163,6 +134,11 @@ const findMissingTypes = (
};

for (const assignedType of assignedTypes) {
// The 'void' type shouldn't be assigned to anything, so we ignore it
if (assignedType.flags === ts.TypeFlags.Void) {
remainingMissingTypes.delete(assignedType);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the next if be converted to an else-if, or does isAssignedTypeMissingFromDeclared need to run for side-effects?

Copy link
Owner Author

Choose a reason for hiding this comment

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

That might be a good first contribution for you 😄 do you see any buggy behavior from it?

Copy link
Contributor

Choose a reason for hiding this comment

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

My primary concern was efficiency (it always starts with a single if-statement in a loop, but then one more if-statement is added, and then one more, and suddenly the short and fast loop is not so short and fast any longer).

But there's also a readability aspect, with an else-if it's clear that either isAssignedTypeMissingFromDeclared doesn't have any important side-effects, or that things are really intricate and any minor change might break things.


if (!isAssignedTypeMissingFromDeclared(assignedType)) {
remainingMissingTypes.delete(assignedType);
}
Expand Down
42 changes: 0 additions & 42 deletions src/mutations/collecting/flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,6 @@
import * as tsutils from "ts-api-utils";
import ts from "typescript";

import { setSubtract } from "../../shared/sets.js";

const knownTypeFlagEquivalents = new Map([
[ts.TypeFlags.BigInt, ts.TypeFlags.BigIntLiteral],
[ts.TypeFlags.BigIntLiteral, ts.TypeFlags.BigInt],
[ts.TypeFlags.Boolean, ts.TypeFlags.BooleanLiteral],
[ts.TypeFlags.BooleanLiteral, ts.TypeFlags.Boolean],
[ts.TypeFlags.Number, ts.TypeFlags.NumberLiteral],
[ts.TypeFlags.NumberLiteral, ts.TypeFlags.Number],
[ts.TypeFlags.String, ts.TypeFlags.StringLiteral],
[ts.TypeFlags.StringLiteral, ts.TypeFlags.String],
[ts.TypeFlags.Undefined, ts.TypeFlags.Void],
[ts.TypeFlags.Void, ts.TypeFlags.Undefined],
]);

export const findMissingFlags = (
declaredType: ts.Type,
assignedFlags: ReadonlySet<ts.TypeFlags>,
declaredFlags: ReadonlySet<ts.TypeFlags>,
): Set<ts.TypeFlags> => {
// If the type is declared to allow `any`, it can't be missing anything
if (isTypeFlagSetRecursively(declaredType, ts.TypeFlags.Any)) {
return new Set();
}

// Otherwise, it's all the flags assigned to it that weren't already declared
const missingFlags = setSubtract(assignedFlags, declaredFlags);

// Remove any flags that are just equivalents of the existing ones
// For example, initial presence of `void` makes `undefined` unnecessary, and vice versa
for (const [original, equivalent] of knownTypeFlagEquivalents) {
if (
missingFlags.has(equivalent) &&
isTypeFlagSetRecursively(declaredType, original)
) {
missingFlags.delete(equivalent);
}
}

return missingFlags;
};

/**
* Checks if a type contains a type flag, accounting for deep nested type unions.
* @param parentType Parent type to check for the type flag.
Expand Down
23 changes: 13 additions & 10 deletions src/mutations/creators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
isKnownGlobalBaseType,
} from "../shared/nodeTypes.js";
import { joinIntoType } from "./aliasing/joinIntoType.js";
import { collectUsageFlagsAndSymbols } from "./collecting.js";
import { collectUsageSymbols } from "./collecting.js";

/**
* Creates a mutation to add types to an existing type, if any are new.
Expand All @@ -30,20 +30,20 @@
return undefined;
}

// Find any missing flags and symbols (a.k.a. types)
const { missingFlags, missingTypes } = collectUsageFlagsAndSymbols(
// Find any missing symbols (a.k.a. types)
const { missingTypes } = collectUsageSymbols(
request,
declaredType,
allAssignedTypes,
);

// If nothing is missing, rejoice! The type was already fine.
if (missingFlags.size === 0 && missingTypes.size === 0) {
if (missingTypes.size === 0) {
return undefined;
}

// Join the missing types into a type string to declare
const newTypeAlias = joinIntoType(missingFlags, missingTypes, request);
const newTypeAlias = joinIntoType(missingTypes, request);

// If the original type was a bottom type or just something like Function or Object, replace it entirely
if (
Expand Down Expand Up @@ -87,17 +87,20 @@
declaredType: ts.Type,
allAssignedTypes: readonly ts.Type[],
): TextInsertMutation | undefined => {
// Find the already assigned flags and symbols, as well as any missing ones
const { assignedFlags, assignedTypes, missingFlags, missingTypes } =
collectUsageFlagsAndSymbols(request, declaredType, allAssignedTypes);
// Find the already assigned symbols, as well as any missing ones
const { assignedTypes, missingTypes } = collectUsageSymbols(
request,
declaredType,
allAssignedTypes,
);

Check warning on line 95 in src/mutations/creators.ts

View check run for this annotation

Codecov / codecov/patch

src/mutations/creators.ts#L90-L95 

Added lines #L90 - L95 were not covered by tests

// If nothing is missing, rejoice! The type was already fine.
if (missingFlags.size === 0 && missingTypes.size === 0) {
if (missingTypes.size === 0) {

Check warning on line 98 in src/mutations/creators.ts

View check run for this annotation

Codecov / codecov/patch

src/mutations/creators.ts#L98 

Added line #L98 was not covered by tests
return undefined;
}

// Join the missing types into a type string to declare
const newTypeAlias = joinIntoType(assignedFlags, assignedTypes, request);
const newTypeAlias = joinIntoType(assignedTypes, request);

Check warning on line 103 in src/mutations/creators.ts

View check run for this annotation

Codecov / codecov/patch

src/mutations/creators.ts#L103 

Added line #L103 was not covered by tests

// Create a mutation insertion that adds the assigned types in
return {
Expand Down
Loading