Decompose Adjustment into smaller steps and remove the method map. #42281

eddyb · 2017-05-28T13:48:28Z

The method map held method callee information for:

actual method calls (x.f(...))
overloaded unary, binary, indexing and call operators
every overloaded deref adjustment (many can exist for each expression)

That last one was a historical ~~accident~~ hack, and part of the motivation for this PR, along with:

a desire to compose adjustments more freely
containing the autoderef logic better to avoid mutation within an inference snapshot
not creating TyFnDef types which are incompatible with the original one
- i.e. we used to take aTyFnDef's for<'a> &'a T -> &'a U signature and instantiate 'a using a region inference variable, then package the resulting &'b T -> &'b U signature in another TyFnDef, while keeping the same DefId and Substs
to fix port the "autoref-arg" code used in overloaded operators to use the ptr adjustment table #3548 by explicitly writing autorefs for the RHS of comparison operators

Individual commits tell their own story, of "atomic" changes avoiding breaking semantics.

Future work based on this PR could include:

removing the signature from TyFnDef, now that it's always "canonical"
- some questions of variance remain, as subtyping still treats the signature differently
moving part of the typeck logic for methods, autoderef and coercion into rustc::traits
allowing LUB coercions (joining multiple expressions) to "stack up" many adjustments
transitive coercions (e.g. reify or unsize after multiple steps of autoderef)

r? @nikomatsakis

eddyb · 2017-05-28T16:16:07Z

Started crater run.

eddyb · 2017-05-28T17:37:27Z

The current Travis failures are in UI tests due to error output changed (intentionally), e.g.:

 error[E0277]: the trait bound `{integer}: std::ops::Mul<()>` is not satisfied
-  --> $DIR/binops.rs:14:5
+  --> $DIR/binops.rs:14:7
    |
 14 |     3 * ();
-   |     ^^^^^^ no implementation for `{integer} * ()`
+   |       ^ no implementation for `{integer} * ()`
    |
    = help: the trait `std::ops::Mul<()>` is not implemented for `{integer}`

cc @rust-lang/docs @nikomatsakis @jonathandturner Is this is an improvement?

GuillaumeGomez · 2017-05-28T17:58:13Z

@eddyb: I don't think it is. It should still underline the whole expression.

Mark-Simulacrum · 2017-05-28T18:31:59Z

I personally consider it an improvement, as it points at the thing that isn't implemented. The operands aren't really part of the problem; making the span as small as possible seems better.

eddyb · 2017-05-28T20:48:11Z

Crater report is clean (modulo the usual network errors and lint plugins).

QuietMisdreavus · 2017-05-28T20:59:49Z

I'm with @Mark-Simulacrum here. The error text still says which specific kind of multiplication isn't implemented, and pointing directly at the operator can save some confusion if it were part of a larger/more complicated expression.

sophiajt · 2017-05-29T01:46:31Z

@eddyb - yes, pointing at the offending operator looks like an improvement to me. As @QuietMisdreavus points out, this becomes more important with larger expressions, but even small ones seem to be a little clearer

eddyb · 2017-05-29T11:59:37Z

Thanks everyone! I pushed an update to the UI tests.

bors · 2017-05-30T00:19:18Z

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

nikomatsakis

a few nits, but r=me. Very cool!

nikomatsakis · 2017-05-30T22:50:33Z

src/librustc/ty/adjustment.rs

+/// Represents coercing a value to a different type of value.
+///
+/// We transform values by following the following steps in order:
+/// 1. Apply a step of `Adjust` (see its variants for details).


this should say "apply the base transformation described by kind", right?

What do you mean by "base"? But yeah I could use kind and then refer to "variants of Adjust".

Oh look at the latest version of this comment, that was transient.

nikomatsakis · 2017-05-30T22:55:51Z

src/librustc/ty/context.rs

            .map_or_else(|| self.expr_ty(expr), |adj| adj.target)
    }

    pub fn expr_ty_adjusted_opt(&self, expr: &hir::Expr) -> Option<Ty<'tcx>> {
-        self.adjustments.get(&expr.id)
+        self.expr_adjustments(expr).last()


Nit: I'd find this easier to read if each step in the chain (i.e., .last(), .map(), and .or_else() were on their own line). Or at least .map() and .or_else().

