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 parentheses properly for borrowing suggestion #105019

Merged
merged 1 commit into from
Feb 11, 2023

Conversation

chenyukang
Copy link
Member

Fixes #104961

@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2022

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@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 Nov 28, 2022
@chenyukang
Copy link
Member Author

It's a hacky fix, but seems we also use snippet for this.
There should be a completed fix if we can get expr kind stuff here, like this:

let needs_parens = match expr.kind {

@@ -1118,13 +1118,22 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
);
} else {
let is_mut = mut_ref_self_ty_satisfies_pred || ref_inner_ty_mut;
// FIXME: hacky way to get the correct suggestion, see #104961
Copy link
Member

Choose a reason for hiding this comment

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

I would honestly just suggest &(<expr>) always, it's too difficult to create a correct check here, since you could do String::new() + ("").

Since we don't have access to the hir::Expr, we should just always be pessimistic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, let me check how we passing to here, I had a dig yesterday, seems need lot of changes.

@cjgillot cjgillot 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 Dec 25, 2022
@chenyukang
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2023
if !needs_parens {
sugg_prefix
} else {
format!("{}({})", sugg_prefix, snippet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestions that copy user code are typically hard to understand: everything is underlined, and the difference is not underlined.
Could you prefer a multipart suggestion with &( at the beginning and ) at the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

make sense, fixed.

@@ -1351,13 +1352,34 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the parentheses be added to this suggestion too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, I go through all the UI testcase come from this part,
they are all with single argument, and scenrio here is both & and &mut can be added for borrowing.

I haven't figure out an case which we should add parentheses here.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 10, 2023

📌 Commit 8f736a4 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2023
Rollup of 9 pull requests

Successful merges:

 - rust-lang#105019 (Add parentheses properly for borrowing suggestion)
 - rust-lang#106001 (Stop at the first `NULL` argument when iterating `argv`)
 - rust-lang#107098 (Suggest function call on pattern type mismatch)
 - rust-lang#107490 (rustdoc: remove inconsistently-present sidebar tooltips)
 - rust-lang#107855 (Add a couple random projection tests for new solver)
 - rust-lang#107857 (Add ui test for implementation on projection)
 - rust-lang#107878 (Clarify `new_size` for realloc means bytes)
 - rust-lang#107888 (revert rust-lang#107074, add regression test)
 - rust-lang#107900 (Zero the `REPARSE_MOUNTPOINT_DATA_BUFFER` header)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0781d5d into rust-lang:master Feb 11, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

"consider borrowing here" suggestion is missing parentheses
5 participants