8000 Enhancement: [restrict-plus-operands] Bring in options from restrict-template-expressions · Issue #6110 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Enhancement: [restrict-plus-operands] Bring in options from restrict-template-expressions #6110

8000
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.

Sign up for GitHub

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

Jump to bottom
Closed
4 tasks done
JoshuaKGoldberg opened this issue Nov 27, 2022 · 7 comments · Fixed by #6161
Closed
4 tasks done

Enhancement: [restrict-plus-operands] Bring in options from restrict-template-expressions #6110

JoshuaKGoldberg opened this issue Nov 27, 2022 · 7 comments · Fixed by #6161
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look

Comments

@JoshuaKGoldberg
Copy link
Member
JoshuaKGoldberg commented Nov 27, 2022 •
edited
Loading

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/restrict-plus-operands

Description

restrict-plus-operands and restrict-template-expressions don't tackle the exact same JavaScript behaviors (+ operands and template expressions don't concatenate/toString()/toValue() quite the same). But they both generally tackle the specific common case of code trying to join a string with a non-string:

'' + 0;
'' + {};
// _roughly_ equivalent to:
`${0}`;
`${{}}`;

As a user, I've been inconvenienced by restrict-plus-operands complaining on concatenating two primitive values: e.g. number and string. Sometimes I want to be able to join primitives together in my logs!

Proposal: can we add the following options that already exist in restrict-template-expressions?

Fail

'' + {}

Pass

'' + 0

Additional Info

I think we've seen a bigger demand for them in restrict-template-expressions because of https://eslint.org/docs/latest/rules/prefer-template. Linted projects don't concatenate strings with + as much as they used to.
@JoshuaKGoldberg JoshuaKGoldberg added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look enhancement: plugin rule option New rule option for an existing eslint-plugin rule labels Nov 27, 2022
@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Nov 27, 2022
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Dec 3, 2022
Merged
3 tasks
@JoshuaKGoldberg
Copy link
Member Author

@bradzacher bringing in #6161 (comment) on allowNullish:

The nullish case will create TS errors for numbers, and is only valid in strings. In the string case it also means you'll be concatenating with 'null' | 'undefined' - which doesn't seem desirable in the vast majority of code.

I don't think that this option really makes sense in the context of the rule.

...and #6161 (comment) on allowBoolean:

Similar to the nullish case the boolean case will create TS errors for numbers, and is only valid when adding with strings. In the string case it also means you'll be concatenating with 'true' | 'false' - which doesn't seem desirable in the vast majority of code.

I don't think that this option really makes sense in the context of the rule.

I've anecdotally seen developers intentionally do both those string concatenations, such as for console logs when debugging. If we want to enable this in the recommended preset I think we'd need to make it so users aren't getting warnings from their working-as-intended code. We can turn these options off in strict. WDYT?

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for team members to take a look and removed accepting prs Go ahead, send a pull request that resolves this issue labels Mar 11, 2023
@bradzacher
Copy link
Member

I've anecdotally seen developers intentionally do both those string concatenations, such as for console logs when debugging

IMO lint rules exist to ensure your code is ready for production.
Which means that if you do console.log('debug value' + boolean) - that code should definitely cause a lint error!

If the default/recommended config ignored that then you could very easily do like alert('You selected' + boolean) in production code - which would be bad and then lead the user to have to manually reconfigure the lint rule to turn off that check.

If we want to enable this in the recommended preset

restrict-plus-operands is currently a recommended rule and I haven't heard of people being upset that it doesn't allow booleans at all.
Have you had reports of people getting annoyed by it?

@JoshuaKGoldberg
Copy link
Member Author

Have you had reports of people getting annoyed by it?

Yes, I've been annoyed by it. It's happened a few times that I've wanted to print out something that includes, say, a boolean or regular expression in a message in a CLI and been bit by this rule. But I can't remember where those are now.

You know, in my mind I had real world evidence from other users supporting this issue. But I can't find it anywhere. I suppose it's fine if we just close it until a real user requests the need. Thanks for the discussion! 😄

@JoshuaKGoldberg
Copy link
Member Author
JoshuaKGoldberg commented Mar 14, 2023 •
edited
Loading

Aha @bradzacher I just found some examples. I tried enabling on the TypeScript repo and found about 200 lint complaints. A few examples of common code styles there are ...

  • Verbose logging: log("getCompletionData: Get previous token: " + (timestamp() - start)); [completions.ts]
  • Key generation: "" + (nameType as StringLiteralType | NumberLiteralType).value [checker.ts]
  • assert.equal(lac.line + 1, lp.line, "Line number mismatch " + (lac.line + 1) + " " + lp.line + " " + i); [unittests ... versionCache.ts]

The unfortunate predicament many repos are in, I think, is that they intentionally want to violate this rule if and only if the types are known to safely create developer-readable strings. Without the more permissive opt-in options, their choices are to either disable the rule, use very many inline comments, or modify the code to be more verbose.

@bradzacher
Copy link
Member

I wonder if there is a better way to do this sort of thing instead of using concatenation at the callsite?

For example instead of APIs like log(arg: string) and manually concatenating things together to a single argument, perhaps instead more like console log like log(...args: (string | number | boolean)[])

log('Foo bar' + baz + ' ' + bam);

log('Foo bar', baz, bam);

Eg an implementation like

function log(...args: (string | number | boolean)[]) {
  debugger(args.join(' '));
}

There's opportunities to improve the codebase and have more safety in general instead of doing a shotgun, super unsafe option like allowStringAndNumber.

I definitely understand why you would want to turn on such an option - because it's easier to ignore the cases instead of changing the codebase - but IMO "fix the API and disable the existing cases to be fixed later" is a better approach.

IDK. Options live forever, but migrations are temporary. I'm always very averse to adding options instead of guiding teams to just do better.

@JoshuaKGoldberg
Copy link
Member Author

better way to do this sort of thing instead of using concatenation at the callsite

Sure - I think we're in agreement that the concatenation pattern isn't great. I like the log function you're writing. Unfortunately I think we're not going to be able to win the battle of convincing people to use it, and our choice is between giving them an option with angry docs or them not using the rule at all.

@bradzacher
Copy link
Member

I'm unhappy with, but won't block the addition of an option - but I think that we need to clearly document the caveat that it severely weakens the rule to the point that all it bans is string + nullish.

I think we should document alternatives and better patterns to use to help guide users towards better code because better code means you can have a stricter rule and a better codebase.

This was referenced Mar 24, 2023
Open
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(eslint-plugin): [restrict-plus-operands] add allow* options JoshuaKGoldberg/typescript-eslint
2 participants
@JoshuaKGoldberg @bradzacher
0