Update dependencies and lint for no-unsafe-any by andy-hanson · Pull Request #2544 · palantir/tslint

andy-hanson · 2017-04-11T01:10:27Z

PR checklist

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

Overview of change:

The no-unsafe-any rule mostly just required appropriate casts, but in the case of FormatterFunction it seems to have discovered a bug -- formatters actually seem to have to be new-able. So created a FormatterStatic instead. This is technically a breaking change, but code that compiled against FormatterFunction would have been wrong.

ajafff · 2017-04-11T09:29:49Z

src/language/formatter/formatter.ts


 export type ConsumerType = "human" | "machine";

+export interface FormatterStatic {


rename to FormatterConstructor?

ajafff · 2017-04-11T09:32:05Z

src/ruleLoader.ts


 const moduleDirectory = path.dirname(module.filename);
 const CORE_RULES_DIRECTORY = path.resolve(moduleDirectory, ".", "rules");
-const cachedRules = new Map<string, typeof AbstractRule | null>(); // null indicates that the rule was not found


actually I found this to be clearer

But rules aren't actually guaranteed to be implemented by AbstractRule subclasses, are they?

Now that it's renamed to RuleConstructor I'm fine with it.

ajafff · 2017-04-11T09:33:14Z

src/language/rule/rule.ts

 import {arrayify, flatMap} from "../../utils";
 import {IWalker} from "../walker";

+export interface RuleStatic {


also rename to RuleConstructor?

ajafff · 2017-04-11T09:35:00Z

src/ruleLoader.ts


-    let Rule: typeof AbstractRule | null = null;
+    let Rule: RuleStatic | null = null;
    if (absolutePath != null) {


this comparison can be strict

ajafff · 2017-04-11T09:37:11Z

src/rules/noInferredEmptyObjectTypeRule.ts

@@ -87,7 +87,7 @@ class NoInferredEmptyObjectTypeRule extends Lint.ProgramAwareRuleWalker {
        let isAnonymous: boolean;
        if (ts.ObjectFlags == null) {
            // typescript 2.0.x specific code


this branch can be removed since support for ts 2.0.x was dropped in v5.0.0

ajafff · 2017-04-11T09:41:10Z

src/utils.ts

 */
 export function camelize(stringWithHyphens: string): string {
-    return stringWithHyphens.replace(/-(.)/g, (_, nextLetter) => nextLetter.toUpperCase());
+    return stringWithHyphens.replace(/-(.)/g, (_, nextLetter) => (nextLetter as string).toUpperCase());


consider adding the type annotation to the parameter instead of asserting it

I don't like that style since it behaves the same as a typecast but doesn't look like one. Was actually considering writing a lint rule for that.

um, I don't think that's quite right @andy-hanson. type assertions (as string) are generally less safe than typedefs, because the assignability checks for the latter are stricter

A type annotation in a callback is unsafe.

declare function f(cb: (x: {}) => void): void; f((n: number) => { console.log(n + 1); }); // Huh, no error?

A type assertion is of course also unsafe, but it's more obvious that it is.

adidahiya · 2017-04-12T05:19:35Z

src/ruleLoader.ts

        }
    }
-    return undefined;
+    return null;


why the switch to nulls? can't we use undefined?

We use cachedRules.set(fullPath, Rule);, so if Rule is undefined, it will look like we didn't cache a result at all. I actually already had to do this in #2369.

maybe we should use a string literal like "not-found" or "load-error" instead of null

adidahiya · 2017-04-12T05:22:28Z

src/utils.ts

 */
 export function camelize(stringWithHyphens: string): string {
-    return stringWithHyphens.replace(/-(.)/g, (_, nextLetter) => nextLetter.toUpperCase());
+    return stringWithHyphens.replace(/-(.)/g, (_, nextLetter) => (nextLetter as string).toUpperCase());


um, I don't think that's quite right @andy-hanson. type assertions (as string) are generally less safe than typedefs, because the assignability checks for the latter are stricter

adidahiya · 2017-04-12T05:22:59Z

src/runner.ts

        } catch (error) {
-            if (error.name === FatalError.NAME) {
-                console.error(error.message);
+            if ((error as FatalError).name === FatalError.NAME) {


can you use error: FatalError on the line above instead?

No, TS doesn't allow a type annotation there. It wouldn't be safe anyway since TS doesn't have checked exceptions.

adidahiya · 2017-04-12T05:23:26Z

src/rules/noUnsafeAnyRule.ts

    /* tslint:enable:object-literal-sort-keys */

-    public static FAILURE_STRING = "Unsafe use of expression of type 'any'.";
+    public static FAILURE_STRING: string = "Unsafe use of expression of type 'any'.";


surely this isn't necessary?

Whoops, no idea why that got there!

adidahiya · 2017-04-12T05:23:52Z

src/rules/noConsecutiveBlankLinesRule.ts


    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
-        const limit: number = this.ruleArguments[0] || Rule.DEFAULT_ALLOWED_BLANKS;
+        const limit = this.ruleArguments[0] as number | undefined || Rule.DEFAULT_ALLOWED_BLANKS;


why the switch from typedef to type assertion?

A typedef const x: number = thisIsAny; is flagged by the no-unsafe-any rule, since it looks safe but isn't. const x = thisIsAny as number; is OK since that's obviously an assertion.

nchen63 · 2017-04-16T04:51:49Z

src/configuration.ts

-function getHomeDir() {
+function getHomeDir(): string | undefined {
    const environment = global.process.env;
    const paths = [


prefer type annotation on paths instead of the function return value. or both

nchen63 · 2017-04-16T05:11:47Z

src/ruleLoader.ts

                    const ruleSpecificList = enableDisableRules || [];
                    ruleOptions.disabledIntervals = buildDisabledIntervalsFromSwitches(ruleSpecificList);
-                    rules.push(new (Rule as any)(ruleOptions));
+                    rules.push(new (Rule as any)(ruleOptions) as IRule);


don't need either cast

nchen63 · 2017-04-16T05:14:33Z

src/configuration.ts

 * limitations under the License.
 */

+// tslint:disable no-unsafe-any (TODO)


why is this needed?

nchen63 · 2017-04-16T05:15:55Z

src/formatterLoader.ts

    if (fs.existsSync(`${fullPath}.js`)) {
        const formatterModule = require(fullPath);
-        return formatterModule.Formatter;
+        return formatterModule.Formatter; // tslint:disable-line no-unsafe-any


instead of disabling rule, cast the require to an interface like { Formatter: FormatterConstructor }

nchen63 · 2017-04-16T05:16:03Z

src/formatterLoader.ts

        return undefined;
    }
-    return require(src).Formatter;
+    return require(src).Formatter; // tslint:disable-line no-unsafe-any


same as above comment

nchen63 · 2017-04-16T05:20:30Z

src/ruleLoader.ts

        }
    }
-    return undefined;
+    return null;


maybe we should use a string literal like "not-found" or "load-error" instead of null

nchen63 · 2017-04-16T05:23:10Z

src/ruleLoader.ts

-        Rule = loadRule(absolutePath, ruleName);
+    let Rule: RuleConstructor | null = null;
+    if (absolutePath !== undefined) {
+        Rule = loadRule(absolutePath, ruleName); // tslint:disable-line no-unsafe-any


I don't think you need to disable the rule here

nchen63 · 2017-04-16T05:35:45Z

src/rules/memberOrderingRule.ts

    }

+    // tslint:disable-next-line no-unsafe-any
    return { order: categoryFromOption(firstOption[OPTION_ORDER]), alphabetize: !!firstOption[OPTION_ALPHABETIZE] };


instead of disabling, annotate firstOption as type Options, then call using firstOption.order and firstOption.alphabetize

nchen63 · 2017-04-16T05:44:13Z

src/tslint-cli.ts

 * limitations under the License.
 */

+// tslint:disable no-unsafe-any (TODO)


I don't think you need to disable the rule here

nchen63

just some nits

nchen63 · 2017-04-17T06:52:27Z

src/configuration.ts

 }

-/**
+/**findRule


why is this here?

nchen63 · 2017-04-17T07:10:45Z

src/ruleLoader.ts

 const moduleDirectory = path.dirname(module.filename);
 const CORE_RULES_DIRECTORY = path.resolve(moduleDirectory, ".", "rules");
-const cachedRules = new Map<string, typeof AbstractRule | null>(); // null indicates that the rule was not found
+const cachedRules = new Map<string, RuleConstructor | "not-found">(); // null indicates that the rule was not found


can remove/update this comment

Update dependencies and lint for no-unsafe-any

d9faf67

andy-hanson force-pushed the use_no-unsafe-any branch from 1e9977f to d9faf67 Compare April 11, 2017 01:18

ajafff reviewed Apr 11, 2017

View reviewed changes

Respond to PR comments

c422969

adidahiya reviewed Apr 12, 2017

View reviewed changes

andy-hanson mentioned this pull request Apr 13, 2017

Add use-default-type-parameter rule #2253

Merged

4 tasks

Remove unnecessary type annotation

cb8b9da

andy-hanson mentioned this pull request Apr 13, 2017

Use tslint:all to lint tslint #2563

Merged

4 tasks

andy-hanson added 2 commits April 12, 2017 18:38

Merge branch 'master' into use_no-unsafe-any

8b28da7

Merge branch 'master' into use_no-unsafe-any

cb1614d

adidahiya requested a review from nchen63 April 14, 2017 17:03

andy-hanson mentioned this pull request Apr 16, 2017

Add 'object-literal-contextual-type' rule #2581

Closed

4 tasks

nchen63 suggested changes Apr 16, 2017

View reviewed changes

Respond to PR comments

dd04557

nchen63 suggested changes Apr 17, 2017

View reviewed changes

nchen63 added 2 commits April 17, 2017 17:49

remove findRule, appears to be misplaced text

416951a

remove out of date comment

0dca004

nchen63 approved these changes Apr 17, 2017

View reviewed changes

nchen63 merged commit a3d8d6f into palantir:master Apr 17, 2017

andy-hanson deleted the use_no-unsafe-any branch April 18, 2017 00:40


		export type ConsumerType = "human" \| "machine";

		export interface FormatterStatic {

Conversation

andy-hanson commented Apr 11, 2017

PR checklist

Overview of change:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajafff Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hanson Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hanson Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchen63 Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchen63 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajafff Apr 11, 2017 •

edited

Loading

andy-hanson Apr 13, 2017 •

edited

Loading

andy-hanson Apr 13, 2017 •

edited

Loading

nchen63 Apr 16, 2017 •

edited

Loading