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

Don't suggest &mut mut #59808

Conversation

saleemjaffer
Copy link
Contributor

@saleemjaffer saleemjaffer commented Apr 9, 2019

fixes #57431

@rust-highfive
Copy link
Collaborator

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2019
@saleemjaffer
Copy link
Contributor Author

r? @spastorino

@rust-highfive rust-highfive assigned spastorino and unassigned varkor Apr 9, 2019
@saleemjaffer saleemjaffer changed the title adding a bool field in BindingMode::BindByValue Don't suggest &mut mut Apr 9, 2019
@saleemjaffer
Copy link
Contributor Author

@spastorino I have added a coerced field in BindByValue. Things only compile now. What do you think?

@saleemjaffer saleemjaffer force-pushed the remove_ampmut_mut_suggestions branch from cd0c6a5 to d543ae7 Compare April 9, 2019 05:06
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:24cb5920:start=1554786460307083927,finish=1554786536254755371,duration=75947671444
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@spastorino
Copy link
Member

Yeah, continue in this direction. I'm not entirely sure if adding the attribute there is the best idea as we discussed on Zulip. Anyway, keep this going, now you need to assign coerced to true in the right spot and handle that in the diagnostics part :).

@matthewjasper
Copy link
Contributor

Looking at the way this message is generated, I think that a larger change is probably deserved here.
If you just want to avoid the incorrect message though. I would suggest moving the flag to mir::Ravlue::Ref or mir::BorrowKind. You can then retrieve the flag by inspecting mir.basic_blocks()[locations[0].block].statements.get(locations[0].statement_index) here:

let assignment_rhs_span = mir.source_info(locations[0]).span;

@saleemjaffer
Copy link
Contributor Author

@matthewjasper This is the the output of mir.basic_blocks()[locations[0].block].statements.get(locations[0].statement_index):
Some( _3 = &(*_5),).

I have no idea how to make sense of what this means. Any help?

@matthewjasper
Copy link
Contributor

The Debug implementation for Statement pretty prints the statement. That's the output for something like

Statement {
    source_info: / * ... */,
    kind: StatementKind::Assign(
        _3,
        Rvalue::Ref('_, BorrowKind::Shared, *_5),
    ),
}

@saleemjaffer
Copy link
Contributor Author

saleemjaffer commented Apr 20, 2019

@matthewjasper

I tried: println!("{:#?}", mir.basic_blocks()[locations[0].block].statements.get(locations[0].statement_index));

Should I be doing something else?

Looks like we cannot pretty print this! Can we?

@bors
Copy link
Contributor

bors commented Apr 22, 2019

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

@matthewjasper
Copy link
Contributor

There's no way to structurally pretty print. You can match on the some case and print out the .kind, which has a derived Debug implementation (but it's fields all have hand written debug implementations). You can have a look at https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/enum.StatementKind.html to see how this type looks.

@matthewjasper
Copy link
Contributor

matthewjasper commented Apr 27, 2019

Alternatively you could look in mir/mod.rs and replace the manual debug implemenations with derived ones while you're working (and revert later).

impl<'tcx> Debug for Statement<'tcx> {

impl<'tcx> Debug for Place<'tcx> {

impl<'tcx> Debug for Operand<'tcx> {

impl<'tcx> Debug for Rvalue<'tcx> {

@Mark-Simulacrum Mark-Simulacrum 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 18, 2019
@spastorino
Copy link
Member

r? @varkor, assigning you again because I don't have rights to merge. Anyway this is not ready :).

@rust-highfive rust-highfive assigned varkor and unassigned spastorino May 27, 2019
@@ -828,7 +828,7 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
match bm {
ty::BindByReference(..) =>
mode.lub(BorrowingMatch),
ty::BindByValue(..) => {
ty::BindByValue{..} => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ty::BindByValue{..} => {
ty::BindByValue { .. } => {

(here and elsewhere)

@jonas-schievink
Copy link
Contributor

Ping from triage @saleemjaffer, there's merge conflicts waiting to be resolved

@pnkfelix
Copy link
Member

I'm going to close this PR since it seems to be inactive and incomplete. (If nothing else, it lacks a regression test.)

as @matthewjasper said, we probably should try to resolve the problem of #57431 via a larger change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

&mut &T coerced to &T suggests &mut mut x
9 participants