Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Refactor and fix no-unsafe-any #3196

Merged
merged 13 commits into from Oct 20, 2017
Merged

Refactor and fix no-unsafe-any #3196

merged 13 commits into from Oct 20, 2017

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Sep 2, 2017

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

  • No longer ignore certain expression like property name in destructuring or expressions on both sides of a comparison.
  • Check destructuring assignments and destructuring initializer.
  • Disallow destructuring of any, e.g. function foo({a, b}: any){}
  • Stricter checks for class property initializer when the property is declared in the superclass with a non-any type.
  • Check yield expressions.
  • Exclude type nodes completely.
  • Ignore export = x and export default x
  • Better handling of JSX.
  • Allow any in certain locations when there is no contextual type, e.g. declare let x: any; let obj = {x}; or () => x.
  • Don't add redundant failures: if we know that x of x.y is of type any and we also reported that error, we don't add another for x.y, x.y.z or x.y(), etc.
  • Fixed all TODOs

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

[enhancement] no-unsafe-any checks more expressions, for example destructuring, yield, property initializer

@adidahiya adidahiya added this to the TSLint v5.8 milestone Sep 14, 2017
@adidahiya adidahiya self-assigned this Sep 14, 2017
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good, feel free to merge after fixing minor comments

sourceFile.statements.forEach(this.noCheck);
}

private noCheck = (node: ts.Node) => void this.visitNode(node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method name is a little unintuitive; can you make it more verbose and add a comment explaining its usage?

/** OK for this value to be 'any' if that's its contextual type. */
function checkContextual(arg: ts.Expression): void {
return cb(arg, /*anyOk*/ isAny(checker.getContextualType(arg)));
private checkContextual(node: ts.Expression, allowIfNoContextualType?: boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: would prefer the verbosity of naming this checkContextualType

@adidahiya adidahiya merged commit db73e4b into palantir:master Oct 20, 2017
HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants