[array-type] False negative when using 'array-simple'

[Shinigami92]

Repro

{
  "rules": {
    "@typescript-eslint/array-type": ["warn", { "default": "array-simple", "readonly": "generic" }]
  }
}

public get selectableTemplates(): (ExpandedTextTemplateModel & IsSelectable)[] {
  return this.templates.filter(({ isSelectable }) => isSelectable);
}

Expected Result

Show warning and suggest to write Array<ExpandedTextTemplateModel & IsSelectable> instead

Actual Result

No warning shown

Additional Info

This is working as expected and logs many (~116) warnings

{
  "rules": {
    "@typescript-eslint/array-type": ["warn"]
  }
}

So I think the problem/bug has something to do with how I setup my preferred option object 🤔

Versions

package	version
@typescript-eslint/eslint-plugin	3.7.0
@typescript-eslint/parser	3.7.0
TypeScript	3.9.7
ESLint	7.5.0
node	v14.1.0
npm	6.14.4

[Shinigami92]

When I just remove , "readonly": "generic" I see warnings again 🤔

[Shinigami92]

Also it seems that my case has no test defined ^^' Is it that special?
https://github.com/typescript-eslint/typescript-eslint/blob/fd90e31019afb81aee8f83d9632096236069520b/packages/eslint-plugin/tests/rules/array-type.test.ts

[Shinigami92]

I'm new to the project's code and don't know how to debug or test only this rule.
So it's hard to test, cause I always running 17197 tests...

I guess when using { default: 'array-simple', readonly: 'generic' }, following code snippets run into returning out of the function. But I'm not sure 🤷‍♂️

So I found out this:
Hopefully this can help someone else, or someone can guide me a little bit

TSArrayType(node): void

typescript-eslint/packages/eslint-plugin/src/rules/array-type.ts

Lines 207 to 212 in fd90e31

	if (
	(isReadonlyGeneric && !isReadonly) ||
	(isReadonlyArray && isReadonly)
	) {
	return;
	}

isReadonlyGeneric => true
isReadonlyArray => false

(isReadonlyGeneric && !isReadonly) => maybe true or false, depends on isReadonly
(isReadonlyArray && isReadonly) => always false

---

TSTypeReference(node): void

typescript-eslint/packages/eslint-plugin/src/rules/array-type.ts

Lines 275 to 281 in fd90e31

	if (
	!(isArrayType || isReadonlyArrayType) ||
	(readonlyOption === 'generic' && isReadonlyArrayType) ||
	(defaultOption === 'generic' && !isReadonlyArrayType)
	) {
	return;
	}

readonlyOption => 'generic'
defaultOption => 'array-simple'

!(isArrayType || isReadonlyArrayType) => depends on isArrayType and isReadonlyArrayType
(readonlyOption === 'generic' && isReadonlyArrayType) => depends on isReadonlyArrayType
(defaultOption === 'generic' && !isReadonlyArrayType) => always false

[bradzacher]

There's a vscode launch config which will allow you to run any given singular test in the vscode debugger.
Failing that we just use jest, so you can simply cd into the package directory and use jest to filter to a specific test.

We don't do any magic on top of the commands like other repos do. yarn test just maps directly to yarn jest

---

From what I can see, it looks like you're correct - the logic for the option checks looks wrong.
The options checks actually look really weird and wrong. Might be worth just revisiting them and making that logic all more sane.

[Shinigami92]

Oh my bad!
Because I'm working on Windows @home, I tried yarn test tests\rules\array-type.test.ts, but then no tests were found.
Now I tried yarn test "tests/rules/array-type.test.ts" and that works :)
But I even could simplify this to just yarn test array-type and this results the same ^^

Thanks for leading the way, now I can console.log the 💩 out of it 😅

[Shinigami92]

Ok, I have now created a matrix that lists all possible combinations and their expected results

The first two columns represent the input values
The header of column 3-6 are cases and the column list the expected outcome

