feat(eslint-plugin): [no-unnecessary-type-conversion] add rule #10182
Conversation
|
Thanks for the PR, @skondrashov! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit 0d16798.
☁️ Nx Cloud last updated this comment at |
|
Sorry for force-push, wasn't sure how else to resolve the merge conflicts other than redoing the commit cause they didn't show up locally. Doesn't seem like it did anything to the merge conflict though, so I'm out of ideas. I see I have problems from the perfectionist plugin in my files as well, but the plugin doesn't run locally when I use Would be happy to address the 2 missing lines of coverage at some point if everything else gets figured out. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10182 +/- ##
==========================================
+ Coverage 90.82% 90.89% +0.06%
==========================================
Files 497 498 +1
Lines 50204 50552 +348
Branches 8274 8306 +32
==========================================
+ Hits 45600 45948 +348
Misses 4589 4589
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
(hint) - We often use // setup
declare let str: string | number;
declare const num: number | string;
// etcIt's a useful pattern for testing sometimes too, since you may run into surprising behavior when
Makes sense 👍
Yep, also makes sense. And, even if TS did allow them, we don't need to stress about identifying every possible way for a value to be coerced, just the common patterns, and others can always be added in the future, too. Appreciate your close attention to detail on all these, though!
This is an astute observation. Is there anything this rule would flag that that rule doesn't flag? If not, I'd lean towards letting that separate rule handle this case. If you want you could even put a note in the docs that tells the user that template expressions intentionally aren't handled because they're already dealt with more thoroughly in the
Hmm, I hear you. I personally think either one is fine. Maybe someone else will care more strongly 🤷
I think for these reports, reporting the whole node is totally fine declare const s: string;
s + '';
~~~~~~
!!(longExpressionThatsABoolean);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~But, I really like what you've done with the more granular approach. !!(longExpressionThatsABoolean);
~~So I'd say go for it! But if it becomes an implementation hassle, no stress either if you end up falling back to the typical strategy of letting the |
No worries! The real pain is just force pushing between reviews, not before a reviewer has looked at it at all 🙂.
Oh, so I think the trouble that you're having is that you've used the
But you're in a slightly awkward scenario since your branch is on main. What I personally do (which differs from the above) is to set a second remote on your local repo, i.e. LMK if this helps or if you'd like some more help. (I can also push things directly to your branch to bail you out if needed 🤣). And no stress if you end up doing a force push again this time to get yourself in a clean state here! |
packages/eslint-plugin/tests/rules/no-unnecessary-coercion.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/no-unnecessary-coercion.test.ts
Outdated
Show resolved
Hide resolved
Agreed; let's go with |
…th more than just string types
|
👋 Just checking in @skondrashov - is this still something you have time and energy for? |
|
I'll have time to finish up the changes probably early in the new year! |
|
Awesome! I'll move this to draft then in the meantime, so we don't bug you. 🚀 |
|
After latest round of fixes, I believe there are 3 things I still need to do:
I left some comments in the appropriate threads. @kirkwaiblinger do you mind resolving the threads that have been resolved to your satisfaction? I don't want to do it myself in case I'm wrong in resolving something. |
Let's not do this, actually. It's complex to detect, a rare situation, the only situation in which it would affect correctness the rule's report is if someone overrides
Sounds good 👍 On the motivation, for me:
responded asking for more detail 👍
Will do! 👍 |
packages/eslint-plugin/src/rules/no-unnecessary-type-conversion.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-unnecessary-type-conversion.ts
Outdated
Show resolved
Hide resolved
| innerNode: [node.argument], | ||
| sourceCode: context.sourceCode, | ||
| }; | ||
| const typeString = |
There was a problem hiding this comment.
This assumes knowledge of its caller, and doesn't really seem to be necessary..... I was playing around and refactored a couple things, WDYT?
- removed
typeStringdeduction - provide report
dataexplicitly (probably best to do since the report data have very loose types) - remove duplication of the
isDoubleOperatordeduction withouterNodevariable (which tbh might be better off coming from the caller as well).
function handleUnaryOperator(
node: TSESTree.UnaryExpression,
typeFlag: ts.TypeFlags,
isDoubleOperator: boolean, // !! or ~~
typeString: string,
violation: string,
) {
const outerNode = isDoubleOperator ? node.parent : node;
const type = services.getTypeAtLocation(node.argument);
if (doesUnderlyingTypeMatchFlag(type, typeFlag)) {
context.report({
loc: {
start: outerNode.loc.start,
end: {
column: node.loc.start.column + 1,
line: node.loc.start.line,
},
},
messageId: 'unnecessaryTypeConversion',
data: {
type: typeString,
violation,
},
fix: getWrappingFixer({
node: outerNode,
innerNode: [node.argument],
sourceCode: context.sourceCode,
}),
suggest: [
{
messageId: 'unnecessaryTypeConversionSuggestion',
data: { type: typeString },
fix: getWrappingFixer({
node: outerNode,
innerNode: [node.argument],
sourceCode: context.sourceCode,
wrap: expr => `${expr} satisfies ${typeString}`,
}),
},
],
});
}
}There was a problem hiding this comment.
lovely, did this almost exactly but with typeString: string, changed to typeString: 'boolean' | 'number', and the order of parameters changed so that isDoubleOperator is last cause it feels like it should be. I feel like you probably don't care about the order of the parameters, but let me know if you think changing the type is a bad move.
packages/eslint-plugin/src/rules/no-unnecessary-type-conversion.ts
Outdated
Show resolved
Hide resolved
| getConstrainedTypeAtLocation(services, node.arguments[0]); | ||
|
|
||
| const isBuiltInCall = (name: string) => { | ||
| if ((node.callee as TSESTree.Identifier).name === name) { |
There was a problem hiding this comment.
You can avoid this type assertion by
- Define a type predicate for the node type
const isIdentifier = isNodeOfType(AST_NODE_TYPES.Identifier) - Alias the
node.calleeto use it in the closure,
CallExpression(node: TSESTree.CallExpression): void {
if (
isIdentifier(node.callee) &&
node.arguments.length === 1
) {
const nodeCallee = node.callee;
const getType = () =>
getConstrainedTypeAtLocation(services, node.arguments[0]);
function isBuiltInCall(name: string) {
if (nodeCallee.name === name) {
const scope = context.sourceCode.getScope(node);
const variable = scope.set.get(name);
return !variable?.defs.length;
}
return false;
}There was a problem hiding this comment.
Fun fact, I think it's a bug in TS that you need to alias nodeCallee to preserve the narrowing in the closure, see microsoft/TypeScript#61158
There was a problem hiding this comment.
Is there an advantage of using isNodeOfType over nodeCallee.type === AST_NODE_TYPES.Identifier? Aliasing nodeCallee seemed to solve the problem on its own.
packages/eslint-plugin/src/rules/no-unnecessary-type-conversion.ts
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| }, | ||
| 'CallExpression > MemberExpression.callee > Identifier[name = "toString"].property'( |
There was a problem hiding this comment.
Today I learned that you could use ESQuery with the .callee like this. Cool!
Question, though, does the .property at the end do anything? I can't think what it would do and no tests fail if I change it to
| 'CallExpression > MemberExpression.callee > Identifier[name = "toString"].property'( | |
| 'CallExpression > MemberExpression.callee > Identifier[name = "toString"]'( |
There was a problem hiding this comment.
To make sure the selector is correct could you also add the following (valid) test case?
export {};
declare const toString: string;
toString.toUpperCase();There was a problem hiding this comment.
That test fails without the .property, which I guess is what you had in mind? Adding the test, leaving the selector for that reason.
| const services = getParserServices(context); | ||
| const checker = services.program.getTypeChecker(); | ||
|
|
||
| const surroundWithParentheses = ( |
There was a problem hiding this comment.
Would you mind providing a few examples? Would save me quite a bit of time 🙏 In general, we're not too stressed about some extra parens, which most users are likely to fix with a formatter anyway, unlike too few parens, which will actually break people's code.
| }); | ||
|
|
||
| ruleTester.run('no-unnecessary-type-conversion', rule, { | ||
| valid: [ |
There was a problem hiding this comment.
(testing)
Could you please add some valid cases around ensuring that only primitives cause the rule to flag? In particular, I'm thinking about the boxed primitives, so, things like
String(new String('foo'));
function f(x: Number) {
return +x;
}etc
Also it would be good to add some tests around stringy and numbery enums.
Also let's add some coverage around generics, i.e.
function f<T extends string>(x: T) {
return String(x);
}There was a problem hiding this comment.
Added cases for boxed primitives and generics, but enums are elusive since I can't copy the tests for no-unnecessary-template-expression - there are no tests for enums there, and it has what I think is a bug as a result:
enum Foo {
A = "A",
}
function f(foo: Foo) {
return `${foo}`;
}
const c = `${Foo.A}`;This finds autofixes for both the string templates, but applying them changes the type of c or the return type of f from Foo to string if applied. I'm not sure if this is a bug but this PR does the same thing, albeit with String(foo) instead of `${foo}`.
This comment was marked as resolved.
This comment was marked as resolved.
|
@jdufresne fun suggestion! Yeah, let's scope this as an enhancement rather than part of the initial implementation 👍 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@kirkwaiblinger I think I'm ready for another look. I left some replies, but I thought I'd reply to the I said before in one of my comments that there are cases with double parentheses, but I couldn't find any now. Not sure if I was wrong before, but everything else I tried this time made sense. Nothing with too few parentheses and just one with too many. I found some sets of patterns that might be enlightening to play with anyway - These patterns (and similar for other types) strip all parentheses, no matter how many you put: String(((('a')))); // -> 'a';
((('a'))).toString(); // -> 'a';
((('a'))) + ''; // -> 'a';If the thing being operated on isn't unary, the output gets a single set of parentheses, again regardless of how many there are. The last case here is the one that feels like it's adding extra parentheses: String('a' + 'b'); // -> ('a' + 'b');
('a' + 'b').toString(); // -> ('a' + 'b');
'a' + 'b' + ''; // -> ('a' + 'b');Using the 'a'.toString(); // -> 'a' satisfies string;
'a'.toString().length; // -> ('a' satisfies string).length;
(('a').toString()).length; // -> ('a' satisfies string).length;
('a' + 'b').toString().length; // -> (('a' + 'b') satisfies string).length;
(('a' + 'b').toString()).length; // -> (('a' + 'b') satisfies string).length;Examples like this show that everything somehow works fine with weird double operators, parentheses, and whitespace: ( ~ ( ~ ( 5 ) ) ); // -> ( 5 );I think my main concern is whether what I did to the code in Still to do:
|
|
Excited about this rule! I just saw a lot of |
PR Checklist
Overview
Shows autofixable errors for:
with the following error messaging:
Does not (and should not) show errors for:
Does not do anything with Symbol() because everything weird I tried to do with symbols was already a typescript error.
Does not do anything for some conversions to number which are already forbidden by typescript ("The left-hand side of an arithmetic operation must be of type 'any', 'number', 'bigint' or an enum type."), including:
Does not cover
${"123"}, because that overlaps with theno-unnecessary-template-expressionrule. Need feedback on what to do about that.I went with the word "coercion" because it was chosen by @Josh-Cena in the original issue but I think it's a poor choice. I think "conversion" or even "type conversion idiom" might be the most precise, because
.toString()for example doesn't rely on coercion, and while!!valueand!valueboth coerce the type ofvalueto a boolean, it's the property of the idiom!!that it can be used for type conversion which puts it under the umbrella of this rule. So I thinkno-unnecessary-type-conversionis a better name for this rule, given the things that I'm currently having it check for.Catches the following problems in this repository (separate commit):
Might not have the best placement for the error scope, and might not have the best formatting for autofixes, could use feedback on those too.