accept `String` in `span_lint*` functions directly to avoid unnecessary clones #12453

y21 · 2024-03-10T03:56:41Z

context: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Accepting.20.60Into.3C.7BSub.7DdiagMessage.3E.60.20in.20.60span_lint*.60.20functions/near/425703273

tldr: the span_lint* functions now accept both Strings, which are then reused and not recloned like before, and also &'static str, in which case it doesn't need to allocate.
Previously, it accepted &str and would always call .to_string(), which is worse in any case: it allocates for &'static str and forces a clone even if the caller already has a String.

This PR has a massive diff, but the only interesting change is in the first commit, which changes the message/help/note parameter in the span_lint* functions to not take a &str, but an impl Into<DiagMessage>.

The second commit changes all of the errors that now occur:

&format!(...) cannot be passed to span_lint anymore. Instead, we now simply pass format!() directly.
Into<DiagMessage> can be &'static str, but not any &str. So this requires changing a bunch of other &str to &'static str at call sites as well.
Added Sugg::into_string, which, as opposed to Sugg::to_string, can take advantage of the fact that it takes ownership and is able to reuse the String

changelog: none

rustbot · 2024-03-10T03:56:46Z

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

y21 · 2024-03-10T04:14:40Z

(To be fair, I don't think an unnecessary clone is going to make any noticeable difference on the span_lint path perf-wise, but this still seemed like a low hanging fruit and doesn't really add any complexity to lints. If anything, we now need one borrow less everywhere 🤷 )

bors · 2024-03-10T11:09:28Z