defaultOption	readonlyOption	number[]	Array	Array<Foo & Bar>	readonly number[]	ReadonlyArray	ReadonlyArray<Foo & Bar>
array		number[]	number[]	(Foo & Bar)[]	readonly number[]	readonly number[]	readonly (Foo & Bar)[]
array	array	number[]	number[]	(Foo & Bar)[]	readonly number[]	readonly number[]	readonly (Foo & Bar)[]
array	array-simple	number[]	number[]	(Foo & Bar)[]	readonly number[]	readonly number[]	ReadonlyArray<Foo & Bar>
array	generic	number[]	number[]	(Foo & Bar)[]	ReadonlyArray	ReadonlyArray	ReadonlyArray<Foo & Bar>
array-simple		number[]	number[]	Array<Foo & Bar>	readonly number[]	readonly number[]	ReadonlyArray<Foo & Bar>
array-simple	array	number[]	number[]	Array<Foo & Bar>	readonly number[]	readonly number[]	readonly (Foo & Bar)[]
array-simple	array-simple	number[]	number[]	Array<Foo & Bar>	readonly number[]	readonly number[]	ReadonlyArray<Foo & Bar>
array-simple	generic	number[]	number[]	Array<Foo & Bar>	ReadonlyArray	ReadonlyArray	ReadonlyArray<Foo & Bar>
generic		Array	Array	Array<Foo & Bar>	ReadonlyArray	ReadonlyArray	ReadonlyArray<Foo & Bar>
generic	array	Array	Array	Array<Foo & Bar>	readonly number[]	readonly number[]	readonly (Foo & Bar)[]
generic	array-simple	Array	Array	Array<Foo & Bar>	readonly number[]	readonly number[]	ReadonlyArray<Foo & Bar>
generic	generic	Array	Array	Array<Foo & Bar>	ReadonlyArray	ReadonlyArray	ReadonlyArray<Foo & Bar>

Is the matrix correct or have I done something wrong?

[Shinigami92]

typescript-eslint/packages/eslint-plugin/src/rules/array-type.ts

Line 25 in fd90e31

case AST_NODE_TYPES.TSArrayType:

This line does always pass, also if input is something like (string & number)[], but string & number is not "simple" 🤔

[bradzacher]

Your matrix looks correct to me. I don't think anyone's ever gone so far as to enumerate the options like that. That's a great way to conceptualise it!

It looks like the rule's logic is a bit wrong in how it handles things. Or at least it's not clear.

(string & number) should match TSParenthesizedType, which is not a "simple" type.
The rule's source looks like it handles things a bit weird. The separate readonly option was added in after the rule's creation, so it was probably just built wrong.

Feel free to gut and rewrite the rule however you see fit to fix this issue!

[Shinigami92]

I worked on this code and researched it for about 3 hours today
And I also think that it may be better and easier to rewrite the whole rule

There is also the problem that the error messages do not always fit well, e.g. if it should be readonly it is suggested to use Array <T> instead of ReadonlyArray<T>

But such changes are a minor change instead of a bug fix 😅

---

Ok, give me a couple of days and I'll try to tackle this 💪

But I think I will also alter tests and add more tests

[bradzacher]

There's no requirement to only treat this as a bugfix.
Feel free to do whatever you think. If it improves the rule and fixes the bug then that's a net win for everyone :)

Thanks for putting so much care and effort into this!

[Shinigami92]

Aside whats currently made, what do you expect {default:'array'} --fix should made?

// Input
let numArrArr: Array<Array<number>> = [[1], [2, 3]];



// 1 - only outer
// with message "Array type using 'Array<Array<number>>' is forbidden. Use 'Array<number>[]' instead."
Array<number>[]
// no further warings 🤔 


// 2 - only inner
// with message "Array type using 'Array<Array<number>>' is forbidden. Use 'Array<number[]>' instead."
Array<number[]>
// no further warings 🤔 


// 3 - all at once
// with message "Array type using 'Array<Array<number>>' is forbidden. Use 'number[][]' instead."
// no second message, jump directly to number[][]
number[][]


// 4 - from outer to inner
// with message1 "Array type using 'Array<Array<number>>' is forbidden. Use 'Array<number>[]' instead."
// with message2 "Array type using 'Array<number>[]' is forbidden. Use 'number[][]' instead."
number[][]


// 5 - from inner to outer
// with message1 "Array type using 'Array<Array<number>>' is forbidden. Use 'Array<number[]>' instead."
// with message2 "Array type using 'Array<number[]>' is forbidden. Use 'number[][]' instead."
number[][]

Edit: And I dont even know what should happen if someone uses Array<Array<Array<Array<Array<number>>>>>

IMO 3, 4 and 5 are the only possible options 🤔
3 could be REALLY hard to implement
4 is nice readable for human
5 is maybe easier to implement 🤷‍♂️ (but I dont really know)

[bradzacher]

This rule only needs to be concerned with one array type at a time.
ESLint should handle applying multiple fixers that overlap.

I.e. for {default:'array'}, this should happen:

  type T = number[][][];
//         ^^^^^^^^ error 1     - wants to fix to Array<number>[][]
//         ^^^^^^^^^^ error 2   - wants to fix to Array<number[]>[]
//         ^^^^^^^^^^^^ error 3 - wants to fix to Array<number[][]>

The important thing here is we should avoid using the replaceText fixer - as this creates a large fix.

Instead, for example the fixers should be:

  type T = number[][][];
//         ^^^^^^^^ error 1     - wants to insert "Array<" before number, and replace "[]" with ">"
//         ^^^^^^^^^^ error 2   - wants to insert "Array<" before number[], and replace "[]" with ">"
//         ^^^^^^^^^^^^ error 3 - wants to insert "Array<" before number[][], and replace "[]" with ">"

This makes it easier for ESLint to apply the fixers in order.

[Shinigami92]

So it is 5 - from inner to outer I think?

Beside that, your example confuses me a bit ^^'

{default:'array'} should not print any errors for number[][][] cause this is already the best form for 'array'
Your errors should appear when using 'generic'

---

But damnd this is a hard one task to tackle XD
If you want and have time, you can try in parallel to me working on this task
Cause I currently don't know if I can handle and fix this easily within a timeframe of 1-2 weeks 😅

[bradzacher]

sorry, yes - i meant generic not array!

The ESLint docs provide guidance on fixes:
https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes

In a nutshell, ESLint does all of the heavy lifting of applying fixes to the file, resolving conflicts, etc.
When you ask ESLint to fix a problem via the --fix command, ESLint will actually do multiple passes over the file so that it can apply all fixers, as well as fix any errors that arrise as part of those fixers.

A simple example, imagine you have a fixer which adds quotes to object keys:

({ foo: 1 })
// fixes to
({ 'foo': 1 })

But what happens if the user prefers double quotes? Well then the quotes eslint rule fires. But because ESLint does multiple passes of fixes, eslint will apply the above fix, scan and find the quotes problem, apply its fix, and then scan and attempt t o find any more lint rules again.

So in our case, it's not necessary for the rule itself to care that a user might write number[][][] and that it should be fixed to Array<Array<Array<number>>>. Instead the rule just needs to care about one case at a time:

1. It needs to care that number[] should be fixed to Array<number>
2. It needs to care that number[][] should be fixed to Array<number[]>
3. It needs to care that number[][][] should be fixed to Array<number[][]>

ESLint will handle the rest.

In terms of order - outside in vs inside out - that's dependent on what the AST produces. In this case the AST is constructed outside in, so that's the order in which the rule will report errors.

---

In regards to my second point about which fix API to use.
There's a few methods on the fix object (detailed in https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes).
The thing we should do is attempt to use the insertTextBefore and insertTextAfter methods instead of the replaceText method.

This is so that each fix is as small as possible, to make it easier for ESLint to merge the fixes and apply it all at once.

[Shinigami92]

Just want to let you know that I may come later to this again
Currently I'm involved in other projects and next week I'm in vacation (maybe I have time there, maybe I will spend more time with IRL humans 😅)

[Shinigami92]

@bradzacher I'm so sorry
I'm to much involved in other tasks and don't find time to solve this issue

I hope someone else can benefit from my research an fix this anytime

[sonallux]

I going to work on this issue 🚀