add suggestions to rc_clone_in_vec_init #8814

yonip23 · 2022-05-10T22:13:22Z

A followup to #8769
I also switch the order of the 2 suggestions, since the loop initialization one is probably the common case.

@xFrednet I'm not letting you guys rest for a minute 😅
changelog: add suggestions to [rc_clone_in_vec_init]

rust-highfive · 2022-05-10T22:13:25Z

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

yonip23 · 2022-05-10T22:14:42Z

clippy_lints/src/rc_clone_in_vec_init.rs

    }
 }

-fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, macro_call: &MacroCall) {
+struct LintSuggestion {


I'm wondering whether this can be extracted to a util somewhere

I think in most cases it's better to use span_lint_and_then and pass the DiagnosticBuilder around. That's also a bit more efficient when it comes to performance. The DiagnosticBuilder will store the suggestions internally. Is there a reason why you want to use this intermediate struct to store the suggestions? 🙃

Just so I can construct the suggestions separately from where I'm emitting the lint, so that the span_lint_and_then code will be clean from logistics.

In terms of performance, since I'm passing a & mut Diagnostic the performance overhead is basically only the Vec<LintSuggestion> creation - which I thought is a small price to pay for readability.

Lmk if there's some other design you'd prefer 🙂

The idea in rustc is actually to create the suggestion inside the closure. Clippy does it a bit differently to have a simpler interface, but if we use span_lint_and_then we usually try to also create the suggestion inside. Extracting the specific string creation to single functions like this is nice, but I wouldn't store them in another struct. Also, since the Span and Applicability is always the same.

In terms of performance

This comment was more about the internal implementation in rustc. If lints are allowed, they won't be emitted, rustc then never calls the closure. That's what I was referring to. That is super nit picky though, it only came to mind since I've worked on that recently. I sometimes go a bit overboard with my reviews, this side node is an example for this 😅. So please ignore the performance comment 🙃

I'm willing to go as deep into this rabbit hole as you are 🙂
np, I'll change the design a bit so that everything is being passed to emit_lint and it'll construct the suggestion by calling another function inside the clousure

Question is, how important are performance here? Is vec allocation going to cost too much here?

Not really, this is more about the style, I don't really see the reason for having the struct just to pass them to the DiagnosticBuilder afterwards. What about passing in the DiagnosticBuilder into the function as an argument?

that's basically what I offered, just don't know what is DiagnosticBuilder exactly 🙈
the closure receives a &mut Diagnostic.
I'm offering to pass it into construct_lint_suggestions such that this function will span the suggestions instead of returning them as a vec.

I don't really see the reason for having the struct just to pass them to the DiagnosticBuilder afterwards.

Imo passing named data between scopes is arguably justifying a struct.
None the less, I'll change it as you wish of course 🙂

I ended up basically taking your suggestion from here: #8814 (comment)

Imo passing named data between scopes is arguably justifying a struct.

That's totally correct. In this case, I just felt a bit weird, since I would say that the DiagnosticBuilder is exactly for this. Sorry for not explaining what the builder is, it's just the argument received from the span_lint_and_then argument. This implementation looks good to me on a quick glance, I'll try to do a full review soon 🙃

xFrednet · 2022-05-10T22:17:02Z

I'm not letting you guys rest for a minute

There ain't no rest for the wicked 🎶 🎶 😂

Thank you for the ping. I'm happy to see the follow-up PR. I'll put it on my todo list. FYI, you can also request a specific reviewer with 🙃

r? @xFrednet

xFrednet

The lint suggestions looks fantastic, I left some comments regarding the implementation 🙃

clippy_lints/src/rc_clone_in_vec_init.rs

xFrednet · 2022-05-11T18:44:42Z

clippy_lints/src/rc_clone_in_vec_init.rs

+        let reference_initialization_end =
+            elem_snippet.find(&reference_initialization).unwrap() + reference_initialization.len();
+        elem_snippet.replace_range(reference_initialization_end.., "..");


Nice explanatory comment 🙃

Why do you use replace_range? Wouldn't it be simpler to reassign elem_snippet like this?

if elem_snippet.contains('\n') { elem_snippet = format!("{symbol_name}::new(..)"); }

lol I played around with the snippet trimming, tried some variations until i came up with this monstrosity 🤣

ah, actually now that im looking at it, if you had std::rc::Rc::new... in the original code, the suggestion should also start the same way, and not just Rc::new(..)

I'll change the code to be more straight forward though

xFrednet · 2022-05-11T18:47:52Z

clippy_lints/src/rc_clone_in_vec_init.rs

    }
 }

-fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, macro_call: &MacroCall) {
+struct LintSuggestion {


The idea in rustc is actually to create the suggestion inside the closure. Clippy does it a bit differently to have a simpler interface, but if we use span_lint_and_then we usually try to also create the suggestion inside. Extracting the specific string creation to single functions like this is nice, but I wouldn't store them in another struct. Also, since the Span and Applicability is always the same.

In terms of performance

This comment was more about the internal implementation in rustc. If lints are allowed, they won't be emitted, rustc then never calls the closure. That's what I was referring to. That is super nit picky though, it only came to mind since I've worked on that recently. I sometimes go a bit overboard with my reviews, this side node is an example for this 😅. So please ignore the performance comment 🙃

yonip23 · 2022-05-11T20:17:44Z

I think the only comment I didn't fix was the {indent} one - lmk wdyt

xFrednet · 2022-05-17T05:45:55Z

Looks good to me, thank you for the update and diagnostic improvement. I like the new diagnostic message :)

@bors r+

bors · 2022-05-17T05:45:56Z

📌 Commit ed3744b has been approved by xFrednet

bors · 2022-05-17T05:46:02Z

⌛ Testing commit ed3744b with merge d901079...

bors · 2022-05-17T06:01:00Z

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

add suggestions to rc_clone_in_vec_init

f3e01c4

rust-highfive assigned Manishearth May 10, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 10, 2022

yonip23 commented May 10, 2022

View reviewed changes

rust-highfive assigned xFrednet and unassigned Manishearth May 10, 2022

xFrednet reviewed May 11, 2022

View reviewed changes

yonip23 added 2 commits May 11, 2022 23:11

fix review comments

344888a

fix clippy warning

a791205

yonip23 added 2 commits May 12, 2022 10:06

fix indentation + test

dc23b5d

inline construct_lint_suggestions

ed3744b

xFrednet approved these changes May 17, 2022

View reviewed changes

bors merged commit d901079 into rust-lang:master May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add suggestions to rc_clone_in_vec_init #8814

add suggestions to rc_clone_in_vec_init #8814

yonip23 commented May 10, 2022

rust-highfive commented May 10, 2022

yonip23 May 10, 2022 •

edited

Loading

xFrednet May 10, 2022

yonip23 May 10, 2022

xFrednet May 11, 2022

yonip23 May 11, 2022

yonip23 May 12, 2022

xFrednet May 12, 2022

yonip23 May 12, 2022

yonip23 May 12, 2022

xFrednet May 15, 2022

xFrednet commented May 10, 2022 •

edited

Loading

xFrednet left a comment •

edited

Loading

xFrednet May 11, 2022

yonip23 May 11, 2022

yonip23 May 11, 2022 •

edited

Loading

xFrednet May 11, 2022

yonip23 commented May 11, 2022

xFrednet commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

add suggestions to rc_clone_in_vec_init #8814

add suggestions to rc_clone_in_vec_init #8814

Conversation

yonip23 commented May 10, 2022

rust-highfive commented May 10, 2022

yonip23 May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xFrednet commented May 10, 2022 • edited Loading

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonip23 May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonip23 commented May 11, 2022

xFrednet commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

yonip23 May 10, 2022 •

edited

Loading

xFrednet commented May 10, 2022 •

edited

Loading

xFrednet left a comment •

edited

Loading

yonip23 May 11, 2022 •

edited

Loading