☔ The latest upstream changes (presumably #12445) made this pull request unmergeable. Please resolve the merge conflicts.

y21 · 2024-03-10T14:03:30Z

I'll wait with fixing conflicts since there will be many more.

About this:

rustc seems to intentionally ICE if a function is called with both an impl Into<SubdiagMessage> and impl Into<DiagMessage> parameter for some reason, so this currently works around it with a new trait.

It looks like the span_bug I ran into (here) was added in rust-lang/rust#121382. @nnethercote since you created that PR, do you think that it'd make sense to add a way to "allow" it in some way? I'm not too sure what exactly it's asserting, but it seems somewhat specific to rustc and translatable diagnostics, which clippy doesn't have anyway

nnethercote · 2024-03-10T21:33:20Z

FWIW, the compiler isn't ICEing (i.e. crashing due to a bug), rather a compiler-internal lint is giving an error.

There's no fundamental reason for the restriction on the number of impl Into<{D,Subd}iagMessage> parameters. It was just slightly easier to implement that way and it was good enough at the time. It will be easy to change impl_into_diagnostic_message_param from an Option to a Vec to handle multiple such arguments. I can do that later today.

y21 · 2024-03-10T21:45:04Z

I'm thinking that having our own trait for things that can be turned into diagnostic messages wouldn't be so bad of an idea anyway (like it currently does as a workaround), since it allows us to implement it for other clippy-specific types (which normally wouldn't work b/c of orphan rules).

For example, we could implement it for the Sugg helper type used for building up suggestions, then lints can pass it without having to turn it into a String at callsite first

y21 · 2024-03-10T21:47:40Z

Oh wait no, we should totally be able to impl From<DiagMessage> for Sugg. Not sure why I thought that'd go against the orphan rules

The internal diagnostic lint currently only allows one, because that was all that occurred in practice. But rust-lang/rust-clippy/pull/12453 wants to introduce functions with more than one, and this limitation is getting in the way.

nnethercote · 2024-03-10T23:13:26Z

FWIW, the compiler isn't ICEing (i.e. crashing due to a bug), rather a compiler-internal lint is giving an error.

My mistake, it is an ICE.

There's no fundamental reason for the restriction on the number of impl Into<{D,Subd}iagMessage> parameters. It was just slightly easier to implement that way and it was good enough at the time. It will be easy to change impl_into_diagnostic_message_param from an Option to a Vec to handle multiple such arguments. I can do that later today.

Done in rust-lang/rust/pull/122315.

y21 · 2024-03-10T23:15:22Z

Nice, thanks for getting to it so quickly!

The internal diagnostic lint currently only allows one, because that was all that occurred in practice. But rust-lang/rust-clippy/pull/12453 wants to introduce functions with more than one, and this limitation is getting in the way.

…sage, r=Nilstrieb Allow multiple `impl Into<{D,Subd}iagMessage>` parameters in a function. The internal diagnostic lint currently only allows one, because that was all that occurred in practice. But rust-lang/rust-clippy/pull/12453 wants to introduce functions with more than one, and this limitation is getting in the way. r? `@Nilstrieb`

Rollup merge of rust-lang#122315 - nnethercote:multiple-into-diag-message, r=Nilstrieb Allow multiple `impl Into<{D,Subd}iagMessage>` parameters in a function. The internal diagnostic lint currently only allows one, because that was all that occurred in practice. But rust-lang/rust-clippy/pull/12453 wants to introduce functions with more than one, and this limitation is getting in the way. r? `@Nilstrieb`

blyxyas · 2024-03-11T19:30:28Z

@y21 Meow meow, meow I think that the wisest course of action now would be to wait until the sync, and then undo the workaround (it isn't necessary anymore thanks to nnethercote's fix)

…ilstrieb Allow multiple `impl Into<{D,Subd}iagMessage>` parameters in a function. The internal diagnostic lint currently only allows one, because that was all that occurred in practice. But rust-lang/rust-clippy/pull/12453 wants to introduce functions with more than one, and this limitation is getting in the way. r? `@Nilstrieb`

Centri3 · 2024-03-19T19:45:39Z

fwiw, this might have a bigger impact than would first seem (haven't done a benchmark tho). to_string is called regardless of whether the lint is allowed or not, so for, e.g., allow_attributes_without_reason (which is very likely to show up 100s of times in a codebase, and is allow-by-default), this allocation would be done a lot of times.

It doesn't do anything else before checking the lint level, so is definitely pretty good for that case.

blyxyas · 2024-03-19T20:08:18Z

I'll do a benchmark soon, maybe this is even better than expected!
Edit: I'll do a benchmark when upstream is synched, I tried doing it myself but it's kinda complicated.

blyxyas · 2024-03-22T13:05:58Z

Okis, the sync has been done today =^w^=

y21 · 2024-03-23T06:00:21Z

Alright, I removed the workaround. The functions now accept the real impl Into<DiagMessage> as a parameter.
(The relevant changes are in the first commit again)

blyxyas · 2024-03-26T16:03:33Z

I'll benchmark this PR right now (from the server, while working on other issues)

y21 · 2024-03-26T16:57:31Z

It probably makes sense to run the benchmark on a crate/crates that emit a lot of lints. Maybe the unicode-normalization crate would be interesting. I remember that one from lintcheck runs because it emits about 260,000 diagnostics(!) for one match expression. Maybe it'd also be interesting to somehow benchmark the testsuite, since that also primarily consists of code that emits lints?

Even in such an extreme case with 250k diagnostics though, with a "guess" that an allocation takes maybe 20ns, that would still be 5 milliseconds, which I'm not sure if that's noticeable.
Regardless, it seems like a "free" low hanging fruit that doesn't really impact complexity of lints

Centri3 · 2024-03-26T23:50:23Z

I would be very skeptical of that guess since allocating on the heap is usually thousands of instructions, is it not? (and a disassembler does show this, even with the system allocator). There's a lot involved, even just in the kernel (excluding any tricks the allocator does - idk if clippy uses the default [system] global allocator, unlike rustc)

A single cycle should be ~0.4ns for me excluding any other factors, so e.g., fetching from RAM, caching instructions, failed branch prediction, etc...,

But regardless, this has absolutely been discussed many times before. A simple benchmark does take <20ns (sometimes 5ns with jemalloc) but I don't think that's applicable in practice. It wouldn't need to search for long to find free memory for instance, or maybe it takes a different branch in that case. Who knows, really

Centri3 · 2024-03-26T23:58:59Z

unicode-normalization might be a good pick. I didn't realize before that this PR also allocated for &str and thought it was only for passing &String

blyxyas · 2024-03-27T00:26:26Z

I've been trying to benchmark this for days. It's ridiculous how much a patch can resist to being benchmarked.
And I'll do it, I sweat that I'll benchmark this. Even if it takes... I don't know what more it would take ¯\_(ツ)_/¯

y21 · 2024-03-27T00:44:41Z

I would be very skeptical of that guess

Yea, fair points. I realize now that the number could be totally wrong so I take that part of my message back. I guess real benchmarks are the only source of truth we can trust :)
(I wish we had something like the perf bot on the upstream rust repo...)

blyxyas · 2024-03-29T11:51:44Z

Okis, the benchmarks are finally done. Yeah, it has some small improvements.
At some point this became something more related to honor than it was to performance 😅

blyxyas

LGTM, thanks! ❤️

blyxyas · 2024-03-11T19:27:01Z

clippy_utils/src/diagnostics.rs

+        )*
+    }
+}
+impl_into_message!(&'static str, String, std::borrow::Cow<'static, str>);


