Fix more false positives in no-unnecessary-type-assertion #3465

calebegg · 2017-11-07T18:23:37Z

This technique was suggested by @alexeagle -- thanks!

The approach here is to basically determine what would actually happen from the perspective of the TS typechecker if the suggested replacement was applied. That way, we can catch false positives where the assertion actually affects inference.

There's a new test case that demonstrates an instance of this kind of false positive.

I had hoped that adding a check like this would allow me to also eliminate the false positive check for tuple/literal types, but it doesn't. Even with this new approach, the type we get from the type checker still gets widened in certain cases, so we still have to watch for that. Anyway, the old check is much faster, so it's good to keep around regardless.

alexeagle

Cool! /cc @andy-ms who I think removed this trick from no-unused-variable where I had used it before. I think this time it's appropriate since you only do this after the faster mostly-correct check.

ajafff

Based on the experience in #2763 I'm -1 on this change

ajafff · 2017-11-07T18:38:55Z

src/rules/noUnnecessaryTypeAssertionRule.ts

+                    return modifiedSourceFile;
+                }
+                // tslint:disable-next-line:no-unsafe-any Passing args along to original function.
+                return oldGetSourceFile.apply(this, [fileName, ...args]);


This will be VERY slow even in small projects. See #2763 (comment). The difference here is, that this implementation reads all but the currently linted sourcefile from disk instead of using the content of the original sourcefile. That makes it even worse.
Also note that #2763 creates a new Program for every linted SourceFile. This rule does so for every potential failure.

I tried it internally and it took about 0.4 seconds on a medium-large project. I think these cases should be very rare, so this "slow path" will almost always only occur with true positives, which should then (hopefully) get removed.

I think these cases should be very rare

Except for people who just enabled the rule. They will probably think the process ran into an endless loop.

In addition there are projects that disable specific rules for certain (often legacy) files via tslint:disable comments. That doesn't turn off the rule. It just mutes its output.

ajafff · 2017-11-07T18:40:22Z

src/rules/noUnnecessaryTypeAssertionRule.ts

-                : Lint.Replacement.deleteFromTo(node.expression.getEnd(), node.getEnd()));
+                : Lint.Replacement.deleteFromTo(node.expression.getEnd(), node.getEnd());
+            const sourceFile = this.sourceFile;
+            const modifiedHost = ts.createCompilerHost(this.program.getCompilerOptions());


this won't work if the Program was created using a custom CompilerHost.
I know we also do this in Linter.updateProgram, but that's only executed if you run with --fix.

ajafff · 2017-11-07T18:42:51Z

test/rules/no-unnecessary-type-assertion/test.ts.fix

+function bar(s: string) {}
+
+// Type error without this assertion
+const xyzzy = foo() as string;


If this is the only false positive, I don't think it's worth sacrificing the normal case

It's come up several times for us. It's not a common pattern, but it does happen with e.g. Angular's .d.ts

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/angular/index.d.ts#L2058

Instead of asserting the type you can just explicitly declare the type argument on the function call:

const v = foo<string>();

That's true, and that's what I suggest when people file bugs. But people keep filing bugs, and I am a limited resource.

You could just change the error message to mention that there are cases like this, and recommend using a contextual type as in const x: string = foo() or an explicit type argument as in const x = foo<string>(); in these cases. The type assertion is still in some sense unnecessary as the code can be written without any type assertions, which is good since those are unsafe. foo() as string should really be reserved for functions like declare function foo(): string | number;.

ajafff · 2017-11-07T18:45:21Z

src/rules/noUnnecessaryTypeAssertionRule.ts

+    if (haystack.pos === updatedPos && haystack.end === updatedEnd) {
+        return haystack;
+    }
+    for (const child of haystack.getChildren()) {


I'd prefer ts.forEachChild since .getChildren() additionally returns tokens not present in the AST. These tokens are not relevant for the rule.

Done. That's a very strange/subtle undocumented difference....

ajafff · 2017-11-07T18:45:51Z

src/rules/noUnnecessaryTypeAssertionRule.ts

        }
    }
+    throw new Error("Can't find corresponding node");


If an error is thrown, there's not need for the | null in the return type

alexeagle · 2017-11-07T19:15:04Z

A bit more context, in the TS design review meeting microsoft/TypeScript#19393 @mhegazy observed that checks which are computationally expensive (eg requiring control flow analysis) have been traditionally been first-party in TypeScript. That wasn't really intended, it seems to me that TypeScript's diagnostics should be concerned with the Language Spec. One suggestion in that meeting was to extend the TypeChecker APIs to make it easier for third-parties like TSLint to write such rules. I think this deserves some other discussion - I just point that out here in case there is some missing API that would make it possible to fix this false positive without creating a new program.

