Added new rule to find as type assertions and replace with satisfies #117
Conversation
src/no-type-assertion-as.ts
Outdated
| const sourceCode = context.sourceCode; | ||
| const typeAnnotation = sourceCode.getText(node.typeAnnotation); | ||
| const expression = sourceCode.getText(node.expression); | ||
| return fixer.replaceText(node, `${expression} satisfies ${typeAnnotation}`); |
There was a problem hiding this comment.
I have no idea if it's possible (maybe @le-cong knows) but ideally we'd only do this substitution if using satisfies is ok with the compiler.
There was a problem hiding this comment.
also one other situation I can think of - x as unknown as Y should be an error but x satisfies unknown satisfies Y isn't going to work as a fix.
There was a problem hiding this comment.
i haven't get a chance to look into this, but i feel that this rule is more complex than what #116 describes. it might be necessary to use typescript's api to compare the source/target typing in details in order to tell whether or not the replacement is legit. we don't necessarily want to replace all as with satisfies i am afraid ...
There was a problem hiding this comment.
yeah i don't think the rule should introduce compiler errors. if we can't use the TS API to work it out, might be best not to auto-fix at all.
There was a problem hiding this comment.
Added to compare source/target typing to ensure type compatibility, verifying whether the originalType satisfies the constraints of the targetType and removed auto fix
carlansley
left a comment
carlansley
left a comment
There was a problem hiding this comment.
branch needs to be updated from main
src/no-type-assertion-as.ts
Outdated
| node.typeAnnotation.type === AST_NODE_TYPES.TSIntersectionType) && | ||
| node.typeAnnotation.types.some((type) => type.type === AST_NODE_TYPES.TSUnknownKeyword); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
this is a complex bit of logic I don't quite understand, if there was just one context.report() it would help. also maybe some more isSomethingSomething booleans to help readability.
carlansley
left a comment
carlansley
left a comment
There was a problem hiding this comment.
To @le-cong's point, I think we shouldn't do this rule at all if we can't generally understand when as is replaceable, and fix at least a majority of the remaining issues automatically.
Removed the auto-fix and report the error and will add parserServices and checker to access TypeScript's type-checking capabilities for source and target and determine whether to report or not |
…ermine whether to report or not
|
Beta Published - Install Command: |
I updated it to detect as type assertions and ensure the original type and target type are assignable using TypeScript's type checker. Only then report a violation if the types are assignable, as is unnecessary in such case. Since we have added type checker, i will enhance to fix as well instead of just reporting since it would be lot to manually fix. |
carlansley
left a comment
carlansley
left a comment
There was a problem hiding this comment.
the new rule seems ok to me, but can you give some data on how this works in practice with a few of our bigger, more problematic services?
|
❌ PR review status - has 1 reviewer outstanding |
Closes #116
Tested with internal services