Tslint: Parenthesized simple types are not fixed correctly when using array-type array-simple

[MaximeKjaer]

Bug Report

• __TSLint version__: 5.19.0
• __TypeScript version__: 3.5.3
• __Running TSLint via__: CLI

Reproduction using TSLint Playground

Playground link

TypeScript code being linted

type A = (number)[];

with tslint.json configuration:

{
    "extends": ["tslint:recommended"],
    "rules": {
        "array-type": [true, "array-simple"]
    }
}

Actual behavior

1. Running tslint --fix, "fixes" (number)[] to Array<number>. This raises an error because of the "array-simple" rule.
2. The CLI error message says to use Array<T> instead of T[]

Expected behavior

1. Running tslint --fix should fix (number)[] to number[]. No error should be raised.
2. The CLI error message should say to use T[] instead of (T)[].

[MaximeKjaer]

I'm actually wondering what the expected behavior should be. I suggested that (number)[] should be fixed to number[], but I'm starting to think that no error should be raised in the first place, i.e. that (number)[] be considered correct.

There are no rules that fix the following (playground link):

type A = (number);
type B = ((number));

Since TSLint doesn't consider this an error, and doesn't fix it, it would be inconsistent to consider the following an error:

type A = (number)[];
type B = ((number))[]

As a side note, some formatters (notably, Prettier) remove parentheses in the first example, but not in the second, which I suppose means that they consider the latter to be stylistically acceptable.

Perhaps the solution to this issue is simply to consider a parenthesized type to be "simple" if its contents are simple. In other words, the fix would be to ignore parentheses when deciding whether something is a simple type.

I'd therefore like to revise the expected behavior:

Expected behavior

No error is raised for (T)[] in array-simple mode

[ayazhafiz]

No error is raised for (T)[] in array-simple mode

I am inclined to favor this behavior as well. It's also interesting to note how TSLint fixes parenthesized non-simple array types.

type A = ((number&string))[]; // original
type A = Array<(number&string)>; // TSLint-fixed

Is the desired behavior to remove only one layer of parentheses, or all of them (or maybe even none at all)? I'm not sure, but I think may decision made should be consistent.

The parentheses in (number&string)[] have a semantic role, whereas the parentheses in (number)[] are a stylistic choice. If we choose to preserve stylistic choices made with parentheses, it makes sense to strip parentheses only where they no longer play a semantic role.

What I mean by this is

• (number&string)[] is fixed to Array<number&string> (the only pair of parentheses is stripped because its semantic role is replaced by generics syntax)
• ((number&string))[] is fixed to Array<(number&string)> (one pair of parentheses in the original form plays a semantic role; the other is stylistic)
• (number)[] is not fixed because its parentheses are a stylistic choice.

This goes a little beyond the scope of this PR, but it's one argument for accepting (T)[] as valid.

[MaximeKjaer]

I agree with this: I think it's safe to remove parentheses that have a semantic role, but that it is best to let a formatter decide what to do with parentheses serving a stylistic role.

So this bug is fixed by:

• Keeping the current --fix behavior
• Widening the definition of "simple type" to include parenthesized simple types: (T) is considered simple if T is simple. This means (T)[] is acceptable with the array-simple option.

I submitted a PR doing just that :)

[JoshuaKGoldberg]

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

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