[no-inferrable-types][typedef] Rules clash in 'all' config

[fernandopasik]

Seems to have a clash between those rules in all config. Am I missing something to make this case pass?

Repro

{
  "extends": [
    "airbnb-base",
    "plugin:@typescript-eslint/all",
  ]
}

class HelloWorld {
  protected who?: string = 'world';
}

This will result in
error Type string trivially inferred from a string literal, remove type annotation @typescript-eslint/no-inferrable-types

class HelloWorld {
  protected who = 'world';
}

This will result in
error expected who to have a type annotation @typescript-eslint/typedef

Versions

package	version
@typescript-eslint/eslint-plugin	2.0.0
@typescript-eslint/parser	2.0.0
TypeScript	3.5.3
ESLint	6.1.0

[a-tarasyuk]

@bradzacher, In order to fix this issue, need to change all.json config?

[bradzacher]

Technically in this case, it is actually a bug in no-inferrable-types.
foo?: string = 'str' is not inferrable, as the property is optional.

But it also seems like an intractable problem to solve, as the two rules are at-odds in this, and pretty much every case.
all.json includes every single rule turned on with the default config. I don't think we can satisfy the requirement that every single rule will work in harmony with one-another. With every rule we add, the number of combinations that exist grows exponentially (O(n!)).

This might be solvable by adding an option to typedef so that it ignores properties/arguments/variable decls with default values?

[scottohara]

We've just had a round of rule deprecations in v2.0.0, but it sounds like no-inferrable-types could potentially be deprecated/merged into typedef (and later removed in v3.x), given that the two rules check similar, but mutually exclusive things.

It may require changing the options for typedef from booleans to strings though.

For example, something like this might work?

Correct code

/* @typescript-eslint/typedef: ['error', { variableDeclaration: 'always' }]` */
const a: string = 'a string';

/* @typescript-eslint/typedef: ['error', { variableDeclaration: 'if-not-inferred' }]` */
const a = 'a string';

Incorrect code

/* @typescript-eslint/typedef: ['error', { variableDeclaration: 'always' }]` */
const a = 'a string';

/* @typescript-eslint/typedef: ['error', { variableDeclaration: 'if-not-inferred' }]` */
const a: string = 'a string';

Just a thought.

[bradzacher]

(reopening because the bug is fixed, but there is a separate issue at hand here)

[bradzacher]

I'm not sure if typedef and no-inferrable-types belong together.
I think they're attempting to achieve different things.

Originally the TSLint rule was there to cover a bunch of holes in the coverage of noImplicitAny, such as:

// yes, these used to be allowed by noImplicitAny
interface Foo {
  prop;
}
class Foo {
  prop;
}

With each version of TS, they have improved the coverage of noImplicitAny. Since Joshua implemented the rule in this plugin in #581, those two cases have been handled.

There's now only one case (that I know of) that isn't caught by no-implicit-any:

let foo;

I think this is entirely intentional though (as silly as it looks).

---

IMO - I see our version of typedef as more of a "migration rule" - for helping move a codebase onto noImplicitAny. Because of this, I'd almost want to treat typedef as a special case, and remove it from all.json.

I think no-inferrable-types makes sense as is, and works well as a sister rule to no-unnecessary-type-assertion. Though nit could be made more generic (#907) instead of just handling primitives.

[ark120202]

Technically in this case, it is actually a bug in no-inferrable-types.
foo?: string = 'str' is not inferrable, as the property is optional.

class Foo {
  foo? = 'str'
}

Appears to have no errors (with strictest options) and infers the same foo?: string | undefined type. Do you mean that it's not inferrable in the user expectations sense, or am I missing some edge case there?

[bradzacher]

that is weird syntax that I didn't even know existed!
I doubt most users would know that as well.

I think it's probably better to not to treat it as inferrable, as we'd have to create a special fixer specifically for this case (as I don't think this syntax is valid in any other location).

[glen-84]

This issue reminds me of palantir/tslint#711.

IMO, foo?: string = 'str' is trivially inferable as string | undefined and should result in an error under no-inferrable-types.

[bradzacher]

I don't agree because ? is a very uncommon thing in class properties, let alone an optional property with a default value. TBH I'm not sure why that is semantically allowed by TSC.
There's only one other place that supports both an optional type and a default value: function arguments, however function arguments cannot have both at the same time, because it makes no sense to.

Additionally, string | undefined is only an inferrable type in strict mode, which not all users use.

[fernandopasik]

The same problem happens for default arguments in functions in all config

expected a type annotation - eslint(@typescript-eslint/typedef)

const myFunction = (param1 = false) {
}

Type boolean trivially inferred from a boolean literal, remove type annotation.eslint(@typescript-eslint/no-inferrable-types)

const myFunction = (param1: boolean = false) {
}

[fernandopasik]

What about using the function isInferrable (https://github.com/a-tarasyuk/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/no-inferrable-types.ts#L101) in the typedef rule (https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/typedef.ts)

[bradzacher]

this shouldn't happen any more as typedef no longer has any options on by default

[fernandopasik]

True, this is not a problem anymore.

[jonnytest1]

then how would i specify that i want to have typedef except if it is infered ?