new lint: `recursive_format_impl` #8188

jamesmcm · 2021-12-28T18:39:25Z

The to_string_in_display lint is renamed to recursive_format_impl
A check is added for the use of self formatted with Display or Debug inside any format string in the same impl
The to_string_in_display check is kept as is - like in the format_in_format_args lint

This is my first contribution so please check it for better / more idiomatic checks + error messages. Note the format macro paths are shared with the format_in_format_args lint - maybe those could be moved to clippy utils too.

This relates to issues #2691 and #7830

changelog: Renamed to_string_in_display lint to [recursive_format_impl] with new check for any use of self as Display or Debug inside the same format trait impl.

rust-highfive · 2021-12-28T18:39:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @giraffate (or someone else) soon.

Please see the contribution instructions for more information.

ghost · 2021-12-29T07:03:49Z

It seems like the lint doesn't handle nested structures correctly.

Here is a false positive:

#![allow(dead_code)]
#![warn(clippy::all)]

use std::fmt;

enum Tree {
    Leaf,
    Node(Vec<Tree>),
}

impl fmt::Display for Tree {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            Tree::Leaf => write!(f, "*"),
            Tree::Node(children) => {
                write!(f, "(")?;
                for child in children.iter() {
                    write!(f, "{},", child)?;
                }
                write!(f, ")")
            }
        }
    }
}

fn main() {}

warning: using `self` in `fmt::Display` implementation might lead to infinite recursion
  --> src/main.rs:18:21
   |
18 |                     write!(f, "{},", child)?;
   |                     ^^^^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/main.rs:2:9
   |
2  | #![warn(clippy::all)]
   |         ^^^^^^^^^^^
   = note: `#[warn(clippy::recursive_display_impl)]` implied by `#[warn(clippy::all)]`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#recursive_display_impl
   = note: this warning originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info)

jamesmcm · 2021-12-29T09:02:23Z

Thanks, the issue is when the nested type is actually the same as the Self type. It seems the best thing to do would be to limit the lint to check whether it is actually the same self reference (not just the types).

I'll add it as a test case and try to work around it, thanks!

jamesmcm · 2021-12-29T10:16:02Z

Fixed by dealing with the single deref of &self to self inside fmt() as a special case, instead of just checking the types match (that caused the false positive above).

camsteffen · 2022-01-11T16:03:57Z

I realized with #8253 that this lint should work for any formatting trait (e.g. Debug). The two lints can share a LintPass and a naming convention. For the common part of the name I'm thinking either "fmt" or "format_impl".

jamesmcm · 2022-01-13T19:05:49Z

It's a good idea @camsteffen but note the implementations would differ a fair bit since atm this uses:
https://github.com/rust-lang/rust-clippy/pull/8188/files#diff-37e08397df0148cab790ac270ccda86159451eb034ef01a7b59bc46b00074ca4R168-R174

                    if !arg.is_display() {
                        continue;
                    }

to filter the format args, unfortunately there isn't an equivalent for debug atm. See the source code here - https://doc.rust-lang.org/nightly/nightly-rustc/src/clippy_utils/higher.rs.html#680-688

Also note that it's okay to use Debug in the Display Impl and vice versa.

I'll try to look at adding the above for Debug when I get some time free.

camsteffen · 2022-01-13T21:13:06Z

It would be perfectly fine to leave other traits as a future enhancement. I was just thinking about the lint name since we like to get that right the first time.

jamesmcm · 2022-01-14T14:55:30Z

@camsteffen renamed to recursive_format_trait_impl and rebased on upstream (there were some changes to the FormatArg macro parsing).

atm I only check for Debug and Display, but one could add the others (Binary, etc.) pretty easily. It also doesn't check the case where there's a reference cycle i.e. Debug calls Display, and Display calls Debug - we could detect this if we store more information about the other format trait impls as we do the pass, but it doesn't seem like something that is likely to come up in real code anyway.

camsteffen · 2022-01-16T13:57:45Z

I think recursive_format_impl would be clear enough and shorter?

giraffate · 2022-01-18T23:50:27Z

@camsteffen You've already reviewed some of this, so can you review it as is?

camsteffen · 2022-01-19T14:30:37Z

Okay.

r? @camsteffen

camsteffen

