Allow unary operand types to be inferred later

[chenyukang]

View all comments

Fixes #106138

Fix the inference issue by allowing Not and Neg operand types to remain unresolved until obligations and constraints can determine them.

closes #26830

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 74 candidates
• Random selection from 20 candidates

[fee1-dead]

hard for me to know if this is the right thing to do, hmm..

r? types

[jackh726]

So, this is fairly minor, but is new behavior, so I wondering if a @rust-lang/types FCP might be appropriate.

View changes since this review

[jackh726]

This definitely needs a comment. But:

1. Can we just use resolve_vars_with_obligations here for all three?
2. If not, I think it makes sense to just move these into the match arms, rather than matching twice.

[chenyukang]

If we also use resolve_vars_with_obligations for Deref, this code will treat a later-inferred raw pointer as an overloaded Deref, for example this code was reporting a proper E0282:

fn make<T>() -> T {
    loop {}
}

fn main() {
    let pointer = make();
    //~^ ERROR type annotations needed
    let value = unsafe { *pointer };
    let _: *const u8 = pointer;
    let _: u8 = value;
}

error[E0282]: type annotations needed
  --> src/main.rs:9:9
   |
 9 |     let pointer = make();
   |         ^^^^^^^
10 |     //~^ ERROR type annotations needed
11 |     let value = unsafe { *pointer };
   |                          -------- type must be known at this point
   |
help: consider giving `pointer` an explicit type
   |
 9 |     let pointer: /* Type */ = make();
   |                ++++++++++++

For more information about this error, try `rustc --explain E0282`.

if we changed to use resolve_vars_with_obligations, the error changed to:

error[E0277]: the trait bound `*const u8: Deref` is not satisfied
  --> tests/ui/inference/unary-deref-late-raw-pointer-inference-issue-106138.rs:11:26
   |
11 |     let value = unsafe { *pointer };
   |                          ^^^^^^^^ the trait `Deref` is not implemented for `*const u8`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.

I'm not sure whether there is other cases, we'd better keep structurally_resolve_type for Deref, and I added this test code as a unit test.

[dianne]

This should also close #26830, I think?

Also, for what it's worth, this would slightly expand the surface area of #114380 (also reported as #151202). Currently, the following fails to type-check:

fn main() {
    let input = Default::default();
    let output = !{ input };
    let _: u8 = output;
    println!("{}", std::any::type_name_of_val(&input));
}

Under this PR, it would compile successfully and print u8. let output = !{ input }; erroneously coerces input to the expected type of the negation as a whole, unifying its type with that of output (which is now allowed to be inferred later).

[chenyukang]

This should also close #26830, I think?

Also, for what it's worth, this would slightly expand the surface area of #114380 (also reported as #151202). Currently, the following fails to type-check:

fn main() {
    let input = Default::default();
    let output = !{ input };
    let _: u8 = output;
    println!("{}", std::any::type_name_of_val(&input));
}

Under this PR, it would compile successfully and print u8. let output = !{ input }; erroneously coerces input to the expected type of the negation as a whole, unifying its type with that of output (which is now allowed to be inferred later).

I added a test in this PR for #26830

[chenyukang]

@rustbot ready

[jackh726]

@rfcbot merge types

Minor change here. This Not and Neg unary operators use resolve_vars_with_obligations instead of structurally_resolve_type, which lets us infer the output from later information.

[rust-rfcbot]

@jackh726 has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

Concerns:

• lang-decision resolved by Allow unary operand types to be inferred later #159744 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[jackh726]

@rfcbot concern lang-decision

@rust-lang/lang wants to discuss this further

[jackh726]

@rfcbot resolve lang-decision

see poll

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

@rfcbot poll lang

We discussed this in the lang-team meeting. Meeting consensus was that we are ok with this and so we decided not to create a rfcbot merge (but I'm using the poll so people can signal their assent).

We discussed two questions: what is the motivation for this change and are we ok with not having auto-deref here. The answer to both came down to a consistency argument. We felt that !x is conceptually similar to binary operators like x + y that consume a value and not similar to place-operators like index and field access. This change (iiuc) makes the type checker for ! behave more analogously to + (i.e., it consumes the value it is applied to directly and it creates a pending trait obligation that can be constrained at any point during method type checking). Similarly, + does not autoderef. Therefore, this makes sense.

[rust-rfcbot]

@nikomatsakis has asked teams: T-lang, for consensus on:

[scottmcm]

This is okay because we don't do auto-derefs/coercions here, we don't do this for indexing or field projections

The note from the meeting that I found particularly persuasive is that place projections are already a distinct category from value operations, and things like !x or -x are clearly not place projections, and thus them working more like x ^ -1 and 0 - x which also don't autoderef and do flow type inference information is good.

Plus we have existing impls like Not for &u32 that weakly imply no expectation of autoderef, and things like ops::Not for &BigInteger seem clearly reasonable anyway.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.