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

'no-inferrable-types' and 'typedef' rules conflict #711

Closed
zdychacek opened this issue Oct 3, 2015 · 33 comments
Closed

'no-inferrable-types' and 'typedef' rules conflict #711

zdychacek opened this issue Oct 3, 2015 · 33 comments

Comments

@zdychacek
Copy link

Hi again,

I am not sure if this is issue or design decision, but I can see conflict between rules no-inferrebale-types and typedef.

Ex.

function fn(): void {
    for (let i = 0; i < 100; i++) {
        console.log(i);
    }
}

Snippet from https://github.com/palantir/tslint/blob/master/docs/sample.tslint.json:

 "typedef": [true, ...],
 "no-inferrable-types": true,

When I have these two rules turned on, I get:

error (typedef) test.ts[2, 12]: expected variable-declaration: 'i' to have a typedef

Adding type annotation number to variable i causes in turn following error:

error (no-inferrable-types) test.ts[2, 14]: LHS type (number) inferred by RHS expression, remove type annotation

Is there any way to have these two rules coexisted with each other?

For example, I want to have inferrable variables directly declared with primitive type (as rule doc says: number, boolean or string), but on the other hand, I want to force typedefs on non-primitive types.

Thanks,

O.

@jkillian
Copy link
Contributor

jkillian commented Oct 3, 2015

Good catch, thanks for the heads-up. I added the no-inferrable-types rule without thinking about how it might conflict with the typedef rule, whoops! For now I'd recommend turning one of the two off, or only using some options of the typedef rule.

Longer term, I think we'll either want to integrate no-inferrable-types into the typedef rule or we'll want to at least have TSLint detect conflicting configurations like having both rules on.

@timbru31
Copy link

timbru31 commented Oct 5, 2015

I am facing the same issue.
I've turned off no-inferrable-types for now.

@sushilks
Copy link

sushilks commented Oct 8, 2015

Yes same here, I would like to have ability to have both the rules co-exist.
I would not want typedef rule to kick in if the variable's type can be inferred.
i.e. "no-inferrable-types" should have priority over "typedef"

@helios1138
Copy link

+1

@Connormiha
Copy link

let id: number = 0;
for (let job: string of NAMES_PROFFESSIONS) {
     /** some code */
     id++;
}

(no-inferrable-types) LHS type (number) inferred by RHS expression, remove type annotation

@adidahiya adidahiya removed their assignment Jan 6, 2016
@cronon
Copy link

cronon commented Jan 21, 2016

+1

@octogonz
Copy link

Does your definition of "inferrable" types include constructor assignments?

// BAD (this hurts my eyes to read)
let labels: Map<string, string> = new Map<string, string>();
// GOOD (type is obvious)
let labels = new Map<string, string>();

but also...

// BAD (in a diff, it's not obvious what this type is)
let labels = this.buildLabels();
// GOOD
let labels: Map<string, string> = this.buildLabels();

@Avol-V
Copy link

Avol-V commented Jan 28, 2016

Yes, it's dangerous. If I want to simplify my code and prevent to use type declaration for directly initialized variables, I can't do this strictly and this brings to such thing:

let x = 2;
let y;
let z: number;

x = 1;
y = 1;
z = 1;
x = 's'; // Type 'string' is not assignable to type 'number'
y = 's'; // It's OK
z = 's'; // Type 'string' is not assignable to type 'number'

It's may be a very useful option to allow skip type declaration only for initialized variables.

@Avol-V
Copy link

Avol-V commented Feb 24, 2016

… and really not only for primitive types, as @pgonzal says!
Look at this, it's terrible:

const onChange: () => void = () => this.update();

@englercj
Copy link

englercj commented Mar 9, 2016

👍 Ideally (imo) I would like a way to say "always require a typedef, unless the type is inferrable". Which I don't think is possible right now.

@abierbaum
Copy link
Contributor

I ran into this and made an ignore-params flag that helps out in my case. I want to force typedefs for all method/function parameters even when they can be easily inferred.

See PR: #1190 if you want to try it out

@corydeppen
Copy link
Contributor

It's been a while since the original issue was submitted. Is the recommend option at this point to disable no-inferrable-types and include the type on everything? Anything else to try and enable and combine both no-inferrable-types and typedef seems like a hack and results in a bunch of pointless warnings. Hoping for a better solution in the near future.

@adidahiya adidahiya added this to the TSLint v3.x milestone Jun 13, 2016
azdavis added a commit to azdavis/azdavis.net that referenced this issue Aug 18, 2016
@adidahiya adidahiya modified the milestones: TSLint v3.x, TSLint v4.x Sep 1, 2016
@jkillian
Copy link
Contributor

jkillian commented May 15, 2018

Would accept a PR to:

  • add an option to typedef that lets it ignore cases where no-inferrable-types says not to provide a type