This is looking good! Nice tests. I have some comments but it's pretty minor stuff.

It would be nice to have a separate commit to rename the lint. Also we need to add a register_renamed call and add to the rename.rs test.

camsteffen · 2022-01-22T21:40:55Z

clippy_lints/src/recursive_format_impl.rs

+    &paths::PRINTLN_MACRO,
+    &paths::WRITE_MACRO,
+    &paths::WRITELN_MACRO,
+];


We should avoid duplicating these lists. Instead we can have a util like is_format_macro(DefId) -> bool.

But I'm also thinking it might be a good idea to just use the FormatArgs LintPass. Up to you.

Done, but I'll leave making the changes to the FormatArgs lint to another PR so as not to make this huge.

How could we re-use the lint pass, when here we need to check we are inside the impl Display or Debug, etc. ?

Done, but I'll leave making the changes to the FormatArgs lint to another PR so as not to make this huge.

I still would like to avoid duplicating the list in this PR. I small refactor can be done in the other lint.

How could we re-use the lint pass, when here we need to check we are inside the impl Display or Debug, etc. ?

It seems like it should be possible to merge the two lint passes and maintain all cases. But I may be underestimating the difficulty. Again I'll leave it up to you to decide if that ultimately makes sense to do.

@jamesmcm did you decide against merging the lint passes or still looking into it?

I took a look at it, in theory it should be possible, it's not trivial though since the format_args case also needs to work when the macro isn't nested. It might be best to do in another PR though.

Btw is there any way to share the pass without combining the source files (and error files)?

If we combine them, I think it would be better to do it in this PR since it would be less churn and reviewing. The tests can still be separated per lint.

Cool, looking at it in detail I don't think it is worth combining them. Since handling the nested macro difference means the code isn't really shared between the non format trait impl and format trait impl cases, so it's more just moving it around and I don't think that's worth merging them (since at that point you might as well merge #8253 too for example).

@camsteffen considering this, I think this is mergeable as is?

Sorry for the delay.