ajafff · 2017-11-07T19:24:04Z

in case there is some missing API that would make it possible to fix this false positive without creating a new program.

That goes too deep into TypeScript internals. I don't think they will expose an API to find out if a node is involved in inferring the type of another node.

It'd be great if it was possible to reuse the whole program except for one specific SourceFile. Unfortunately that doesn't work as it messes up everything else: #2736 and #2649

alexeagle · 2017-11-17T16:26:06Z

I fear that we will write more type-checked rules where we are forced to accept false positives, or worse, we won't be able to write the rule at all given our limited ability to query or interact with the type-checker. At Google, we don't want to train our users to suppress compiler diagnostics due to their incorrectness, and we want to have some rules which cannot be suppressed (eg. related to security sensitive APIs)

Discussed with @mhegazy and @DanielRosenwasser yesterday. There are two options on the table, but neither is a short-term fix for this PR:

Expose additional query operations on the typechecker to support rules like this one. TS team is okay with doing this. I think the big problem is that we can't know a priori what queries we might need for other type-checked rules in the future, so we'll frequently be in the position of waiting for an upstream TS release to unblock a rule implementation.
Make @calebegg's approach here perform. Mohamed points out that it still doesn't scale in general (can't implement every check by creating a new ts.Program) but our objective is only to use it after we've exited the hot code path with other simpler conditionals. Rough proposal needs writing up, but something like:

use new ts.CustomTransformers API to create an AST transform. Make sure any new nodes created in this way are bound and have parent pointers.
Change TypeScript so that checker.ts doesn't rely on the tree being backed by ts.SourceFile text. This ought to be the design anyway. Would require some refactoring.
[mumble mumble] avoid destructive operations on the old ts.Program by doing the right thing. I think this is simply avoiding the hacks we did with the fake host in the unused variable check you mention above.
now we can query the type-checker about anything in the transformed program

giladgray · 2018-07-28T00:05:38Z

@calebegg please update this branch so we can review it, or close it if not relevant.

this change seems rather dangerous and lacks a safe answer. what would happen if we didn't do this?

calebegg · 2018-07-28T20:31:21Z

I don't think updating this old PR would be a worthwhile use of my time. The decision of whether or not to accept it can be made without doing that.

johnwiseheart · 2018-08-22T18:34:47Z

Closing

calebegg added 4 commits September 18, 2017 19:31

Merge remote-tracking branch 'palantir/master'

cffa68e

Merge remote-tracking branch 'palantir/master'

02b7f81

Merge remote-tracking branch 'palantir/master'

fcf0359

Reduce false positives from no-unnecessary-type-assertion

3aab8d6

alexeagle reviewed Nov 7, 2017

View reviewed changes

ajafff suggested changes Nov 7, 2017

View reviewed changes

Respond to review comments

ffcf335

ajafff mentioned this pull request Dec 2, 2017

no-unnecessary-type-assertion false positive #3540

Closed

johnwiseheart closed this Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix more false positives in no-unnecessary-type-assertion #3465

Fix more false positives in no-unnecessary-type-assertion #3465

calebegg commented Nov 7, 2017

alexeagle left a comment

ajafff left a comment

ajafff Nov 7, 2017

calebegg Nov 7, 2017

ajafff Nov 7, 2017

ajafff Nov 7, 2017

ajafff Nov 7, 2017

calebegg Nov 7, 2017

ajafff Nov 7, 2017

calebegg Nov 7, 2017

ghost Nov 8, 2017

ajafff Nov 7, 2017

calebegg Nov 7, 2017

ajafff Nov 7, 2017

calebegg Nov 7, 2017

alexeagle commented Nov 7, 2017

ajafff commented Nov 7, 2017

alexeagle commented Nov 17, 2017

giladgray commented Jul 28, 2018

calebegg commented Jul 28, 2018

johnwiseheart commented Aug 22, 2018

Fix more false positives in no-unnecessary-type-assertion #3465

Fix more false positives in no-unnecessary-type-assertion #3465

Conversation

calebegg commented Nov 7, 2017

alexeagle left a comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle commented Nov 7, 2017

ajafff commented Nov 7, 2017

alexeagle commented Nov 17, 2017

giladgray commented Jul 28, 2018

calebegg commented Jul 28, 2018

johnwiseheart commented Aug 22, 2018