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

[WIP] empty object type inference #1821

Merged
merged 1 commit into from
Dec 7, 2016

Conversation

subash-a
Copy link
Contributor

@subash-a subash-a commented Dec 5, 2016

PR checklist

Introduced a rule which checks for type inference being {} (empty object type) and then throws an error, this PR is based on discussions in the issue mentioned above. @myitcv any comments on how we can improve this?

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @subash-a! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@myitcv
Copy link
Contributor

myitcv commented Dec 5, 2016

Looks great to me 👍

Copy link
Contributor

@nchen63 nchen63 left a comment

Choose a reason for hiding this comment

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

Looks pretty good, minor changes

@@ -0,0 +1,77 @@
import * as ts from "typescript";
Copy link
Contributor

Choose a reason for hiding this comment

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

license, metadata

import * as Lint from "../index";

export class Rule extends Lint.Rules.TypedRule {
public static EMPTY_INTERFACE_INSTANCE = "Invalid {} type: Explicit type parameter needs to be provided to the constructor";
Copy link
Contributor

Choose a reason for hiding this comment

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

remove Invalid {} type:

class NoInferredEmptyObjectTypeRule extends Lint.ProgramAwareRuleWalker {

public visitNewExpression(node: ts.NewExpression): void {
let checker = this.getTypeChecker();
Copy link
Contributor

Choose a reason for hiding this comment

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

do this once, in a constructor. Store in class member

let nodeTypeArgs = node.typeArguments;
let isObjectReference = (o: ts.TypeReference) => {
/* tslint:disable:no-bitwise */
return ((o.flags & ts.TypeFlags.Reference) === ts.TypeFlags.Reference);
Copy link
Contributor

Choose a reason for hiding this comment

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

use utils.isNodeFlagSet

private isEmptyObjectInterface(objType: ts.ObjectType): boolean {
let typeChecker = this.getTypeChecker();
/* tslint:disable:no-bitwise */
let isAnonymous = ((objType.flags & ts.TypeFlags.Anonymous) === ts.TypeFlags.Anonymous); // the type of the Object will be anonymous
Copy link
Contributor

Choose a reason for hiding this comment

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

use utils.isNodeFlagSet

hasProblematicCallSignatures = true;
}
}
if (isAnonymous && !hasProblematicCallSignatures && !hasProperties && !hasNumberIndexType && !hasStringIndexType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need if statement, just return the condition

@subash-a
Copy link
Contributor Author

subash-a commented Dec 6, 2016

@nchen63 Thanks for the review again, I have made all the suggested changes, however the utils.isNodeFlagSet function would not be useful to check TypeFlags so I can either add a new function to check the type flags or leave the statement as is, awaiting your answer on the same to make the final changes.

@nchen63
Copy link
Contributor

nchen63 commented Dec 6, 2016

Yes, please add this function to utils

The `no-inferred-empty-object-type` rule is useful for capturing the
errors caused by typescript in places where it assumes {} as the
default type since no explicit type has been provided.
@subash-a
Copy link
Contributor Author

subash-a commented Dec 7, 2016

@nchen63 I have added the function isTypeFlagSet to check type flag value, have done the same in the PR #1820 as well.

Copy link
Contributor

@nchen63 nchen63 left a comment

Choose a reason for hiding this comment

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

lgtm

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.

4 participants