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

Fix #7903 #7906

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Fix #7903 #7906

merged 1 commit into from
Nov 2, 2021

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Oct 31, 2021

Fixes #7903 (cc: @Arnavion)

changelog: none (bug is in same release)

r? @camsteffen

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 31, 2021
@camsteffen
Copy link
Contributor

camsteffen commented Nov 1, 2021

    println!("{} and again {0}", x.to_string());

The lint should fire, but it was not firing.

That is a negative test because caching the string might be faster than calling <X as Display>::fmt twice.

if let Ok(i) = usize::try_from(position);
let arg = &self.args[i];
if let ExprKind::Call(_, [arg_name, _]) = arg.kind;
if let Some(j) = self.arg_names.iter().position(|pat| match_arg(pat, arg_name));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should work

Suggested change
if let Some(j) = self.arg_names.iter().position(|pat| match_arg(pat, arg_name));
if let Some(j) = self.arg_names.iter().position(|pat| path_to_local_id(arg_name, pat.hir_id));

@@ -557,13 +559,15 @@ impl FormatArgsExpn<'tcx> {
_ => None,
})
.collect();
if let PatKind::Tuple(arg_names, None) = arm.pat.kind;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is okay for now since it fixes an ICE, but note that we want to move away from depending on the exact HIR per #7843.


println!("error: something failed at {}", Somewhere.to_string());
println!("{} and again {0}", x.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

See my other comment on this. Comments would be helpful here. One to separate // negative tests and one to explain the intent behind this test since it isn't obvious.

@smoelius smoelius changed the title Fix #7903 and to_string_in_format_args false negative Fix #7903 Nov 1, 2021
@smoelius smoelius force-pushed the master branch 2 times, most recently from e6cd02d to 61bb9b6 Compare November 1, 2021 23:31
@smoelius
Copy link
Contributor Author

smoelius commented Nov 1, 2021

I think I've addressed all of your comments, @camsteffen. Thanks a lot for reviewing this.

@camsteffen
Copy link
Contributor

Thanks! I added a changelog line to the PR.

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 2, 2021

📌 Commit 5edb02a has been approved by camsteffen

@bors
Copy link
Collaborator

bors commented Nov 2, 2021

⌛ Testing commit 5edb02a with merge 2ebd535...

@bors
Copy link
Collaborator

bors commented Nov 2, 2021

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

@bors bors merged commit 2ebd535 into rust-lang:master Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
4 participants