nikomatsakis · 2017-05-30T22:58:26Z

src/libcollections/tests/binary_heap.rs

@@ -134,22 +134,22 @@ fn test_push() {
 fn test_push_unique() {
    let mut heap = BinaryHeap::<Box<_>>::from(vec![box 2, box 4, box 9]);
    assert_eq!(heap.len(), 3);
-    assert!(*heap.peek().unwrap() == box 9);
+    assert!(**heap.peek().unwrap() == 9);


what's happening here? some lints firing?

Yupp! Comparison operators are now understood by the "unnecessary allocation" lint.

nikomatsakis · 2017-05-30T22:59:09Z

src/test/ui/impl-trait/equality.stderr

   |
 34 |         n + sum_to(n - 1)
-   |         ^^^^^^^^^^^^^^^^^ no implementation for `u32 + impl Foo`
+   |           ^ no implementation for `u32 + impl Foo`


nikomatsakis · 2017-05-30T23:00:25Z

Regarding the new spans -- yes, I consider it an improvement. I believe this is what clang does. Part of the reason I think this is better: mutil-line spans are terrible, and this avoids that.

nikomatsakis · 2017-05-30T23:00:53Z

some questions of variance remain, as subtyping still treats the signature differently

I was wondering what happened to those changes....

eddyb · 2017-05-30T23:17:34Z

@nikomatsakis I actually rebased those on top of this branch today, but tried to keep this PR separate.

eddyb · 2017-05-31T08:18:35Z

@bors r=nikomatsakis

bors · 2017-05-31T08:18:36Z

📌 Commit e3b49c5 has been approved by nikomatsakis

bors · 2017-05-31T23:54:38Z

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

…orefs.

eddyb · 2017-06-01T06:00:21Z

@bors r=nikomatsakis

bors · 2017-06-01T06:00:22Z

📌 Commit 5fb37be has been approved by nikomatsakis

bors · 2017-06-01T11:34:19Z

⌛ Testing commit 5fb37be with merge 4ed2eda...

@nikomatsakis

Decompose Adjustment into smaller steps and remove the method map. The method map held method callee information for: * actual method calls (`x.f(...)`) * overloaded unary, binary, indexing and call operators * *every overloaded deref adjustment* (many can exist for each expression) That last one was a historical ~~accident~~ hack, and part of the motivation for this PR, along with: * a desire to compose adjustments more freely * containing the autoderef logic better to avoid mutation within an inference snapshot * not creating `TyFnDef` types which are incompatible with the original one * i.e. we used to take a`TyFnDef`'s `for<'a> &'a T -> &'a U` signature and instantiate `'a` using a region inference variable, *then* package the resulting `&'b T -> &'b U` signature in another `TyFnDef`, while keeping *the same* `DefId` and `Substs` * to fix #3548 by explicitly writing autorefs for the RHS of comparison operators Individual commits tell their own story, of "atomic" changes avoiding breaking semantics. Future work based on this PR could include: * removing the signature from `TyFnDef`, now that it's always "canonical" * some questions of variance remain, as subtyping *still* treats the signature differently * moving part of the typeck logic for methods, autoderef and coercion into `rustc::traits` * allowing LUB coercions (joining multiple expressions) to "stack up" many adjustments * transitive coercions (e.g. reify or unsize after multiple steps of autoderef) r? @nikomatsakis

bors · 2017-06-01T14:03:14Z

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

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 28, 2017

eddyb requested a review from nikomatsakis May 28, 2017 13:48

rust-highfive assigned nikomatsakis May 28, 2017

eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 28, 2017

eddyb force-pushed the well-adjusted branch from 3564169 to 635a469 Compare May 29, 2017 11:56

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2017

eddyb force-pushed the well-adjusted branch from 635a469 to 187d11f Compare May 30, 2017 07:11

nikomatsakis requested changes May 30, 2017

View reviewed changes

eddyb force-pushed the well-adjusted branch from 187d11f to e3b49c5 Compare May 31, 2017 08:16

eddyb added 6 commits June 1, 2017 08:59

rustc: remove Copy from Adjustment and Adjust.

c6651ff

rustc: remove unused field of mc::Categorization::Deref.

9a53c3e

rustc: keep overloaded autoderef MethodCallee's in Adjust.

b4988f0

rustc: replace TyFnDef in MethodCallee with just the FnSig.

22510f3

rustc: avoid using MethodCallee's signature where possible.

a65ced5

rustc: remove unnecessary ItemSubsts wrapper.

9eae6ba

eddyb added 8 commits June 1, 2017 08:59

rustc: replace method_map with Def::Method and node_substs entries.

552ab37

rustc: remove unused TypeFolder methods.

c0e8fff

rustc: replace autoderefs' use of MethodCallee with OverloadedDeref.

4a754f2

rustc: move autoref and unsize from Adjust::DerefRef to Adjustment.

91d603a

rustc: decompose Adjustment into a vector of adjustment steps.

194fe69

rustc: adjust the RHS of comparison operators instead of assuming aut…

3ce4438

…orefs.

tests: fix fallout from empowering unused_allocation in comparisons.

58632f3

tests: fix fallout from changing the span of binop errors.

5fb37be

eddyb force-pushed the well-adjusted branch from e3b49c5 to 5fb37be Compare June 1, 2017 05:59

bors merged commit 5fb37be into rust-lang:master Jun 1, 2017

eddyb mentioned this pull request Jun 1, 2017

Check types for privacy #42125

Merged

eddyb deleted the well-adjusted branch June 1, 2017 15:13

messense mentioned this pull request Jun 2, 2017

Compilation error on rustc 1.19.0-nightly (4ed2edaaf 2017-06-01) rust-lang/rust-clippy#1807

Closed

Mark-Simulacrum mentioned this pull request Jun 6, 2017

Rust 1.19 regression, scopeguard, ICE #42463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decompose Adjustment into smaller steps and remove the method map. #42281

Decompose Adjustment into smaller steps and remove the method map. #42281

eddyb commented May 28, 2017 •

edited

Loading

eddyb commented May 28, 2017

eddyb commented May 28, 2017

GuillaumeGomez commented May 28, 2017

Mark-Simulacrum commented May 28, 2017

eddyb commented May 28, 2017

QuietMisdreavus commented May 28, 2017

sophiajt commented May 29, 2017

eddyb commented May 29, 2017

bors commented May 30, 2017

nikomatsakis left a comment

nikomatsakis May 30, 2017

eddyb May 30, 2017

eddyb May 31, 2017

nikomatsakis May 30, 2017

nikomatsakis May 30, 2017

eddyb May 30, 2017

nikomatsakis May 30, 2017

nikomatsakis commented May 30, 2017

nikomatsakis commented May 30, 2017 •

edited

Loading

eddyb commented May 30, 2017

eddyb commented May 31, 2017

bors commented May 31, 2017

bors commented May 31, 2017

eddyb commented Jun 1, 2017

bors commented Jun 1, 2017

bors commented Jun 1, 2017

bors commented Jun 1, 2017

Decompose Adjustment into smaller steps and remove the method map. #42281

Decompose Adjustment into smaller steps and remove the method map. #42281

Conversation

eddyb commented May 28, 2017 • edited Loading

eddyb commented May 28, 2017

eddyb commented May 28, 2017

GuillaumeGomez commented May 28, 2017

Mark-Simulacrum commented May 28, 2017

eddyb commented May 28, 2017

QuietMisdreavus commented May 28, 2017

sophiajt commented May 29, 2017

eddyb commented May 29, 2017

bors commented May 30, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis May 30, 2017

Choose a reason for hiding this comment

eddyb May 30, 2017

Choose a reason for hiding this comment

eddyb May 31, 2017

Choose a reason for hiding this comment

nikomatsakis May 30, 2017

Choose a reason for hiding this comment

nikomatsakis May 30, 2017

Choose a reason for hiding this comment

eddyb May 30, 2017

Choose a reason for hiding this comment

nikomatsakis May 30, 2017

Choose a reason for hiding this comment

nikomatsakis commented May 30, 2017

nikomatsakis commented May 30, 2017 • edited Loading

eddyb commented May 30, 2017

eddyb commented May 31, 2017

bors commented May 31, 2017

bors commented May 31, 2017

eddyb commented Jun 1, 2017

bors commented Jun 1, 2017

bors commented Jun 1, 2017

bors commented Jun 1, 2017

eddyb commented May 28, 2017 •

edited

Loading

nikomatsakis commented May 30, 2017 •

edited

Loading