Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parenthesized simple types are not fixed correctly when using array-type array-simple #4844

Closed
MaximeKjaer opened this issue Aug 29, 2019 · 3 comments · Fixed by #4846

Comments

@MaximeKjaer
Copy link
Contributor

commented Aug 29, 2019

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 added a commit to hashml/hashml that referenced this issue Aug 29, 2019
Format twice as a workaround to palantir/tslint#4844
@MaximeKjaer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

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

mbovel added a commit to hashml/hashml that referenced this issue Aug 30, 2019
@ayazhafiz

This comment has been minimized.

Copy link

commented Aug 30, 2019

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 added a commit to MaximeKjaer/tslint that referenced this issue Aug 30, 2019
@MaximeKjaer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.