Fix many no-object-literal-type-assertion lint errors · Pull Request #17278 · microsoft/TypeScript

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

-                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

            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

                    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

                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

                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

-                    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

                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

                    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

                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

-                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

                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

                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

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


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

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

            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

-        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

         * @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

                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

-                    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

    // 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.

Conversation

ghost commented Jul 18, 2017

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!

DanielRosenwasser Jul 18, 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!

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!

ghost commented Jul 18, 2017

Uh oh!

ghost commented Jul 27, 2017

Uh oh!

ghost commented Aug 3, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DanielRosenwasser Jul 18, 2017 •

edited

Loading