(since at that point you might as well merge #8253 too for example).

I am planning to have that lint merged with this one at least. But if that doesn't change your decision, yes this is ready to merge!

clippy_lints/src/recursive_format_impl.rs

clippy_utils/src/format_macros.rs

bors · 2022-01-27T14:05:52Z

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

bors · 2022-02-06T16:24:27Z

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

bors · 2022-02-12T07:43:49Z

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

bors · 2022-02-14T07:20:04Z

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

The to_string_in_display lint is renamed to recursive_format_impl A check is added for the use of self formatted with Display or Debug inside any format string in the same impl The to_string_in_display check is kept as is - like in the format_in_format_args lint For now only Display and Debug are checked This could also be extended to other Format traits (Binary, etc.)

camsteffen · 2022-02-16T22:01:44Z

Thanks for your patience, and great first contribution!

@bors r+

bors · 2022-02-16T22:01:46Z

📌 Commit b162b11 has been approved by camsteffen

bors · 2022-02-16T22:01:52Z

⌛ Testing commit b162b11 with merge b34cd79...

bors · 2022-02-16T22:19:48Z

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

rust-highfive assigned giraffate Dec 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 28, 2021

jamesmcm force-pushed the recursive_display_impl branch 3 times, most recently from 147cb29 to a0ce3b0 Compare December 28, 2021 19:04

jamesmcm force-pushed the recursive_display_impl branch from a0ce3b0 to e88ec71 Compare December 29, 2021 10:06

jamesmcm force-pushed the recursive_display_impl branch 2 times, most recently from da2cd64 to 6e97fc9 Compare January 14, 2022 14:16

jamesmcm changed the title ~~new lint: recursive_display_impl~~ new lint: recursive_format_trait_impl Jan 14, 2022

jamesmcm force-pushed the recursive_display_impl branch from 6e97fc9 to 2f48fcc Compare January 14, 2022 14:38

jamesmcm force-pushed the recursive_display_impl branch from 2f48fcc to f201863 Compare January 17, 2022 18:02

camsteffen mentioned this pull request Jan 17, 2022

Add print_in_format_impl lint #8253

Merged

jamesmcm changed the title ~~new lint: recursive_format_trait_impl~~ new lint: recursive_format_impl Jan 17, 2022

camsteffen assigned camsteffen and unassigned giraffate Jan 19, 2022

camsteffen suggested changes Jan 22, 2022

View reviewed changes

jamesmcm force-pushed the recursive_display_impl branch 2 times, most recently from 7de473e to cfb32b3 Compare January 23, 2022 20:40

camsteffen reviewed Jan 24, 2022

View reviewed changes

clippy_lints/src/recursive_format_impl.rs Outdated Show resolved Hide resolved

camsteffen reviewed Jan 24, 2022

View reviewed changes

clippy_utils/src/format_macros.rs Outdated Show resolved Hide resolved

jamesmcm force-pushed the recursive_display_impl branch from cfb32b3 to 0c615e6 Compare January 25, 2022 19:12

jamesmcm force-pushed the recursive_display_impl branch from 0c615e6 to 3bbd06d Compare January 27, 2022 17:16

jamesmcm force-pushed the recursive_display_impl branch from 3bbd06d to d058516 Compare February 9, 2022 18:14

jamesmcm force-pushed the recursive_display_impl branch from d058516 to 514b0fc Compare February 12, 2022 09:21

jamesmcm force-pushed the recursive_display_impl branch from 514b0fc to b162b11 Compare February 14, 2022 17:50

bors merged commit b34cd79 into rust-lang:master Feb 16, 2022

This was referenced Feb 16, 2022

Transmute_undefined_repr to nursery again #8432

Merged

Nightly lints for Clippy #8435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new lint: `recursive_format_impl` #8188

new lint: `recursive_format_impl` #8188

jamesmcm commented Dec 28, 2021 •

edited

Loading

rust-highfive commented Dec 28, 2021

ghost commented Dec 29, 2021 •

edited by ghost

Loading

jamesmcm commented Dec 29, 2021 •

edited

Loading

jamesmcm commented Dec 29, 2021

camsteffen commented Jan 11, 2022 •

edited

Loading

jamesmcm commented Jan 13, 2022

camsteffen commented Jan 13, 2022

jamesmcm commented Jan 14, 2022

camsteffen commented Jan 16, 2022

giraffate commented Jan 18, 2022

camsteffen commented Jan 19, 2022

camsteffen left a comment •

edited

Loading

camsteffen Jan 22, 2022

jamesmcm Jan 23, 2022

camsteffen Jan 24, 2022

camsteffen Jan 28, 2022

jamesmcm Jan 28, 2022

camsteffen Jan 28, 2022

jamesmcm Jan 29, 2022

jamesmcm Feb 4, 2022

camsteffen Feb 4, 2022

bors commented Jan 27, 2022

bors commented Feb 6, 2022

bors commented Feb 12, 2022

bors commented Feb 14, 2022

camsteffen commented Feb 16, 2022

bors commented Feb 16, 2022

bors commented Feb 16, 2022

bors commented Feb 16, 2022

new lint: recursive_format_impl #8188

new lint: recursive_format_impl #8188

Conversation

jamesmcm commented Dec 28, 2021 • edited Loading

rust-highfive commented Dec 28, 2021

ghost commented Dec 29, 2021 • edited by ghost Loading

jamesmcm commented Dec 29, 2021 • edited Loading

jamesmcm commented Dec 29, 2021

camsteffen commented Jan 11, 2022 • edited Loading

jamesmcm commented Jan 13, 2022

camsteffen commented Jan 13, 2022

jamesmcm commented Jan 14, 2022

camsteffen commented Jan 16, 2022

giraffate commented Jan 18, 2022

camsteffen commented Jan 19, 2022

camsteffen 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

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

bors commented Jan 27, 2022

bors commented Feb 6, 2022

bors commented Feb 12, 2022

bors commented Feb 14, 2022

camsteffen commented Feb 16, 2022

bors commented Feb 16, 2022

bors commented Feb 16, 2022

bors commented Feb 16, 2022

new lint: `recursive_format_impl` #8188

new lint: `recursive_format_impl` #8188

jamesmcm commented Dec 28, 2021 •

edited

Loading

ghost commented Dec 29, 2021 •

edited by ghost

Loading

jamesmcm commented Dec 29, 2021 •

edited

Loading

camsteffen commented Jan 11, 2022 •

edited

Loading

camsteffen left a comment •

edited

Loading