Fix many no-object-literal-type-assertion lint errors #17278

ghost · 2017-07-18T20:40:07Z

Tested enabling the no-object-literal-type-assertion lint rule.
This doesn't fix every case; the remaining cases will be harder to fix, and a few may be genuine bugs.
There were only a few categories:

Totally unnecessary casts -> deleted.
Needed a contextual type -> provided one somehow, often by just wrapping in id<T>({ ... }).
Truly needed to pass off a badly-typed object -> added a disable.
If not one of the above, not handled here.

DanielRosenwasser · 2017-07-18T20:44:53Z

src/compiler/binder.ts

-                clauseEnd,
-                antecedent
-            };
+            return id<FlowSwitchClause>({ flags: FlowFlags.SwitchClause, switchStatement, clauseStart, clauseEnd, antecedent });


How is this better? You're now hoping this gets inlined.

It's better from a type perspective, because type checking within a cast is weaker than assignability type checking. (See https://palantir.github.io/tslint/rules/no-object-literal-type-assertion/) If you want I could add a local variable instead of using id.

For these methods, I'd just change the return type of each create function to return the more specific kind of FlowNode it actually creates.

Agreed with Wesley; best solution is to create a closed set of control-flow nodes and change the return type.

Those functions in binder.ts don't actually always return the specific type they're named after.

We could invert FlowNode into a union of its current subtypes.

weswigham · 2017-07-18T21:29:31Z

src/server/editorServices.ts

