Add no-unnecessary-type-assertion rule #2519
Conversation
|
Thanks for your interest in palantir/tslint, @mitchellwills! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
There was a problem hiding this comment.
Thank you for the effort you put into this.
Long term we try to migrate all rules from RuleWalker to another walker design. Please refer to https://palantir.github.io/tslint/develop/custom-rules/walker-design.html for an explanation of the new concepts. Would you mind migrating your implementation?
In addition this rule needs tests for union types that are narrowed by type guards and/or a discriminant property
|
Sure i'll take a stab at the changes you suggested. Any idea on how to enable strict null types for the tests (I guess I could just switch it to a union type, but it would probably be good to test both)? |
* Fix tests * Add tests for union types with type guards and discriminant properties * Migrate implementation to AbstractWalker * Revert changes to src/language/walker/syntaxWalker.ts
|
Migrated to AbstractWalker and added/fixed tests |
|
microsoft/TypeScript#13502 would make this rule much better. Presumably it won't warn for |
|
Hmm yah that would be pretty neat. I wonder if some people would still want to use a cast since there are some cases where removing those casts would cause errors. Although they should probably use |
|
re CLA: I am covered under the Google corporate CLA. I just made my membership of the google organization public though and it doesn't look like there's a way for me to retrigger the CLA bot. |
There was a problem hiding this comment.
looks like a good rule. some changes requested. most important is the fact that these are type assertions, not casts (the latter implies some runtime behavior).
src/rules/noUnnecessaryCastRule.ts
Outdated
|
|
||
| export class Rule extends Lint.Rules.TypedRule { | ||
| /* tslint:disable:object-literal-sort-keys */ | ||
| public static metadata: Lint.IRuleMetadata = { |
There was a problem hiding this comment.
minor: please use 4-space indentation (as per the root .editorconfig)
There was a problem hiding this comment.
Reformatted all the files
src/rules/noUnnecessaryCastRule.ts
Outdated
| type: "typescript", | ||
| typescriptOnly: true, | ||
| requiresTypeInfo: true, | ||
| }; |
There was a problem hiding this comment.
looks like you implemented a fixer. can you add hasFix: true to the metadata?
src/rules/noUnnecessaryCastRule.ts
Outdated
| }; | ||
| /* tslint:enable:object-literal-sort-keys */ | ||
|
|
||
| public static FAILURE_STRING = "This cast is unnecessary."; |
There was a problem hiding this comment.
Please be more descriptive in the failure string and explain that the cast doesn't change the type of the expression. Also, to be slightly pedantic here (well within the rights of a linter), this is more correctly referred to as a type assertion in TS syntax, so let's refer to it as such (we have other rules that relate to type assertions). The name should be changed to no-unnecessary-type-assertion.
FAILURE_STRING = "This assertion is unnecessary since it does not change the type of the expression."
There was a problem hiding this comment.
Added suggested failure string and renamed the rule everywhere. The AST is a little misleading here since it refers to "expr" as a type assertion and the others as something else.
src/rules/noUnnecessaryCastRule.ts
Outdated
| const castType = this.typeChecker.getTypeAtLocation(node); | ||
| const uncastType = this.typeChecker.getTypeAtLocation(node.expression); | ||
|
|
||
| if (uncastType && castType && uncastType === castType) { |
There was a problem hiding this comment.
please avoid implicit type coercion to boolean (I'd like to turn on strict-boolean-expressions). do explicit null checks uncastType != null && castType != null && ...
There was a problem hiding this comment.
Fixed implicit boolean coercion
* Replace references to cast with the more correct type assertion * Fix formatting * Improve metadata
mitchellwills
changed the title
|
thanks @mitchellwills! |
PR checklist
Overview of change:
Adds rule that warns when a cast has no effect (the uncast expression has the same type as the cast expression).
Is there anything you'd like reviewers to focus on?
The tests are broken because the lint check typecheck is not running with strict null checks ("string|undefined" type gets reported as "string" by typechecker). Adding the strictNullChecks fixed this for me in an internal repo, but doesn't seem to work here for some reason (may be ts2.2 vs ts2.3?). Any ideas what needs to change to make it work?
I compare the two types using ===. From my testing this seems to work, but there doesn't seem to be any documentation that indicates this behavior can be relied on. Is there a preferred way to check the equality of types?