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

rubiesonthesky · 2024-04-03T19:01:39Z

PR Checklist

Addresses an existing open issue: fixes 🐛 Bug: noInferableTypes removes incorrectly type from Map variable #1512
That issue was marked as status: accepting prs
Steps in CONTRIBUTING.md were taken

Overview

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.

codecov · 2024-04-03T19:47:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.68%. Comparing base (e484ffc) to head (47a9292).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1520      +/-   ##
==========================================
+ Coverage   78.62%   78.68%   +0.05%     
==========================================
  Files         175      175              
  Lines       10957    10968      +11     
  Branches     1021     1026       +5     
==========================================
+ Hits         8615     8630      +15     
+ Misses       2338     2334       -4     
  Partials        4        4

Flag	Coverage Δ
mutation	`73.31% <100.00%> (+0.06%)`	⬆️
unit	`28.34% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoshuaKGoldberg

I think this is a good start but there are some tricky edge cases that I worry might make this a lot more complex than it seems. Good luck! ✨

src/shared/comparisons.ts

JoshuaKGoldberg · 2024-04-09T14:05:27Z

test/cases/fixes/noInferableTypes/variableDeclarations/original.ts

[Testing] A few more cases come to mind...

let stringMapTyped: Map<string, string | number> = new Map<string, number>();: should keep the type annotation, I think?

ReadonlyMap

Custom generic classes like class MyMap<K, V> { ... }

type aliases

Nice! thanks. And I agree this is more complex, I just touched surface. 😅

Co-authored-by: Josh Goldberg ✨ <[email protected]>

rubiesonthesky · 2024-04-10T20:12:44Z

Comparing ts.Types of code like this const stringSet: Set<string> = new Set(); does not work. Both sides have same type arguments. But if we compare the actual nodes and their type arguments, then we can find differences. In most cases, you want to compare the types typeChecker has inferred for you, but this seems special case in that regard.

If you look this tool https://ts-ast-viewer.com/#code/MYewdgzgLgBNBOBLMBzAygUygLhpqAPAsigHwwC8MYGA7nlgBQCUA3EA and then compare the inferred type of NewExpression and TypeReference, you can see that both have Set<string>. And that is where this bug comes from.

For Map this is different. Same comparisation in that tool tells us that the types are Map<string, string> and Map<any, any>.

rubiesonthesky · 2024-04-11T17:40:48Z

src/shared/comparisons.ts

-			case "RegExp":
-				return initializer.kind === ts.SyntaxKind.RegularExpressionLiteral;
-		}
+	if (ts.isRegularExpressionLiteral(declaration)) {


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

No preference on my end 😄

rubiesonthesky · 2024-04-11T17:49:44Z

src/shared/comparisons.ts

+		return false;
+	}
+
+	for (let i = 0; i < declarationTypeArguments.length; i += 1) {


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

rubiesonthesky · 2024-04-11T17:53:01Z

src/shared/comparisons.ts


 export const declaredInitializedTypeNodeIsRedundant = (
 	request: FileMutationsRequest,
 	declaration: ts.TypeNode,
 	initializer: ts.Node,
-) => {
+): boolean | undefined => {


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.

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

rubiesonthesky · 2024-04-11T17:56:24Z

test/cases/fixes/noInferableTypes/variableDeclarations/expected.ts

+	>();
+	const stringMapTyped = new Map<string, number>();
+	const anyMap = new Map();
+	const anySet: Set<any> = new Set();


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

src/shared/comparisons.ts

rubiesonthesky · 2024-04-13T20:49:27Z

I added isTypeBuiltIn check for Map, Set and Readonly. I'm not totally sure why it would matter here, but I guess it can be a safe guard in future.

JoshuaKGoldberg

Looks great to me, thanks!

I've context switched away from TypeStat for a bit to focus on ESLint stuff. Sorry that reviewing is taking a while. But I'm still very excited you're looking at all this! 🙌

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

0d3d550

rubiesonthesky force-pushed the fix-for-removing-map-type branch from 6a6c5ae to 0d3d550 Compare April 3, 2024 19:03

rubiesonthesky added 2 commits April 5, 2024 00:29

Merge branch 'main' into fix-for-removing-map-type

47a9292

Merge branch 'main' into fix-for-removing-map-type

1254c46

JoshuaKGoldberg requested changes Apr 9, 2024

View reviewed changes

JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Apr 9, 2024

rubiesonthesky and others added 7 commits April 9, 2024 18:32

Update src/shared/comparisons.ts

7623d57

Co-authored-by: Josh Goldberg ✨ <[email protected]>

more tests

ea4af19

copy test utils

8d3b985

more tests and changing comparation logic little bit

b7e0f4f

working comparization for Set

1a708e1

revert unintended changes

e692849

fix spelling and imports

ffe63dc

move code back

14f7e26

rubiesonthesky commented Apr 11, 2024

View reviewed changes

JoshuaKGoldberg requested changes Apr 13, 2024

View reviewed changes

src/shared/comparisons.ts Show resolved Hide resolved

rubiesonthesky commented Apr 13, 2024

View reviewed changes

src/shared/comparisons.ts Outdated Show resolved Hide resolved

check if Set or Map is built-in type

7b6b48a

JoshuaKGoldberg approved these changes Apr 23, 2024

View reviewed changes

JoshuaKGoldberg removed the status: waiting for author Needs an action taken by the original poster label Apr 23, 2024

JoshuaKGoldberg merged commit d27679b into JoshuaKGoldberg:main Apr 23, 2024
11 checks passed

rubiesonthesky deleted the fix-for-removing-map-type branch May 4, 2024 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rubiesonthesky commented Apr 3, 2024

codecov bot commented Apr 3, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Apr 9, 2024

rubiesonthesky Apr 9, 2024

rubiesonthesky commented Apr 10, 2024

rubiesonthesky Apr 11, 2024

JoshuaKGoldberg Apr 13, 2024

rubiesonthesky Apr 11, 2024

rubiesonthesky Apr 11, 2024

JoshuaKGoldberg Apr 13, 2024

rubiesonthesky Apr 11, 2024

rubiesonthesky commented Apr 13, 2024

JoshuaKGoldberg left a comment

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

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

Conversation

rubiesonthesky commented Apr 3, 2024

PR Checklist

Overview

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubiesonthesky commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubiesonthesky commented Apr 13, 2024

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2024 •

edited

Loading