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

Extend Infer ty for binary operators #107567

Closed

Conversation

chenyukang
Copy link
Member

Fixes #106138

@rustbot
Copy link
Collaborator

rustbot commented Feb 1, 2023

r? @compiler-errors

(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 Feb 1, 2023
@chenyukang
Copy link
Member Author

This may a little bit aggressive, my intution is when the operand ty is a Infer var, neg and not may also return a Infer var type. Otherwise we need to check whether an Infer var type support the neg and not operation, which will introduce an error type.

With this change, validate.rs also need to change, codegen is OK. The validation for binary op is introduced by 9ac5e98, without test case, I'm not sure the reason for this.

@@ -383,6 +383,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let mut oprnd_t = self.check_expr_with_expectation(&oprnd, expected_inner);

if !oprnd_t.references_error() {
match (unop, oprnd_t.kind()) {
Copy link
Member

@compiler-errors compiler-errors Feb 1, 2023

Choose a reason for hiding this comment

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

This inference strategy is incorrect. There's no guarantee that the input and output type of Neg or Not are related, and this causes an ICE in the compiler:

#[derive(Copy, Clone, Default)]
struct A;

struct B;

impl std::ops::Not for A {
    type Output = B;

    fn not(self) -> B { B }
}

fn main() {
    let x = Default::default();
    let y: A = !x;
}

Copy link
Member

Choose a reason for hiding this comment

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

In this example x is constrained to be A because of the inference, but that would imply that y is type B, not A.

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 👍
this is a trivial difference here, binary op is more eager, so unary will infer type error.

pub fn bools(x: &Vec<bool>) {
    let binary = |i, a: &Vec<bool>| {
        a[i] && a[i+1] // ok
    };

    let unary = |i, a: &Vec<bool>| {
        !a[i] // cannot infer type
    };

    binary(0, x);
    unary(0, x);
}

Seems we can not find proper fix for this scenario?

}
}
}
Rvalue::UnaryOp(..) => {}
Copy link
Member

Choose a reason for hiding this comment

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

This validation should not be removed without at least a valid explanation for why it's not necessary anymore 😓

Copy link
Member

Choose a reason for hiding this comment

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

cc @JakobDegen who added this validation

Copy link
Member Author

Choose a reason for hiding this comment

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

It's too late last night, this should be a Draft.
I just found more document for this validations, 6343691

@chenyukang chenyukang marked this pull request as draft February 2, 2023 01:50
@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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 Feb 2, 2023
@bors
Copy link
Contributor

bors commented Feb 4, 2023

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

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. 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.

cannot infer type when expression is logically negated in closure
4 participants