Upgrade "boolean-trivia" lint to new "argument-trivia" lint that uses type info, has quick fixes, etc.

[jakebailey]

This PR:

• Renames the lint, as we do more than just booleans.
• Also lints new expressions.
• When type information is available, checks that the provided name matches the parameter name for the given argument.
• Add quickfixes for adding a new comment, fixing an old comment, and fixing whitespace.

This is something that we probably didn't do before because it was too slow. However, this new lint appears to only take about 5-10% more time to run, likely because the expensive bits only execute if we have a literal argument that needs to be checked.

This also has the benefit of meaning that if we rename a parameter, we're forced to reexamine all of the places we previously used that parameter and agree that it's still okay.

[jakebailey]

Starting with a draft so we get the few hundred new errors in CI to look at.

[jakebailey]

This case is now "okay" because the lint takes the last comment as the hint, which I think is correct and is needed to handle cases where we put other comments before the parameter.

[fatcerberus]

When type information is available, checks that the provided name matches the parameter name for the given argument.

I'm confused - JS doesn't have named parameters? (I tried to check the diff first to see what was going on but it was huge). Unless you're destructuring in which case this would already be enforced by the type system.

[jakebailey]

No, it certainly doesn't. Positional only.

[fatcerberus]

Yes, that’s what I meant. There are no named parameters in JS so what is this lint checking for?

[jakebailey]

In this repo, we have a lint rule which for each argument that is undefined, false, true, or null, requires a comment be provided which says which parameter it is associated with. This is because if you pass a bunch of undefineds to a function, you have no clue what each of them are, as compared to say, a local variable, which will be named something like typeParameters or modifiers and therefore include more context for future reading/editing.

But, the current rule in main is simply "any comment", but this PR strengthens the check to make sure that the name provided actually matches the implementation.

In effect, this is basically like adding named parameters to the language, but only for certain kinds of literals.

I wanted to experiment with adding named positional arguments to the language, but, since that's not in ECMAScript (and I'm guessing will never be, as it's useless without a checker), we'd never actually add it so making this lint better is the best I can do.

[jakebailey]

Rechecking perf using hyperfine 'npx eslint .'

before:
  Time (mean ± σ):     23.488 s ±  0.277 s    [User: 37.317 s, System: 1.685 s]
  Range (min … max):   23.028 s … 24.057 s    10 runs

after:
  Time (mean ± σ):     25.204 s ±  0.153 s    [User: 40.119 s, System: 1.681 s]
  Range (min … max):   24.955 s … 25.481 s    10 runs

Which is about 7% slower overall.

[jakebailey]

@bradzacher @JamesHenry @JoshuaKGoldberg

Is this a kind of rule that you might be interested in taking into typescript-eslint? @Zamiell in Discord said this rule was neat and asked if they could use it, but this is internal to us at the moment, so I'm curious what your opinion is. (I truly don't know where most typescript-eslint consuming lint rules actually go.)

[Zamiell]

for more context, the idea behind the rule seems to be that you can refactor:

function myFunction(foo: number, bar: number) {}

to:

function myFunction(bar: number, foo: number) {}

normally, doing that would be disastrous, but with this lint rule, now you can find all the bugs!

[jakebailey]

To be clear, it wouldn't actually find that bug, because the rule as-written only cares about undefined, null, true, and false. But, that is of course arbitrary. It'd be a lot more expensive to check more and more of these. The only reason this PR is feasible is because it avoids asking for type info when there aren't any literals in that set.

TypeScript itself could do this cheaply, but, I don't think that's something we can really add to the language, especially if there's any potential of ECMAScript getting named parameters at any future point.

[JoshuaKGoldberg]

@jakebailey thanks for tagging - I suspect this isn't something we'd want to pull into typescript-eslint, as very few projects use this style. But that's not up to me to decide. Would you be open to filing an issue on us? We can mark it as evaluating community feedback, share it out, and see if there's a big swell of interest.

Worst case scenario, you and/or @Zamiell and/or I could make an external ESLint plugin for it. I'd certainly be interested in helping/maintaining that!

[bradzacher]

I'm in the same boat, personally - I don't think that it's something that really fits the general style in the community.
Anecdotally I see that most codebases will switch to an object parameter form if they want named (or unordered) params - though I get that TS doesn't do this because performance reasons.

[jakebailey]

All good, just testing the waters. I don't really know the licensing logistics of publishing this anyway.

[JoshuaKGoldberg]

Huh, do you not always lint with full type information? Is there context around here? Just curious 😄

[jakebailey]

Only src is set up for that. Reasonably, we could also add scripts to this list, but any root-level file like Herebyfile.mjs is not going to be a part of any project.

Now, if we get ts.ProjectService into ts-eslint... then everything could have type info, as TS would automagically figure out what project each file needs to be in, and allocate it to the implicit project where needed, without tsconfig being configured in eslintrc at all.

[jakebailey]

Also, I've actually been trying to drop type info, because it's just really expensive, but, we get too much value out of the singular type-using lint we have enabled, the one about unused type assertions.

That's a little funny from the POV that this new lint needs type info to be useful. So, oops.

[JoshuaKGoldberg]

Ooh we're adding in a little set of utilities for calling these checker APIs directly from services. getResolvedSignature doesn't exist right now, but if it did, we could simplify this to:

Suggested change

	const checker = parserServices.program.getTypeChecker();
	const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
	return checker.getResolvedSignature(tsNode);
	return parserServices.getResolvedSignature(node);

How does that make you feel? And if it's not negative, are there any other APIs you think would be good to make available?

Right now we just have getSymbolAtLocation and getTypeAtLocation wrapped.

• https://v6--typescript-eslint.netlify.app/custom-rules#typed-rules
• Make a "shortcut" API around TypeScript for node types typescript-eslint/typescript-eslint#6244
• feat(typescript-estree): add type checker wrapper APIs to ParserServicesWithTypeInformation typescript-eslint/typescript-eslint#6404

[jakebailey]

I personally don't mind the couple of lines to make this work, but, I'm also not writing rules all day.

[jakebailey]

Per design meeting, I think that we were alright with adding some extra lint time to get this. This should be ready for review!

[weswigham]

? What's this mean?

[jakebailey]

Ah, this is the lint for when you have /*param*/ true, with extra whitespace, and this lint rule apparently determines that style and deletes the extra spaces.

I wanted the error range to just be on the spaces but had trouble calculating it.