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

New lint suggestion: Suspicious {} in string literal #11486

Closed
emilk opened this issue Sep 12, 2023 · 3 comments
Closed

New lint suggestion: Suspicious {} in string literal #11486

emilk opened this issue Sep 12, 2023 · 3 comments
Labels
A-lint Area: New lints

Comments

@emilk
Copy link

emilk commented Sep 12, 2023

What it does

Look for curly braces in string literals and warn that the user may be missing a format! call.

Advantage

I have several times forgotten to call format!, leading to subtle bugs. For instance:

on_error("There was an error: {error}");

Drawbacks

There may be legitimate uses of curly-braces in strings, so there will likely be a lot of false positives. This should therefor be a suspicious lint, that is opt-in.

Example

on_error("There was an error: {error}");

Could be written as:

on_error(format!("There was an error: {error}"));
@emilk emilk added the A-lint Area: New lints label Sep 12, 2023
@y21
Copy link
Member

y21 commented Sep 12, 2023

cc #10195
It seems like it'd have FPs for JSON-like strings, but maybe we could try to parse the literal as a format string to see if it is one instead of only looking for just { .. } (not sure if that ends up being too expensive) since most JSON is most likely not valid format syntax.

This should therefor be a suspicious lint, that is opt-in.

I'm not sure if thats possible. Suspicious is warn by default and afaik needs to be opt out of manually by the user. Pedantic and restriction are allow by default (opt in)

@emilk
Copy link
Author

emilk commented Sep 12, 2023

You are right, this is a duplicate of #10195 - I missed it when searching 🤦

Parsing the {contents} would indeed be nice, or at least have a smart regex that checks the content is something like \w*(:.*)?

And yeah, I think I am confused about what suspicious means then :) pedantic/restriction is probably correct

@emilk
Copy link
Author

emilk commented Sep 12, 2023

Closing as duplicate of #10195

@emilk emilk closed this as not planned Won't fix, can't repro, duplicate, stale Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

2 participants