Suggested change

impl_into_message!(&'static str, String, std::borrow::Cow<'static, str>);

impl_into_message![

&'static str,

String,

std::borrow::Cow<'static, str>

];

blyxyas · 2024-03-30T19:02:55Z

@bors r+

bors · 2024-03-30T19:02:58Z

📌 Commit 9fc88bc has been approved by blyxyas

It is now in the queue for this repository.

accept `String` in `span_lint*` functions directly to avoid unnecessary clones context: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Accepting.20.60Into.3C.7BSub.7DdiagMessage.3E.60.20in.20.60span_lint*.60.20functions/near/425703273 tldr: the `span_lint*` functions now accept both `String`s, which are then reused and not recloned like before, and also `&'static str`, in which case it [doesn't need to allocate](https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_error_messages/lib.rs.html#359). Previously, it accepted `&str` and would always call `.to_string()`, which is worse in any case: it allocates for `&'static str` and forces a clone even if the caller already has a `String`. --------- This PR has a massive diff, but the only interesting change is in the first commit, which changes the message/help/note parameter in the `span_lint*` functions to not take a `&str`, but an `impl Into<DiagMessage>`. The second commit changes all of the errors that now occur: - `&format!(...)` cannot be passed to `span_lint` anymore. Instead, we now simply pass `format!()` directly. - `Into<DiagMessage>` can be `&'static str`, but not any `&str`. So this requires changing a bunch of other `&str` to `&'static str` at call sites as well. - Added [`Sugg::into_string`](https://github.com/y21/rust-clippy/blob/9fc88bc2851fbb287d89f65b78fb67af504f8362/clippy_utils/src/sugg.rs#L362), which, as opposed to `Sugg::to_string`, can take advantage of the fact that it takes ownership and is able to reuse the `String` changelog: none

bors · 2024-03-30T19:04:06Z

⌛ Testing commit 9fc88bc with merge b2d5eb2...

bors · 2024-03-30T19:05:19Z

💔 Test failed - checks-action_dev_test

y21 · 2024-03-30T20:38:58Z

Looks like the large_stack_frames lint was changed in the meantime to pass &format! and now had to be changed too.

blyxyas · 2024-04-01T08:25:36Z

@bors retry

blyxyas · 2024-04-01T08:43:06Z

??
@bors r+

bors · 2024-04-01T08:43:08Z

📌 Commit a6881b0 has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-04-01T08:44:15Z

⌛ Testing commit a6881b0 with merge f678d26...

accept `String` in `span_lint*` functions directly to avoid unnecessary clones context: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Accepting.20.60Into.3C.7BSub.7DdiagMessage.3E.60.20in.20.60span_lint*.60.20functions/near/425703273 tldr: the `span_lint*` functions now accept both `String`s, which are then reused and not recloned like before, and also `&'static str`, in which case it [doesn't need to allocate](https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_error_messages/lib.rs.html#359). Previously, it accepted `&str` and would always call `.to_string()`, which is worse in any case: it allocates for `&'static str` and forces a clone even if the caller already has a `String`. --------- This PR has a massive diff, but the only interesting change is in the first commit, which changes the message/help/note parameter in the `span_lint*` functions to not take a `&str`, but an `impl Into<DiagMessage>`. The second commit changes all of the errors that now occur: - `&format!(...)` cannot be passed to `span_lint` anymore. Instead, we now simply pass `format!()` directly. - `Into<DiagMessage>` can be `&'static str`, but not any `&str`. So this requires changing a bunch of other `&str` to `&'static str` at call sites as well. - Added [`Sugg::into_string`](https://github.com/y21/rust-clippy/blob/9fc88bc2851fbb287d89f65b78fb67af504f8362/clippy_utils/src/sugg.rs#L362), which, as opposed to `Sugg::to_string`, can take advantage of the fact that it takes ownership and is able to reuse the `String` changelog: none

bors · 2024-04-01T08:45:35Z

💔 Test failed - checks-action_dev_test

blyxyas · 2024-04-01T08:49:55Z

Oh I thought you fixed it in the latest push 😅
DM me on Zulip when you have it :3

y21 · 2024-04-01T11:56:50Z

oh well, another lint was changed to use &format!() in the meantime. I did fix the other one. But yeah, I can fix it locally and we can coordinate on zulip when I should push so this doesnt keep happening

blyxyas · 2024-04-01T14:09:33Z

@bors r+

bors · 2024-04-01T14:09:35Z

📌 Commit 91f514c has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-04-01T14:10:42Z

⌛ Testing commit 91f514c with merge c2681f2...

bors · 2024-04-01T14:20:38Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing c2681f2 to master...

rustbot assigned blyxyas Mar 10, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 10, 2024

nnethercote mentioned this pull request Mar 10, 2024

Allow multiple impl Into<{D,Subd}iagMessage> parameters in a function. rust-lang/rust#122315

Merged

y21 force-pushed the span_lint_into_diag branch 3 times, most recently from 5bebbf3 to 9fc88bc Compare March 23, 2024 05:57

blyxyas approved these changes Mar 30, 2024

View reviewed changes

y21 force-pushed the span_lint_into_diag branch from 9fc88bc to a6881b0 Compare March 30, 2024 20:36

y21 added 2 commits April 1, 2024 14:50

accept Into<{Sub}DiagMessage> in span_lint functions

bd4d456

fix fallout from previous commit

91f514c

y21 force-pushed the span_lint_into_diag branch from a6881b0 to 91f514c Compare April 1, 2024 13:05

bors merged commit c2681f2 into rust-lang:master Apr 1, 2024
5 checks passed

This was referenced Apr 1, 2024

Fix std_instead_of_core for ListStem use #12474

Closed

Check for Default trait implementation in initial condition when linting [manual_unwrap_or_default] #12566

Closed

accept String in span_lint* functions directly to avoid unnecessary clones #12453

accept String in span_lint* functions directly to avoid unnecessary clones #12453

Conversation

y21 commented Mar 10, 2024 • edited Loading

rustbot commented Mar 10, 2024

y21 commented Mar 10, 2024 • edited Loading

bors commented Mar 10, 2024

y21 commented Mar 10, 2024

nnethercote commented Mar 10, 2024

y21 commented Mar 10, 2024

y21 commented Mar 10, 2024

nnethercote commented Mar 10, 2024

y21 commented Mar 10, 2024

blyxyas commented Mar 11, 2024

Centri3 commented Mar 19, 2024

blyxyas commented Mar 19, 2024 • edited Loading

blyxyas commented Mar 22, 2024

y21 commented Mar 23, 2024

blyxyas commented Mar 26, 2024

y21 commented Mar 26, 2024 • edited Loading

Centri3 commented Mar 26, 2024 • edited Loading

Centri3 commented Mar 26, 2024

blyxyas commented Mar 27, 2024

y21 commented Mar 27, 2024 • edited Loading

blyxyas commented Mar 29, 2024

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas Mar 11, 2024

Choose a reason for hiding this comment

blyxyas commented Mar 30, 2024

bors commented Mar 30, 2024

bors commented Mar 30, 2024

bors commented Mar 30, 2024

y21 commented Mar 30, 2024

blyxyas commented Apr 1, 2024

blyxyas commented Apr 1, 2024

bors commented Apr 1, 2024

bors commented Apr 1, 2024

bors commented Apr 1, 2024

blyxyas commented Apr 1, 2024

y21 commented Apr 1, 2024

blyxyas commented Apr 1, 2024

bors commented Apr 1, 2024

bors commented Apr 1, 2024

bors commented Apr 1, 2024

accept `String` in `span_lint*` functions directly to avoid unnecessary clones #12453

accept `String` in `span_lint*` functions directly to avoid unnecessary clones #12453

y21 commented Mar 10, 2024 •

edited

Loading

y21 commented Mar 10, 2024 •

edited

Loading

blyxyas commented Mar 19, 2024 •

edited

Loading

y21 commented Mar 26, 2024 •

edited

Loading

Centri3 commented Mar 26, 2024 •

edited

Loading

y21 commented Mar 27, 2024 •

edited

Loading