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

Add suggestion for captured and passed arg case in fmt macro #105635

Closed
wants to merge 7 commits into from
Closed

Add suggestion for captured and passed arg case in fmt macro #105635

wants to merge 7 commits into from

Conversation

atamakahere-git
Copy link

Fixes #105225

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2022

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

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 12, 2022
let current_arg = symbol.as_str();
let current_arg_ph = "{".to_owned() + current_arg + "}";
if current_arg.len() > 0 && fmt_str.contains(current_arg_ph.as_str()) {
err = "argument never used, consider removing it"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the argument is also a duplicate of an existing argument, this should probably mention that the argument is a duplicate of an inline argument, which is the reason it should be removed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"argument is a duplicate of an inline argument" is an intuitive error suggestion I think? or should I include ", consider removing it" afterwards?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2022

r? @estebank

@rustbot rustbot assigned estebank and unassigned oli-obk Dec 13, 2022
@estebank
Copy link
Contributor

Replying to your question from the ticket:

You should look at all the formatting placeholders that have an argumentof FormatArgumentKind::Named(ident), where ident would be x. From that ident you will also have a Span to point at after realizing that the name also appears in the arg list (which you already found in the PR). Does this make sense?

Ideally the end result would be something along the lines of

error: argument never used, consider removing it
  --> $DIR/issue-105225.rs:3:21
   |
LL |     println!("{x}", x);
   |              -----  ^ argument never used
   |              |
   |              formatting specifier missing
   |
note: the formatting string captures that binding directly already, it doesn't need to be included in the argument list
   |
LL |     println!("{x}", x);
   |               ^^^   - consider removing this
   |               |
   |               this formatting specifier is referencing the `x` binding


@atamakahere-git
Copy link
Author

Sure will do that soon! Thanks.

@atamakahere-git
Copy link
Author

Hello @estebank ,
I have implemented the suggestion message as you said above, I was not able to do exactly the same because I didn't find a way to make a nested span (as you did in your example after note: ), Making a new DiagnosticBuilder will add up an error ( ecx.struct_span_err() ) which is not suitable.

So, If I get to know how to reference these spans again after the note, I'll do it. I'm trying to figure it out but if possible please let me know.

Though the current suggestion is intuitive but I think your representation is more friendly.

@atamakahere-git
Copy link
Author

Hello @estebank can you please review the PR 😅

Comment on lines 1 to 14
error: argument never used
--> $DIR/issue-105225.rs:3:21
|
LL | println!("{x}", x);
| ----- ^ argument never used
| |
| formatting specifier missing
|
= note: the formatting string captures that binding directly, it doesn't need to be included in the argument list
help: Consider removing this
--> $DIR/issue-105225.rs:3:21
|
LL | println!("{x}", x);
| ^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
error: argument never used
--> $DIR/issue-105225.rs:3:21
|
LL | println!("{x}", x);
| ----- ^ argument never used
| |
| formatting specifier missing
|
= note: the formatting string captures that binding directly, it doesn't need to be included in the argument list
help: Consider removing this
--> $DIR/issue-105225.rs:3:21
|
LL | println!("{x}", x);
| ^
error: argument never used
--> $DIR/issue-105225.rs:3:21
|
LL | println!("{x}", x);
| ----- ^ argument never used
| |
| formatting specifier missing
|
note: the formatting string captures that binding directly, it doesn't need to be included in the argument list
--> $DIR/issue-105225.rs:3:21
|
LL | println!("{x}", x);
| ^ - this can be removed
| |
| this formatting argument captures `x` directly

In order to add labels in a note you'd have to use a terrible API (details fudged):

let mut span: MultiSpan = vec_primary_spans.into(); // this should be the formatting arg
for arg in args {
    span.push_span_label(arg.span, format!("this formatting argument captures `{}` directly", arg.name));
}
for span in duplicate_explicit_arg {
    span.push_span_label(span, "this can be removed".to_string());
}

@estebank
Copy link
Contributor

Thanks for the ping!

Could you also add a couple of more test cases where multiple arguments are redundantly captured?

@estebank estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2023
@atamakahere-git
Copy link
Author

Currently going through university exams, will update soon.

@atamakahere-git
Copy link
Author

atamakahere-git commented Jan 14, 2023

There was a small problem, The InnerSpan that arg.position_span returns was rustc_parse_format::InnserSpan, so I had to make a new InnerSpan inline to convert it to rustc_span::InnerSpan
Also do I have to update the repo for test folder migration?
If there's any other workaround let me know.

@rust-log-analyzer

This comment has been minimized.

Comment on lines 477 to 523
if let Some(expr) = args.explicit_args()[i].expr.to_ty() {
if let Some(symbol) = expr.kind.is_simple_path() {
let current_arg = symbol.as_str().to_owned();
let current_arg_ph = format!("{{{current_arg}}}");
if current_arg.len() > 0 && fmt_str.contains(current_arg_ph.as_str()) {
duplicate_explicit_arg.push(args.explicit_args()[i].expr.span);
for piece in &pieces {
if let NextArgument(arg) = piece {
if let ArgumentNamed(specifier) = arg.position {
if specifier == symbol.to_string() {
redundant_expl_args.push({
RedundantArgDiagBuilder {
arg_span: expr.span,
fmt_span: InnerSpan {
start: arg.position_span.start,
end: arg.position_span.end,
},
fmt_str: specifier,
}
});
}
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can take the chance to leverage let chains and reduce the indentation levels of this code:

                if let Some(expr) = args.explicit_args()[i].expr.to_ty()
                    && let Some(symbol) = expr.kind.is_simple_path()
                {
                    let current_arg = symbol.as_str().to_owned();
                    for piece in &pieces {
                        if let NextArgument(arg) = piece
                            && let ArgumentNamed(specifier) = arg.position
                        {
                            if specifier == symbol.to_string() {
                                redundant_expl_args.push(RedundantArgDiagBuilder {
                                    arg_span: expr.span,
                                    fmt_span: InnerSpan {
                                        start: arg.position_span.start,
                                        end: arg.position_span.end,
                                    },
                                    fmt_str: specifier,
                                });
                            }
                        }
                    }
                }

for arg_span in arg_spans {
m_span.push_span_label(arg_span, "this can be removed".to_string());
}
diag.span_help(m_span, "the formatting string captures that binding directly, it doesn't need to be included in the argument list");
Copy link
Contributor

@estebank estebank Jan 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should customize the output depending on the number of bindings:

Suggested change
diag.span_help(m_span, "the formatting string captures that binding directly, it doesn't need to be included in the argument list");
let mut bindings: Vec<_> = arg.iter().collect();
bindings.sort();
bindings.dedup();
diag.span_help(
m_span,
&format!(
"the formatting string captures {} directly, {} need to \
be included in the argument list",
match &bindings[..] {
[.., last] if bindings.len() < 8 => format!("bindings `{}` and `{last}`"),
[binding] => format!("binding {binding}"),
_ => "these bindings".to_string(),
},
if bindings.len() == 1 {
"it doesn't"
} else {
"they don't"
},
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This request is still valid :)

You can choose to simplify it, but at least I'd like to specialize the output for a single binding as opposed to multiple.

@estebank
Copy link
Contributor

You will have to rebase on top of the latest version in the repo. I see that you were working on your checkout's master branch, which will make things slightly less intuitive (but few things with git are). The following should work:

git fetch origin
git rebase origin/master

At that point you might have to fix merge conflicts. I find that some kind of interactive UI can help make sense of the situation. But there's potential for things to go awry, so please read https://rustc-dev-guide.rust-lang.org/git.html?highlight=rebase#rebasing and maybe make a backup of your current changes (git checkout -b fmt-bkp to create a new branch with the current state and then checkout master to go back).

BTW this looks awesome! Thank you for working on it!

@atamakahere-git
Copy link
Author

Thank you for your kind and detailed review, Will revert ASAP.

@atamakahere-git
Copy link
Author

I have found a problem with my code while testing,
for the snippet

println!("{x}{x}{x}{x}",x,y,y,x);

The suggestion provided is

help: the formatting string captures bindings `x` directly, it doesn't need to be included in the argument list
 --> new.rs:4:16
  |
4 |     println!("{x}{x}{x}{x}", x, y, y, x);
  |                ^  ^  ^  ^    -        -
  |                |  |  |  |    |        |
  |                |  |  |  |    |        this can be removed
  |                |  |  |  |    |        this can be removed
  |                |  |  |  |    |        this can be removed
  |                |  |  |  |    |        this can be removed
  |                |  |  |  |    this can be removed
  |                |  |  |  |    this can be removed
  |                |  |  |  |    this can be removed
  |                |  |  |  |    this can be removed
  |                |  |  |  this formatting argument captures `x` directly
  |                |  |  |  this formatting argument captures `x` directly
  |                |  |  this formatting argument captures `x` directly
  |                |  |  this formatting argument captures `x` directly
  |                |  this formatting argument captures `x` directly
  |                |  this formatting argument captures `x` directly
  |                this formatting argument captures `x` directly
  |                this formatting argument captures `x` directly

error: aborting due to previous error

This is happening because of lines :

https://github.com/tanveerraza789/rust/blob/master/compiler/rustc_builtin_macros/src/format.rs#L477-L482

we are checking Argument and NextArgument both of which contain the explicit Argument passed, Not the placeholder that the format string has i.e. {x}{y} x,y in this case.

Hence I need a way to find which binding is given inside the placeholder. pieces contain string literals(which are non-placeholder) and the NextArgument type only.

@Dylan-DPC
Copy link
Member

@tanveerraza789 what's the update on this?

@atamakahere-git
Copy link
Author

Hello @Dylan-DPC Give me a week (7d) if I'm not able to solve this then I'll unclaim this issue.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after addressing the nitpicks

Thank you for doing this! It should be very helpful for newcomers that don't know about binding captures yet.

compiler/rustc_builtin_macros/src/format.rs Outdated Show resolved Hide resolved
compiler/rustc_builtin_macros/src/format.rs Outdated Show resolved Hide resolved
@atamakahere-git
Copy link
Author

I've made the changes you said, I've also added the skeleton of the logic I was thinking of but could not implement due to some reasons, like getting &str back from Span .
The test will fail because I have added the desired stdout for the example but It produces something weird.
Let me know of any changes, I'll do it ASAP.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-14 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

---- [ui] tests/ui/macros/issue-105225.rs stdout ----
diff of stderr:

85    |                      this formatting argument captures `y` directly
87 error: multiple unused formatting arguments
-   --> new.rs:14:37
+   --> $DIR/issue-105225.rs:14:37
89    |
89    |
- 14 |     println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
+ LL |     println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
91    |              ---------------------  ^  ^  ^  ^  ^ argument never used
93    |              |                      |  |  |  argument never used

97    |              multiple missing formatting specifiers
98    |
98    |
99 help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
-   --> new.rs:14:16
101    |
101    |
102 LL |     println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
-    |                ^                    - this can be removed
-    |                |
-    |                this formatting argument captures `y` directly
+    |                ^   ^   ^   ^   ^    -  -  -  -  - this can be removed
+    |                |   |   |   |   |    |  |  |  |
+    |                |   |   |   |   |    |  |  |  this can be removed
+    |                |   |   |   |   |    |  |  this can be removed
+    |                |   |   |   |   |    |  |  this can be removed
+    |                |   |   |   |   |    |  |  this can be removed
+    |                |   |   |   |   |    |  |  this can be removed
+    |                |   |   |   |   |    |  this can be removed
+    |                |   |   |   |   |    this can be removed
+    |                |   |   |   |   |    this can be removed
+    |                |   |   |   |   |    this can be removed
+    |                |   |   |   |   |    this can be removed
+    |                |   |   |   |   this formatting argument captures `x` directly
+    |                |   |   |   |   this formatting argument captures `x` directly
+    |                |   |   |   this formatting argument captures `x` directly
+    |                |   |   |   this formatting argument captures `x` directly
+    |                |   |   this formatting argument captures `y` directly
+    |                |   |   this formatting argument captures `y` directly
+    |                |   |   this formatting argument captures `y` directly
+    |                |   this formatting argument captures `x` directly
+    |                |   this formatting argument captures `x` directly
+    |                this formatting argument captures `x` directly
+    |                this formatting argument captures `x` directly
106 error: aborting due to 6 previous errors
107 
108 

---
To only update this specific test, also pass `--test-args macros/issue-105225.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/macros/issue-105225.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "--remap-path-prefix=/checkout/tests/ui=fake-test-src-base" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/issue-105225" "-A" "unused" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/issue-105225/auxiliary"
stdout: none
--- stderr -------------------------------
error: argument never used
  --> fake-test-src-base/macros/issue-105225.rs:3:21
LL |     println!("{x}", x);
LL |     println!("{x}", x);
   |              -----  ^ argument never used
   |              formatting specifier missing
   |
   |
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
  --> fake-test-src-base/macros/issue-105225.rs:3:16
LL |     println!("{x}", x);
   |                ^    - this can be removed
   |                |
   |                |
   |                this formatting argument captures `x` directly
error: multiple unused formatting arguments
  --> fake-test-src-base/macros/issue-105225.rs:6:25
   |
   |
LL |     println!("{x} {y}", x, y);
   |              ---------  ^  ^ argument never used
   |              |          argument never used
   |              multiple missing formatting specifiers
   |
   |
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
  --> fake-test-src-base/macros/issue-105225.rs:6:16
   |
LL |     println!("{x} {y}", x, y);
   |                ^   ^    -  - this can be removed
   |                |   |    |
   |                |   |    this can be removed
   |                |   this formatting argument captures `y` directly
   |                this formatting argument captures `x` directly
error: multiple unused formatting arguments
  --> fake-test-src-base/macros/issue-105225.rs:8:25
   |
   |
LL |     println!("{x} {y}", y, x);
   |              ---------  ^  ^ argument never used
   |              |          argument never used
   |              multiple missing formatting specifiers
   |
   |
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
  --> fake-test-src-base/macros/issue-105225.rs:8:16
   |
LL |     println!("{x} {y}", y, x);
   |                ^   ^    -  - this can be removed
   |                |   |    |
   |                |   |    this can be removed
   |                |   this formatting argument captures `y` directly
   |                this formatting argument captures `x` directly
error: argument never used
  --> fake-test-src-base/macros/issue-105225.rs:10:27
   |
   |
LL |     println!("{} {y}", x, y);
   |              --------     ^ argument never used
   |              formatting specifier missing
   |
   |
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
  --> fake-test-src-base/macros/issue-105225.rs:10:19
   |
LL |     println!("{} {y}", x, y);
   |                   ^       - this can be removed
   |                   |
   |                   this formatting argument captures `y` directly
error: argument never used
  --> fake-test-src-base/macros/issue-105225.rs:12:45
   |
   |
LL |     println!("{} {} {y} {} {}", y, y, y, y, y);
   |              -----------------              ^ argument never used
   |              formatting specifier missing
   |
   |
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
  --> fake-test-src-base/macros/issue-105225.rs:12:22
   |
LL |     println!("{} {} {y} {} {}", y, y, y, y, y);
   |                      ^                      - this can be removed
   |                      |
   |                      this formatting argument captures `y` directly
error: multiple unused formatting arguments
  --> fake-test-src-base/macros/issue-105225.rs:14:37
   |
   |
LL |     println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
   |              ---------------------  ^  ^  ^  ^  ^ argument never used
   |              |                      |  |  |  argument never used
   |              |                      |  |  argument never used
   |              |                      |  argument never used
   |              |                      argument never used
   |              |                      argument never used
   |              multiple missing formatting specifiers
   |
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
  --> fake-test-src-base/macros/issue-105225.rs:14:16
   |
LL |     println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
   |                ^   ^   ^   ^   ^    -  -  -  -  - this can be removed
   |                |   |   |   |   |    |  |  |  |
   |                |   |   |   |   |    |  |  |  this can be removed
   |                |   |   |   |   |    |  |  this can be removed
   |                |   |   |   |   |    |  |  this can be removed
   |                |   |   |   |   |    |  |  this can be removed
   |                |   |   |   |   |    |  |  this can be removed
   |                |   |   |   |   |    |  this can be removed
   |                |   |   |   |   |    this can be removed
   |                |   |   |   |   |    this can be removed
   |                |   |   |   |   |    this can be removed
   |                |   |   |   |   |    this can be removed
   |                |   |   |   |   this formatting argument captures `x` directly
   |                |   |   |   |   this formatting argument captures `x` directly
   |                |   |   |   this formatting argument captures `x` directly
   |                |   |   |   this formatting argument captures `x` directly
   |                |   |   this formatting argument captures `y` directly
   |                |   |   this formatting argument captures `y` directly
   |                |   |   this formatting argument captures `y` directly
   |                |   this formatting argument captures `x` directly
   |                |   this formatting argument captures `x` directly
   |                this formatting argument captures `x` directly
   |                this formatting argument captures `x` directly
error: aborting due to 6 previous errors
------------------------------------------


@bors
Copy link
Contributor

bors commented Apr 11, 2023

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

@Dylan-DPC
Copy link
Member

@tanveerraza789 any updates on this?

@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC closed this Jul 30, 2023
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 30, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 18, 2023
Suggest removing redundant arguments in format!()

Closes rust-lang#105225. This is also a follow-up to rust-lang#105635, which seems to have become stale.

r? `@estebank`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2023
Suggest removing redundant arguments in format!()

Closes rust-lang#105225. This is also a follow-up to rust-lang#105635, which seems to have become stale.

r? `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect println!("{x}", x)
7 participants