The typedef accepts a configuration. So maybe just another configuration just to ignore the type definition if the type is inferrable, meaning 'Just type wherever is not being initialized.'

The typedef rule is for people who like having explicit type definitions in their codebase. If you desire the above behavior to "just type wherever is not being initialized", you're better off disabling typedef and making sure you have noImplictAny enabled as a TypeScript compiler option.

There's also the tricky case where some things that are initialized need a typedef anyways, as in the following snippet:

interface Literal {
    field: "value"
}

const literal0 = {
    field: "value",
};
const literal1: Literal = {
    field: "value",
};

const func = (obj: Literal) => { };
func(literal0);  // Error! Type 'string' is not assignable to type '"value"'.
func(literal1);

@jkillian
Copy link
Contributor

Also, while we get this fixed, wanted to mention that there are likely some great 3rd-party rules out there, like no-unnecessary-type-annotation(https://github.com/ajafff/tslint-consistent-codestyle/blob/master/docs/no-unnecessary-type-annotation.md) for example. If anyone knows of any other 3rd-party rules that give the desired behavior, please post them here and we can officially recommend them or adopt them into core if it makes sense.

@michaeljota
Copy link

@jkillian thanks for the recommendation, I think that's actually what I wanted. There is a very good post about avoiding any type: Don't use "naked any", create an "any interface" instead.

About:

interface Literal {
    field: "value"
}

const literal0 = {
    field: "value",
};
const literal1: Literal = {
    field: "value",
};

const func = (obj: Literal) => { };
func(literal0);  // Error! Type 'string' is not assignable to type '"value"'.
func(literal1);

I don't see how this could be an undesired behavior nor a tricky case. You want to make sure that obj have a property field with value value, and even when you are initializing literal0 with a property that seems like the constrains, you could modify that to another string.

I know is not a good use case, but most of the cases when you are using a literal, you probably want that literal, not a primitive.

@sandrocsimas
Copy link

I have the following configuration:

"no-inferrable-types": true,
"typedef": [true, "call-signature", "parameter"],

And this code:

private static readonly DEVICE_UID: string = 'device_uid';
private static readonly DEVICE_PLATFORM: string = 'browser';
private static readonly AGENT_DEFAULT_ICON = 'http://localhost:3000/icon.png';

Why I'm not getting error in the two first declarations?

@estaub
Copy link

estaub commented Jun 9, 2018

@sandrocsimas Interesting, but off-topic I think; AFAICT that problem's unrelated to this issue. I'd suggest you start another issue (fwiw!).

@sandrocsimas
Copy link

@estaub , yes I will. I'm getting the same behavior even without the typedef rule.

@michaeljota
Copy link

@sandrocsimas that's because it's a readonly property, and such Typescript infer its type as a literal. Typing it as a string you are telling that it should have a string, it does not necessarily will have that literal value and the value should not change statically.

kopelli added a commit to kopelli-forks/bitburner that referenced this issue Jun 25, 2018
After discussing the conflicting 'typedef' vs. 'no-inferrable-types'
TSLint rules (palantir/tslint#711) in the
Discord #development channel, the conclusion was to disable this rule in
order to be uniform and explicit in what all declarations are. Although
it's more verbose, it will help us out in the long run.
@FiretronP75
Copy link

It would be nice to have a 'require-typedef-except-inferrable' rule.

@michaeljota
Copy link

@FiretronP75 as @jkillian said, that's just noImplicitAny option of the TSC.

@FiretronP75
Copy link

FiretronP75 commented Aug 28, 2018

@michaeljota thanks, I didn't realize the noImplicitAny option of the compiler gives exceptions for inferrable. It still would be nice to have in tslint though, for the option of making it a warning instead of breaking compile, and for having the tslint comment flags.

@michaeljota
Copy link

michaeljota commented Aug 28, 2018

I see why this would be something wanted, but having no-unused-variables as an example, I don't think use cases covered by the TSC are going to be supported by the TSLint team. I know that is not the same an linter error than a compiler error but at the end, they both are about written better code. Now days with solutions as Webpack or Parcel that allows you to compile and run the code even with TSC errors, I don't see this as a real issue.

@Tanja-4732
Copy link

Has this been fixed in the latest version?

@michaeljota
Copy link

I still don't think this is on the roadmap. You should consider using noImplicitAny from the TSC

@JoshuaKGoldberg
Copy link
Contributor

☠️ TSLint's time has come! ☠️

TSLint is no longer accepting most feature requests per #4534. See typescript-eslint.io for the new, shiny way to lint your TypeScript code with ESLint. ✨

It was a pleasure open sourcing with you all!

@JoshuaKGoldberg
Copy link
Contributor

🤖 Beep boop! 👉 TSLint is deprecated 👈 (#4534) and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋

@palantir palantir locked and limited conversation to collaborators Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests