-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Enhancement: [no-unnecessary-type-parameters] Special case tuples and parameter arrays as a "single use" generic type #9529
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
Comments
But what about something like |
Why write |
To play devil's advocate,
This introduces unnecessary unsoundness with
const s = [];
s.push('foo'); // TS ERROR - Argument of type 'string' is not assignable to parameter of type 'never'. Either of the following would work, though - const s1: string[] = [];
// note that this is basically identical to gimmeEmptyArray<string>()
const s2 = new Array<string>(); In any case, I would say my interpretation of @Josh-Cena's comment is not "but how else would you initialize an empty array?", but instead "this is a simple, easy-to-follow example of a pattern that could lead to a false positive, and some instances of which might have real world applicability". So, the question stands, of whether it's better to err on the side of false positives or false negatives. |
News to me that I see two problems with this example, though: const s = [];
s.push('foo'); // TS ERROR - Argument of type 'string' is not assignable to parameter of type 'never'. First, if you plug this into the playground, you'll see that TS allows it! An empty array is one of a few special patterns that trigger Evolving any behavior. The type of Second, if you need the type annotation for some reason, is there an argument for The only argument for const arr = [].concat(['a', 'b', 'c']);
// ~~~~~~ Error: No overload matches this call.
const arr2 = gimmeEmptyArray<string>().concat(['a', 'b', 'c']); // ok On the other hand, why write that instead of |
@kirkwaiblinger That's about what I was saying. My argument for this does not just apply to The
I can offer several use cases for
|
Off topic, but
I thought so, too, until I plugged it into the playground. It appears to be tsconfig dependent, with |
You also need to set |
Factory functions are definitely an interesting case. Something like: function dict<T>(): Map<string, T> { return new Map(); } seems fine and does save you some typing. It would be helpful if there were some way to indicate that a type parameter must be specified explicitly and not inferred, which I think is what you want in the factory function case. (
Arrays are special in JS / TS, though. They have special syntax to construct them, tsc special-cases them in several places (e.g. evolving array types), and I believe it's impossible to create an object literal that's assignable to an array in the way that you can for other objects (see my "contrived example" in the original issue for There are clearly concerns about arrays, but are there any objections to treating tuple types as "singular?" The idea there is to disallow code like: declare function takeOne<T>(arg: [T]): void;
declare function returnPair<T>(): [T, string]; The empty container issue isn't relevant here, since the type of an empty tuple is |
Aren't factory functions of that type instances of "return-only generics"? Shouldn't we treat them cautiously anyway? I remember one golden rule about this. |
In general, yes, because they are a form of assertion, but in this case it's sound and probably necessary. |
So this would be a safe exception to another restrictive version of this rule as well, one that would completely disallow return-only generics. Currently if you just wrap the returned type parameter in whatever generic type you want, the rule is fine with it. |
For reference, this is probably the section of the handbook being referenced: https://www.typescriptlang.org/docs/handbook/2/functions.html#type-parameters-should-appear-twice |
Right.
This is stepping way, way back, but are there any general guidelines about when to accept a false positive as a limitation of a rule, and when to take it as evidence that something should not be a rule? I feel like the existing lint rules are all over the place on this. |
I don't think there are fixed guidelines. It depends on empirical evidence of whether the false positives would account for a significant portion of code people write or if such cases are "fishy" in the first place. I would say it's better to be safer here and prefer false negatives to false positives, and gradually add more error conditions over time when we feel they are well-defined enough. |
Yeah. For example, we can't even rely on functions being single-line:
Roughly copying some apps that track resource allocations aggressively: function createSet<T>() {
myFancyLogger("Making a new set...");
return new Set<T>();
}
function createSetLabeled<T>(label: string) {
myFancyLogger("Making a new set: " + label + "...");
return new Set<T>();
} We also can't be sure that allowing just the built-ins such as All that being said, I would be in favor with us adding in an opt-in, nothing-by-default option to allowlist certain types. It could use the standard format like Adding in an allowlist would let us learn from public use how users use this, and give folks an escape hatch for the surprising limitations of the rule. |
It does feel like we're heading towards adding an option, doesn't it? The I want to reference one of my favorite blog posts here: Why not add an option for that? If there are enhancements we can make universally, without adding an option, it will make this rule easier to use and maintain. There are two possible improvements here that no one has identified any downsides for:
If nobody can think of a downside to these, then they'd be nice, option-free additions this rule. The tuple change in particular seems uncontroversial. Assuming we are going to add something like the
typescript-eslint/packages/type-utils/src/TypeOrValueSpecifier.ts Lines 13 to 16 in d9dba42
The idea would be to mark every generic type in The new errors in the test file were a mix of factory functions (
There were no new false positives. This feels validating and makes me think that |
Similar to #8765 (comment), it would be really nice if we can find a name that's:
...but I can't think of anything better than
👎 from me on this for now. There are a bunch of lib types that we wouldn't want this for. This issue's original premise of function createStack<T>(): T[] {
return [];
}
const stack = createStack<string>();
stack.push("abc");
stack.pop(); // $ExpectType string | undefined Similar with:
Tentative 👍. I can't think of a code snippet where specifically
Similarly tentative 👍. Maybe there's something around |
These two do sound like the greatest common factor that we can agree on. |
@Josh-Cena I aligned my PR with these common factors, and added a test. Documentation not yet aligned, I commited this just to get some feedback |
While returning an empty array with a single-use I'm curious which (if any) interfaces in
I guess it depends how broad we want to be about our interpretation of "factory function." Since TypeScript doesn't track Promise error types, async function emptyPromise<T>(): Promise<T[]> { return []; }
async function nullPromise<T>(): Promise<T | null> { return null; } That second example makes me wonder about this: function getNull<T>(): T | null { return null; } This looks fishy. Why not just have a // invalid: may as well return readonly never[]
function factory<T>(): readonly T[] { return []; } |
Good question. I don't know that we have a formal list; the definition kind of wavers depending on the context. Similar with built-in collection classes: but
No but that would be great. We've wanted to have more comprehensive end-to-end testing of downstream & upstream libraries for a while (e.g. #1752)... but haven't had time to make much progress.
👍 agreed. |
Anyway, coming over from #9536 (comment), I just want to confirm: what do we have consensus on? My interpretation is two things:
...where those two list items would be separate PRs (so the second would likely spin out into its own issue). Is that right? |
Btw, #9793 is that. |
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Link to the rule's documentation
https://typescript-eslint.io/rules/no-unnecessary-type-parameters/
Description
For purposes of counting whether a generic type parameter is used twice,
T[]
/Array<T>
should only count as using it once. I'm not aware of any realistic scenario where this would lead to a false positive.We decided to defer this to a follow-up in the initial implementation (see links below). So here's an official request for the follow-up.
Fail
Pass
Additional Info
See https://twitter.com/jfet97/status/1810703565280759964 and #8173 (comment) for context on why this doesn't work as expected today.
See #8173 (comment) for previous discussion of special-casing
Array
and friends.See #8173 (review) for a contrived example of how this could yield a false positive for
Map
.The text was updated successfully, but these errors were encountered: