new lint: format_collect
#11116
new lint: format_collect
#11116
Conversation
|
r? @Manishearth (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
The regex tests failing CI looks unrelated. Should hopefully be fixed when #11111 gets merged |
|
r? @Centri3 |
|
Failed to set assignee to
|
y21
force-pushed
the
format_collect
branch
2 times, most recently
from
cf86887 to
570dff6
Compare
| use rustc_lint::LateContext; | ||
| use rustc_span::Span; | ||
|
|
||
| fn tail_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { |
There was a problem hiding this comment.
Maybe I'm missing something, but can this not use clippy_utils::peel_blocks? Unless from_expansion matters here, which it may in the case of format! (and also that it returns None if block.expr is None)
There was a problem hiding this comment.
yep, the important bit is the check that it doesn't come from an expansion. If we always remove blocks, we would also remove parts of the format! expansion, which makes code further below not work (root_macro_call_first_node will return None, just tested it)
There was a problem hiding this comment.
I think we could check the syntax context (.ctxt()) of the span, then .outer_expn_data(), so we only ignore blocks if it's from format! but that's a major nit (There also may be a better way to do that, no idea)
Also, this should probably be recursive so something like {{ format!("{x:?}) }} works (which I believe doesn't?)
There was a problem hiding this comment.
The name is also a bit odd as well, no suggestions though. Perhaps documenting why it isn't using peel_blocks could be nice
There was a problem hiding this comment.
I think we could check the syntax context (
.ctxt()) of the span, then.outer_expn_data(), so we only ignore blocks if it's fromformat!but that's a major nit (There also may be a better way to do that, no idea)
I'm not sure. I think doing it like that will have FPs (unless I misunderstood) because outer_expn_data() returns the "outermost"(?) expansion, not the very first. In other words, it would trigger here:
macro_rules! x {
($b:expr) => {
format!("{:02X}", $b)
};
}
fn hex_encode(bytes: &[u8]) -> String {
bytes.iter().map(|b| x!(b)).collect()
}when it shouldn't, because format! comes from another macro expansion
Also, this should probably be recursive so something like
{{ format!("{x:?}) }}works (which I believe doesn't?)
Good point.
The name is also a bit odd as well, no suggestions though. Perhaps documenting why it isn't using
peel_blockscould be nice
Are you referring to the name of the function or the lint? I agree with both though 😅
There was a problem hiding this comment.
I'm not sure. I think doing it like that will have FPs (unless I misunderstood) because outer_expn_data() returns the "outermost"(?) expansion, not the very first. In other words, it would trigger here:
Is that so? That's unfortunate then
Are you referring to the name of the function or the lint? I agree with both though sweat_smile
Function, the lint's name is fine imo
There was a problem hiding this comment.
Oh I think this could use marks or remove_mark on Span::ctxt instead, but I highly doubt this is worth the effort ^^ This won't change the actual outcome afaik
| /// ``` | ||
| #[clippy::version = "1.72.0"] | ||
| pub FORMAT_COLLECT, | ||
| perf, |
There was a problem hiding this comment.
I'm not sure whether this should be perf. It's faster, but the example given doesn't seem to be worth the code smell (it should also probably use try_fold so errors from write! are propagated)
There was a problem hiding this comment.
write! never errors on Strings, so I'm not too sure on that. But yes, I agree that this might be too annoying for perf.
There was a problem hiding this comment.
Interesting, that should probably be documented in the lint description with a comment then as that's rather unexpected behavior (at least from the reader's perspective)
There was a problem hiding this comment.
I guess it's only really a useful suggestion if the iterator is gonna yield lots of small items, like bytes or chars (which seems hard to put into a lint, other than maybe restricting it to small types), otherwise it feels pedantic, like here: https://github.com/rust-lang/rust-clippy/pull/11116/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcL135
There was a problem hiding this comment.
write!never errors onStrings, so I'm not too sure on that. But yes, I agree that this might be too annoying forperf.
Display impls can return errors even if String does not (which is not advisable though)
|
Pretty sure this is done ^^ cc @Manishearth |
|
@bors r=centri3 |
new lint: `format_collect` A perf lint that looks for `format!`ing inside of `map`, then collecting it into a `String`. Did a quick benchmark locally and it's a bit more than 2x faster with fold. `write!` is still not optimal (presumably because the fmt stuff goes through dynamic dispatch), but it's still a lot better than creating a new string on every element. I thought about making a machine applicable suggestion, but there's a lot of suggestions that need to be made here, so I decided to just add help messages. changelog: new lint: `format_collect`
|
💔 Test failed - checks-action_dev_test |
|
Formatting, likely a |
|
@bors r+ |
|
oops, should have been r=Centri3, by habit |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
A perf lint that looks for
format!ing inside ofmap, then collecting it into aString. Did a quick benchmark locally and it's a bit more than 2x faster with fold.write!is still not optimal (presumably because the fmt stuff goes through dynamic dispatch), but it's still a lot better than creating a new string on every element.I thought about making a machine applicable suggestion, but there's a lot of suggestions that need to be made here, so I decided to just add help messages.
changelog: new lint:
format_collect