rustc_typeck: use subtyping on the LHS of binops. #45435

eddyb · 2017-10-21T17:19:40Z

Fixes #45425.

r? @nikomatsakis

eddyb · 2017-10-22T03:38:10Z

Crater report has only false positives.

nikomatsakis

Seems ok for the most part. I left a few questions to make sure I'm following along.

nikomatsakis · 2017-10-23T17:11:41Z

src/librustc_typeck/check/coercion.rs

@@ -763,7 +763,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {

        let (adjustments, _) = self.register_infer_ok_obligations(ok);
        self.apply_adjustments(expr, adjustments);
-        Ok(target)
+        Ok(self.resolve_type_vars_with_obligations(target))


Is there a reason for this? Just to improve debug output, or something else?

It's used to avoid repeatedly doing this in the callers.

So you mean that the callers rely on type variables having been resolved in the return value?

Yeah, in the changed binop code.

Then I think we should add some comment that this is important, and also comment the interface of the coercion function. I'm not all that keen on having such a guarantee, though -- it's a subtle link between code imo -- but if you can at least document who cares about it, it would be helpful.

Well then I'll just throw away most of the changes, I guess... I thought this API was much nicer but I can just as well have two extra lines in op.rs instead.

nikomatsakis · 2017-10-23T17:15:53Z

src/librustc_typeck/check/demand.rs

        let expected = self.resolve_type_vars_with_obligations(expected);

-        if let Err(e) = self.try_coerce(expr, checked_ty, self.diverges.get(), expected) {
+        self.try_coerce(expr, checked_ty, self.diverges.get(), expected).map_err(|e| {


Nit: this whole foo.map_err().map().unwrap_or is getting a bit over the top and kind of hard to follow. Can we just rewrite this with a match?

Something like:

let e = match self.try_coerce(...) { Ok(ty) => return (ty, None), Err(e) => e, }; ... (expected, some(err))

I wanted to avoid intending the whole function right, but I'm okay with a match either.

nikomatsakis · 2017-10-23T17:20:26Z

src/librustc_typeck/check/op.rs

+        // trait matching creating lifetime constraints that are too strict.
+        // E.g. adding `&'a T` and `&'b T`, given `&'x T: Add<&'x T>`, will result
+        // in `&'a T <: &'x T` and `&'b T <: &'x T`, instead of `'a = 'b = 'x`.
+        let lhs_ty = self.check_expr_coercable_to_type_with_lvalue_pref(lhs_expr,


Why use coercion here rather than subtyping? I guess it is equivalent anyway, at least for the way we handle coercions now, since if you coerce with a target type of a variable you just get subtyping anyway, right?

OTOH, if we ever support "deferred coercion", I don't see any reason not to use coercion here, at least for by-value operators like + -- not entirely sure about ==.

Subtyping is trickier to get right, I'm far more comfortable using coercion instead.

Trickier in what sense? Coercing is a superset of subtyping, so using it can only cause more things to happen.

I mean doing subtyping manually - I know coercion uses subtyping correctly and I'd rather use an API where it's obvious how not to misuse it.

I guess I'm just nervous that, if coercion gets smarter, we'll now be performing random coercions here -- as opposed to just subtyping -- which maybe we do not want to do. But I can't really think of a good reason not to use coercion.

We already coerce the RHS FWIW.

That does make me feel better =)

nikomatsakis · 2017-10-23T17:21:18Z

src/test/run-pass/issue-32008.rs

+fn try_to_add(input: &Foo) {
+    let local = Foo;
+    &*input + &local; // ok
+    input + &local; // Error!


Can you remove the // Error! comment

Heh I was considering it.

nikomatsakis · 2017-10-23T17:21:49Z

src/test/run-pass/issue-32008.rs

+
+use std::ops::Add;
+
+struct Foo;


Can you add a comment explaining what this is all about? i.e,. "Check that we permit subtyping for operator LHS" or something.

eddyb · 2017-10-28T13:15:10Z

@nikomatsakis Let me know if you want more changes to this.

nikomatsakis · 2017-10-31T14:38:31Z

@eddyb are you going to either comment or remove the "resolve type variables" line? I'm ok either way. Other than that, r=me.

nikomatsakis · 2017-10-31T15:48:34Z

@bors r+

bors · 2017-10-31T15:48:35Z

📌 Commit 1a7fb7d has been approved by nikomatsakis

bors · 2017-11-01T09:40:21Z

⌛ Testing commit 1a7fb7d with merge 2f581cf...

@nikomatsakis

rustc_typeck: use subtyping on the LHS of binops. Fixes #45425. r? @nikomatsakis

bors · 2017-11-01T12:16:14Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2f581cf to master...

alexcrichton · 2017-11-04T14:28:39Z

It looks like this may have regressed the inflate benchmark by ~5%?

eddyb · 2017-11-04T15:40:50Z

I suppose that more inference variables are created even without any lifetimes being involved.

@nikomatsakis, are there any plans to handle lifetime subtyping/generalization more efficiently during typeck?

Rust 1.22 fixes the original issue (rust-ndarray#103) that required `'b` to be added. (See rust-lang/rust#32008 and rust-lang/rust#45435 for the issue and fix in Rust.)

Rust 1.23 fixes the original issue (rust-ndarray#103) that required `'b` to be added. (See rust-lang/rust#32008 and rust-lang/rust#45435 for the issue and fix in Rust.)

Rust 1.23 fixed the original issue (rust-ndarray#103) that required `'b` to be added. (See rust-lang/rust#32008 and rust-lang/rust#45435 for the issue and fix in Rust.)

Rust 1.23 fixed the original issue (rust-ndarray#103) that required `'b` to be added. (See rust-lang/rust#32008, rust-lang/rust#45425, and rust-lang/rust#45435 for the relevant Rust issues/PRs.)

…iant, r=eddyb LHS of assign op is invariant This addresses a bug injected by #45435. That PR changed the way we type-check `LHS <op> RHS` to coerce the LHS to the expected supertype in much the same way that we coerce the RHS. The problem is that when we have a case of `LHS <op>= RHS`, we do not want to coerce to a supertype; we need the type to remain invariant. Otherwise we risk leaking a value with short-lifetimes into a expression context that needs to satisfy a long lifetime. Fix #52126

rust-highfive assigned nikomatsakis Oct 21, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2017

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 22, 2017

nikomatsakis reviewed Oct 23, 2017

View reviewed changes

Freyskeyd mentioned this pull request Oct 26, 2017

Confusing/inconsistent lifetime error with == on types with different covariant lifetimes #45534

Closed

eddyb force-pushed the binop-subtype-lhs branch from 73af9d7 to 7a2a638 Compare October 28, 2017 13:14

rustc_typeck: use subtyping on the LHS of binops.

1a7fb7d

eddyb force-pushed the binop-subtype-lhs branch from 7a2a638 to 1a7fb7d Compare October 31, 2017 15:35

kennytm 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 Nov 1, 2017

bors added a commit that referenced this pull request Nov 1, 2017

Auto merge of #45435 - eddyb:binop-subtype-lhs, r=nikomatsakis

2f581cf

rustc_typeck: use subtyping on the LHS of binops. Fixes #45425. r? @nikomatsakis

bors merged commit 1a7fb7d into rust-lang:master Nov 1, 2017

eddyb deleted the binop-subtype-lhs branch November 1, 2017 13:04

Havvy mentioned this pull request Dec 19, 2017

Document "use subtyping on the LHS of binops" rust-lang/reference#179

Open

jturner314 mentioned this pull request May 10, 2018

Remove unnecessary lifetime parameter rust-ndarray/ndarray#448

Merged

pnkfelix mentioned this pull request Jul 18, 2018

Problem with borrow checker and AddAssign #52126

Closed

pnkfelix mentioned this pull request Jul 20, 2018

LHS of assign op is invariant #52564

Merged

matthewjasper mentioned this pull request Jan 30, 2019

Unexpected lifetime issue when comparing Option<&i32> #42966

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_typeck: use subtyping on the LHS of binops. #45435

rustc_typeck: use subtyping on the LHS of binops. #45435

eddyb commented Oct 21, 2017

eddyb commented Oct 22, 2017

nikomatsakis left a comment

nikomatsakis Oct 23, 2017

eddyb Oct 23, 2017

nikomatsakis Oct 25, 2017

eddyb Oct 25, 2017

nikomatsakis Oct 30, 2017

eddyb Oct 31, 2017

nikomatsakis Oct 23, 2017

eddyb Oct 23, 2017

nikomatsakis Oct 23, 2017

eddyb Oct 23, 2017

nikomatsakis Oct 25, 2017

eddyb Oct 25, 2017

nikomatsakis Oct 30, 2017

eddyb Oct 30, 2017

nikomatsakis Oct 31, 2017

nikomatsakis Oct 23, 2017

eddyb Oct 23, 2017

nikomatsakis Oct 23, 2017

eddyb commented Oct 28, 2017

nikomatsakis commented Oct 31, 2017

nikomatsakis commented Oct 31, 2017

bors commented Oct 31, 2017

bors commented Nov 1, 2017

bors commented Nov 1, 2017

alexcrichton commented Nov 4, 2017

eddyb commented Nov 4, 2017

rustc_typeck: use subtyping on the LHS of binops. #45435

rustc_typeck: use subtyping on the LHS of binops. #45435

Conversation

eddyb commented Oct 21, 2017

eddyb commented Oct 22, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Oct 28, 2017

nikomatsakis commented Oct 31, 2017

nikomatsakis commented Oct 31, 2017

bors commented Oct 31, 2017

bors commented Nov 1, 2017

bors commented Nov 1, 2017

alexcrichton commented Nov 4, 2017

eddyb commented Nov 4, 2017