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

🐛 Bug: noInferableTypes removes types from consts where it's not safe #1498

Closed
3 tasks done
rubiesonthesky opened this issue Mar 31, 2024 · 0 comments · Fixed by #1499
Closed
3 tasks done

🐛 Bug: noInferableTypes removes types from consts where it's not safe #1498

rubiesonthesky opened this issue Mar 31, 2024 · 0 comments · Fixed by #1499
Labels
type: bug Something isn't working :( 🐛

Comments

@rubiesonthesky
Copy link
Collaborator

Bug Report Checklist

  • I have tried restarting my IDE and the issue persists.
  • I have pulled the latest main branch of the repository.
  • I have searched for related issues and found none that matched my issue.

Expected

Keep there types

// map
const incompleteTypes: TypeSummariesPerNodeByName = new Map();
// array
const mutations: Mutation[] = [];
// function
export const fixIncompleteImplicitClassGenerics: FileMutator = (
	request: FileMutationsRequest,
) => collectMutationsFromNodes(request, ts.isClassLike, visitClassLike);

Actual

This is the current output

// map
const incompleteTypes = new Map();
// array
const mutations = [];
// function
export const fixIncompleteImplicitClassGenerics = (
	request: FileMutationsRequest,
) => collectMutationsFromNodes(request, ts.isClassLike, visitClassLike);

Additional Info

Continuing dogfooding this repo to TypeStat

typestat.json

{
    "fixes": {
        "noInferableTypes": true
    },
    "include": [
        "src/**/*.{ts,tsx}"
    ],
    "projectPath": "./tsconfig.json"
}

I think the problem is here with this logic. It's not safe to say that you can remove types always from const. It's fine for primitive types like string, number or boolean. But for arrays, maps or sets it may not be safe.

const getNoInferableTypeVariableDeclarationMutation = (
	node: InferableVariableDeclaration,
	request: FileMutationsRequest,
) => {
	if (
		// `const` variables should always have their declarations removed
		tsutils.isNodeFlagSet(node.parent, ts.NodeFlags.Const) ||
		// `let` variables should have only uninformative declarations removed
		declaredInitializedTypeNodeIsRedundant(request, node.type, node.initializer)
	) {
		return createTypeRemovalMutation(request, node);
	}

	return undefined;
};
@rubiesonthesky rubiesonthesky added the type: bug Something isn't working :( 🐛 label Mar 31, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title 🐛 Bug: noInferableTypes removes types where it's not safe 🐛 Bug: noInferableTypes removes types from consts where it's not safe Apr 1, 2024
rubiesonthesky added a commit that referenced this issue Apr 1, 2024
<!-- 👋 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 #1498
- [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.
-->

It's not safe to remove type from const always. I think the mutation
tests agree with me. Some things are marked as "non-inferable" but still
their types were removed.

Co-authored-by: JoshuaKGoldberg <JoshuaKGoldberg>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working :( 🐛
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant