[eslint-config] Proposal: Allow inferred types for local variables

[octogonz]

Is this a feature or a bug?

• Feature
• Bug

We're converting a migrating a bunch of HBO code to use @rushstack/eslint-config, and in a number of cases it is awkward to have to declare types for local variables. The Rush Stack ruleset has required full type declarations for 5 years now, and lots and lots of code has been written using those rules.

Good reasons to be strict

Generally declaring types for local variables greatly improves readability. Compare this:

// Confusing!
function f(report: Report, owner: string): string[] {
    const results = [];

    const children = report.filter(true);
    for (const child of children) {
        const key = report.getKey(owner)
        const map = child.getByTag();
        const value = map.get(owner);
        if (value) {
            results.push(value.zip);
        }
    }
    return results;
}

with this:

// Better!
function f(report: Report, owner: string): string[] {
    const results: string[] = [];

    const children: Section[] = report.filter(true);
    for (const child of children) {
        const key: SectionId = report.getKey(owner)
        const map: Map<string, ICity> = child.getByTag();
        const value: ICity | undefined = map.get(owner);
        if (value) {
            results.push(value.zip);
        }
    }
    return results;
}

More examples and background are explained in the comments for the config.

Reasons to be a bit more lax

But typedef can be awkward for certain cases such as:

local data structures

function f(): void {
  // Nobody cares to describe the type of this, because it is not used outside this function.
  const samplePayload = { a: 1, b: 2 };

  webService.postJsonBlob(samplePayload);
}

algebraically composed types (that are not accessible outside their scope)

interface IThing {
  a: number;
  b: number;
}

function f(things: IThing[]): void {
  // This type is an ad hoc structure that would be cumbersome
  // to describe explicitly.  This sort of thing arises frequently in code
  // that processes JSON objects to produce reports (similar to SQL queries)
  const report = lodash.keyBy(things, 'a');  // { '1': { a: 1, b: 2 } }
  . . .
}

simple redundancies

This was a classic example where declarations are not always useful:

const map: Map<string, Promise<void>> = new Map<string, Promise<void>>();

(Although I eventually realized for this specific API you can get around it like this:)

const map: Map<string, Promise<void>> = new Map();

destructuring

It's also worth mentioning that the @rushstack/eslint-config ruleset today already makes a special exception for destructuring expressions on similar grounds:

interface IOptions {
    a: number;
    b: string;
    c: number;
}

function f(options: IOptions): void {
    // accepted already
    const {a, b} = options;
    
    // declaring the type often does not provide any actual new information
    // const {a, b}: IOptions = options;

    // and not destructuring does not provide new information either
    console.log(options.a);
    console.log(options.b);
}

Proposal

Let's follow the model of C# and allow type inference for local variables, whose signatures are never seen outside of the scope where they are used.

For code bases that are accustomed to requiring strict types everywhere, we can provide an "mix-in" import to restore the stricter behavior.

The motivation for explicit types is really about:

• making code easier to read: being explicit is especially helpful for code reviewers looking at a Git diff (without fancy VS Code tooltips)
• encouraging better software designs: For objects that are NOT local variables, explicitly declaring types prevents anonymous types, which makes developers go through the useful exercise of declaring an interface, choosing a name for it, and discussing its design in a code review

We can balance this with a couple counter goals:

• encouraging adoption of our coding conventions - typedef is the rule people cite most when resisting our coding conventions
• reducing lint suppressions for the edge cases pointed out above

This proposal arguably preserves the first two benefits, while adding the second two benefits.

@iclanton @hbo-iecheruo @KevinTCoughlin @johnguy0

[KevinTCoughlin]

Let's follow the model of C# and allow type inference for local variables, whose signatures are never seen outside of the scope where they are used.

I'm OK with the proposal, though a fan of the typedef rule myself for the benefits stated above; specifically the improved readability of code. As you stated, we've received feedback from adopters of Rush Stack tooling that the rule is a barrier so this seems like a good balance and eliminates the need for lint suppressions. I know other folks have stronger opinions on this so it would be good to get their feedback. Thanks for starting the discussion @octogonz.

[iclanton]

I'm okay with this proposal as long as:

1. Projects can opt-in to the stricter ruleset and
2. This more lax ruleset does not forbid local typings

I'm personally a big fan of strict typings, but I understand why some people consider that strictness cumbersome. If this promotes adoption, I'm (somewhat begrudgingly) cool with it.

[johnguy0]

Personally I prefer the stricter behavior of requiring types for local variables, but I could just be overly pedantic.

Personal opinion aside, I think this change might be worthwhile. I was strongly opposed until I gave the inconsistency around destructuring some more thought. The main argument to always have the types declared is to avoid the need for a reader (not in an IDE that can infer and display the type, e.g. VS Code) to quickly understand the types being used. As @octogonz pointed out, we don't currently enforce this rule when destructuring which leads to having to still hunt for a function signature or type definition anyways. Consistency is probably more important than the mild inconvenience.

This rule change should definitely be configurable. I think I'm ok with the default behavior being the looser option.

[octogonz]

Here's the PR: #2219

The @typescript-eslint/typedef rule does not distinguish variable scope, so I had to implement a new rule @rushstack/typedef-var that supplements it with this additional logic.