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.

Sign up for GitHub

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

Jump to bottom

Fix invalid suggestion of changing impl trait signature #85100

Merged
merged 4 commits into from
May 11, 2021
Merged

Fix invalid suggestion of changing impl trait signature #85100

merged 4 commits into from
May 11, 2021
+149 −10

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented May 9, 2021

Fix #68049

@HKalbasi
Try to fix issue 68049

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
efc8f65
@rust-highfive
Copy link
Contributor

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 9, 2021
@HKalbasi
Copy link
Member Author

HKalbasi commented May 9, 2021

Seems @matthewjasper is busy (from github contributions). May I request another reviewer?
r? @Aaron1011

@Aaron1011
Copy link
Member

Thank you for your contribution!

There's a large amount of rightward drift in the diff. If it's still there after you switch to tcx.opt_associated_item, could you make a new method for the logic you're adding? That will allow you to return early when a condition doesn't match, which should help keep the rightward drift down.

@Aaron1011 Aaron1011 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 May 9, 2021
@HKalbasi
Copy link
Member Author

I moved the logic to a function so right drift is almost solved. I don't think it helps to use tcx.opt_associated_item because I need trait object (td) and my_hir.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@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 May 10, 2021
@Aaron1011
Copy link
Member

I see what you mean about still needing to use the HIR to get the trait method argument span. However, using opt_associated_item should make some of the code preceding that loop much shorter

@HKalbasi
Copy link
Member Author

@Aaron1011 Is it now ok?

@Aaron1011
Copy link
Member

@HKalbasi Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented May 10, 2021

📌 Commit 8f5585a has been approved by Aaron1011

@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 May 10, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 10, 2021
@Dylan-DPC
Rollup merge of rust-lang#85100 - HKalbasi:issue-68049-fix, r=Aaron1011
e0af56d 
Fix invalid suggestion of changing impl trait signature

Fix rust-lang#68049
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 10, 2021
Closed
@JohnTitor
Copy link
Member

Failed in rollup: #85163 (comment)
Note that someone should approve the workflow for first-time contributors.
@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 11, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@HKalbasi
Copy link
Member Author

I thought ./x.py test tidy --bless will fix styling problems automatically.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@HKalbasi
Copy link
Member Author

What is happening here? tidy --bless fix styling but just have problem with trailing whitespaces?

@JohnTitor
Copy link
Member

IIRC tidy --bless won't fix all the tidy failures, and I guess that the first failure hides the later ones.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@HKalbasi
Copy link
Member Author

But trailing space is the easiest one I think. Maybe it is a bug?

@JohnTitor
Copy link
Member

But trailing space is the easiest one I think. Maybe it is a bug?

Nah, it isn't a bug, --bless means x.py fmt and it won't remove trailing spaces. Just checked on my local and tidy exits with code 1 correctly so I think you missed the failure on your console.

@HKalbasi
Copy link
Member Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@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 May 11, 2021
@JohnTitor
Copy link
Member

Could you squash extra formatting commits into the existing one to clean up the commit log? Once it's done, I'll back this into the queue.

@HKalbasi
Copy link
Member Author

Done

@JohnTitor
Copy link
Member

Thanks! @bors r=Aaron1011

@bors
Copy link
Collaborator

bors commented May 11, 2021

📌 Commit 1f20966 has been approved by Aaron1011

@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 May 11, 2021
@bors
Copy link
Collaborator

bors commented May 11, 2021

⌛ Testing commit 1f20966 with merge 382f748...

@bors
Copy link
Collaborator

bors commented May 11, 2021

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 382f748 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 11, 2021
@bors bors merged commit 382f748 into rust-lang:master May 11, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 11, 2021
@bors bors mentioned this pull request May 11, 2021
Merged
@HKalbasi HKalbasi deleted the issue-68049-fix branch August 7, 2021 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011

Assignees

@Aaron1011 Aaron1011

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.

Invalid suggestion to change &self to &mut self in trait implementation
8 participants
@HKalbasi @rust-highfive @Aaron1011 @bors @JohnTitor @rust-log-analyzer @matthewjasper @rustbot