@@ -444,10 +444,10 @@ namespace ts.server {
            if (!this.eventHandler) {
                return;
            }
-            this.eventHandler(<ProjectLanguageServiceStateEvent>{


The casts in this file aren't required at all if you change ProjectServiceEventHandler to use overloads instead of a single overload with a union for its parameter type. That's probably a nicer solution.

Changing the type of ProjectServiceEventHandler would require changing some other things; I want to avoid that in this PR. How about just a local variable?

Or we could add a union, but a local is better than the function call.

weswigham · 2017-07-18T21:30:46Z

src/server/typingsInstaller/typingsInstaller.ts

@@ -352,14 +352,14 @@ namespace ts.server.typingsInstaller {
                    this.sendResponse(this.createSetTypings(req, currentlyCachedTypings.concat(installedTypingFiles)));
                }
                finally {
-                    this.sendResponse(<EndInstallTypes>{


Same here, actually. An overloaded signature will provide a better behavior than the union here.

weswigham · 2017-07-18T21:33:51Z

src/harness/unittests/typingsInstaller.ts

@@ -935,25 +935,25 @@ namespace ts.projectSystem {
                import * as cmd from "commander
                `
            };
-            session.executeCommand(<server.protocol.OpenRequest>{


The casts in this file would be unneeded by using the overloaded signature, too.

weswigham · 2017-07-18T21:36:48Z

src/harness/unittests/session.ts

@@ -93,14 +93,14 @@ namespace ts.server {

                session.executeCommand(req);

-                expect(lastSent).to.deep.equal(<protocol.Response>{
+                expect(lastSent).to.deep.equal(id<protocol.Response>({


IMO, this is a failing of the chai type definition. I expect expect should capture an generic to expect as the argument for equal, which would allow you to cast the type under expect and receive proper completions in the equal. Failing that, this being a unit test, if we're removing id, a variable assignment is fine.

weswigham · 2017-07-18T21:38:44Z

src/harness/unittests/matchFiles.ts

-                    start: undefined,
-                };
-            });
+            expected.errors = expected.errors.map<Diagnostic>(error => ({


My personal preference is to annotate the return type on the lambda ((error): Diagnostic => ...), rather than specifying the generic argument. Specifying generic arguments always feels a tad clunky to me. No big deal either way, though.

I've always felt specifying the return type on the lambda looked clunky :). It's too bad we don't use contextual typing here, since expected.errors has a definite type.

My preference for the return type stems from being able to refactor generics into or out of the function signature without affecting the consuming code. But I do agree - appropriately contextually typing the object would be best.

weswigham · 2017-07-18T21:39:29Z

src/harness/unittests/compileOnSave.ts

@@ -518,18 +518,18 @@ namespace ts.projectSystem {
                };
                const host = createServerHost([f], { newLine });
                const session = createSession(host);
-                session.executeCommand(<server.protocol.OpenRequest>{
+                session.executeCommand(id<server.protocol.OpenRequest>({


Yep, these id calls are unnecessary if we swap to an overloaded signature.

weswigham · 2017-07-18T21:40:03Z

src/harness/projectsRunner.ts

@@ -426,12 +426,12 @@ class ProjectRunner extends RunnerBase {
                compilerResult.program ?
                    ts.filter(compilerResult.program.getSourceFiles(), sourceFile => !Harness.isDefaultLibraryFile(sourceFile.fileName)) :
                    []),
-                sourceFile => <Harness.Compiler.TestFile>{
+                sourceFile => ts.id<Harness.Compiler.TestFile>({


(sourceFile): Harness.Compiler.TestFile?

weswigham · 2017-07-18T21:43:09Z

src/compiler/checker.ts

@@ -6254,16 +6254,16 @@ namespace ts {
                const parameterName = node.parameterName as Identifier;
                return {
                    kind: TypePredicateKind.Identifier,
-                    parameterName: parameterName ? parameterName.text : undefined,
+                    parameterName: parameterName ? <string>parameterName.text : undefined,


This cast is unsafe and probably indicative of a bug somewhere. Something to do with underscore prefixed identifiers and type guards. Use unescapeLeadingUnderscores(parameterName.text) instead.

Good catch; intended to leave any such changes out of this PR, so I'll revert it.

weswigham · 2017-07-18T21:44:56Z

src/compiler/binder.ts

-                clauseEnd,
-                antecedent
-            };
+            return id<FlowSwitchClause>({ flags: FlowFlags.SwitchClause, switchStatement, clauseStart, clauseEnd, antecedent });


For these methods, I'd just change the return type of each create function to return the more specific kind of FlowNode it actually creates.

ghost · 2017-07-18T22:03:38Z

Don't know why GitHub won't let me respond to the last comment, but those functions in binder.ts don't actually always return the specific type they're named after.

ghost · 2017-07-27T17:48:10Z

@DanielRosenwasser Those functions in binder.ts don't actually always return the specific type they're named after. So I can't just change the return type.

ghost · 2017-08-03T22:38:28Z

@DanielRosenwasser @weswigham Bump -- see above comments about binder.ts.

rbuckton · 2017-08-08T18:10:25Z

src/compiler/emitter.ts

@@ -1195,7 +1195,7 @@ namespace ts {
            if (!(getEmitFlags(node) & EmitFlags.NoIndentation)) {
                const dotRangeStart = node.expression.end;
                const dotRangeEnd = skipTrivia(currentSourceFile.text, node.expression.end) + 1;
-                const dotToken = <Node>{ kind: SyntaxKind.DotToken, pos: dotRangeStart, end: dotRangeEnd };
+                const dotToken = <Node>{ kind: SyntaxKind.DotToken, pos: dotRangeStart, end: dotRangeEnd }; // tslint:disable-line no-object-literal-type-assertion


This could be changed into:

const dotToken = createToken(SyntaxKind.DotToken); dotToken.pos = dotRangeStart; dotToken.end = dotRangeEnd;

rbuckton · 2017-08-08T18:12:31Z

src/harness/unittests/compileOnSave.ts

@@ -518,18 +518,18 @@ namespace ts.projectSystem {
                };
                const host = createServerHost([f], { newLine });
                const session = createSession(host);
-                session.executeCommand(<server.protocol.OpenRequest>{
+                session.executeCommand(id<server.protocol.OpenRequest>({


rbuckton · 2017-08-08T18:15:00Z

src/harness/unittests/matchFiles.ts

-                    start: undefined,
-                };
-            });
+            expected.errors = expected.errors.map((error): Diagnostic => ({


expected.errors = expected.errors.map<Diagnostic>(error => ({?

rbuckton · 2017-08-08T18:19:06Z

src/server/editorServices.ts

@@ -444,10 +444,10 @@ namespace ts.server {
            if (!this.eventHandler) {
                return;
            }
-            this.eventHandler(<ProjectLanguageServiceStateEvent>{


Or we could add a union, but a local is better than the function call.

rbuckton · 2017-08-08T18:19:57Z

src/server/session.ts

-        private convertToDiagnosticsWithLinePositionFromDiagnosticFile(diagnostics: Diagnostic[]) {
-            return diagnostics.map(d => <protocol.DiagnosticWithLinePosition>{
+        private convertToDiagnosticsWithLinePositionFromDiagnosticFile(diagnostics: Diagnostic[]): protocol.DiagnosticWithLinePosition[] {
+            return diagnostics.map(d => ({


diagnostics.map<protocol.DiagnosticWithLinePosition>(d => ({?

weswigham · 2017-08-08T18:57:48Z

src/compiler/transformers/generators.ts

@@ -2015,7 +2015,7 @@ namespace ts {
         *
         * @param block Information about the block.
         */
-        function beginBlock(block: CodeBlock): number {
+        function beginBlock<T extends CodeBlock>(block: T): number {


I think this would also end up better (no casts, no generics) if CodeBlock became a union?

rbuckton · 2017-08-09T20:52:34Z

src/compiler/emitter.ts

@@ -1195,7 +1195,9 @@ namespace ts {
            if (!(getEmitFlags(node) & EmitFlags.NoIndentation)) {
                const dotRangeStart = node.expression.end;
                const dotRangeEnd = skipTrivia(currentSourceFile.text, node.expression.end) + 1;
-                const dotToken = <Node>{ kind: SyntaxKind.DotToken, pos: dotRangeStart, end: dotRangeEnd }; // tslint:disable-line no-object-literal-type-assertion
+                const dotToken = createToken(SyntaxKind.DotToken);
+                dotToken.pos = dotRangeStart;


We can probably inline the locals dotRangeStart and dotRangeEnd as they are fairly trivial.

rbuckton · 2017-08-09T20:54:06Z

src/harness/unittests/matchFiles.ts

-                    start: undefined,
-                };
-            });
+            expected.errors = expected.errors.map<Diagnostic>(error => ({


I've always felt specifying the return type on the lambda looked clunky :). It's too bad we don't use contextual typing here, since expected.errors has a definite type.

rbuckton · 2017-08-09T20:56:23Z

src/compiler/transformers/generators.ts

@@ -164,12 +164,13 @@ namespace ts {
    }

    // A generated code block
-    interface CodeBlock {
+    type CodeBlock = | ExceptionBlock | LabeledBlock | SwitchBlock | LoopBlock | WithBlock;
+    interface CodeBlockBase {


Just move kind to each code block with its more specific type and remove this type. That way we can just leverage kind as a discriminant.

ghost requested a review from weswigham July 18, 2017 20:40

msftclas added the cla-not-required label Jul 18, 2017

DanielRosenwasser requested changes Jul 18, 2017

View reviewed changes

ghost force-pushed the no-object-literal-type-assertion branch from 26977a7 to 31f4930 Compare July 18, 2017 20:49

Fix many no-object-literal-type-assertion lint errors

c0e4a12

ghost force-pushed the no-object-literal-type-assertion branch from 31f4930 to c0e4a12 Compare July 18, 2017 20:56

weswigham requested changes Jul 18, 2017

View reviewed changes

ghost force-pushed the no-object-literal-type-assertion branch from 7bb6c18 to b720e0f Compare July 18, 2017 22:05

Simple fixes

08d8e04

ghost force-pushed the no-object-literal-type-assertion branch from b720e0f to 08d8e04 Compare July 18, 2017 22:05

Andy Hanson added 2 commits August 8, 2017 11:16

Use a union for FlowNode

7fcde00

Merge branch 'master' into no-object-literal-type-assertion

54a5626

rbuckton suggested changes Aug 8, 2017

View reviewed changes

weswigham approved these changes Aug 8, 2017

View reviewed changes

PR feedback and remove remaining id() uses

f6a2ea6

ghost force-pushed the no-object-literal-type-assertion branch from fb6c2fd to f6a2ea6 Compare August 8, 2017 19:34

Andy Hanson added 2 commits August 8, 2017 12:37

Use a union for CodeBlock

78d5db6

Merge branch 'master' into no-object-literal-type-assertion

f5dbb01

rbuckton suggested changes Aug 9, 2017

View reviewed changes

Discriminate CodeBlock by CodeBlockKind

4376d0f

rbuckton approved these changes Aug 9, 2017

View reviewed changes

ghost merged commit 08fbcd8 into master Aug 10, 2017

ghost deleted the no-object-literal-type-assertion branch August 10, 2017 19:52

ghost mentioned this pull request Sep 1, 2017

Finish applying no-object-literal-type-assertion lint rule #18218

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix many no-object-literal-type-assertion lint errors #17278

Fix many no-object-literal-type-assertion lint errors #17278

ghost commented Jul 18, 2017

DanielRosenwasser Jul 18, 2017

ghost Jul 18, 2017

weswigham Jul 18, 2017

DanielRosenwasser Jul 18, 2017 •

edited

Loading

ghost Jul 18, 2017

rbuckton Aug 8, 2017

weswigham Jul 18, 2017

ghost Jul 18, 2017

rbuckton Aug 8, 2017

weswigham Jul 18, 2017

weswigham Jul 18, 2017

weswigham Jul 18, 2017

weswigham Jul 18, 2017

rbuckton Aug 9, 2017

weswigham Aug 9, 2017

weswigham Jul 18, 2017

rbuckton Aug 8, 2017

weswigham Jul 18, 2017

weswigham Jul 18, 2017

ghost Jul 18, 2017

weswigham Jul 18, 2017

ghost commented Jul 18, 2017

ghost commented Jul 27, 2017

ghost commented Aug 3, 2017

rbuckton Aug 8, 2017

rbuckton Aug 8, 2017

rbuckton Aug 8, 2017

rbuckton Aug 8, 2017

rbuckton Aug 8, 2017

weswigham Aug 8, 2017

rbuckton Aug 9, 2017

rbuckton Aug 9, 2017

rbuckton Aug 9, 2017

Fix many no-object-literal-type-assertion lint errors #17278

Fix many no-object-literal-type-assertion lint errors #17278

Conversation

ghost commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser Jul 18, 2017 • edited Loading

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

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

ghost commented Jul 18, 2017

ghost commented Jul 27, 2017

ghost commented Aug 3, 2017

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

DanielRosenwasser Jul 18, 2017 •

edited

Loading