Partial implementation for UFCS trait-less associated paths. #22172

eddyb · 2015-02-11T08:32:08Z

Adds <module::Type>::method support and makes module::Type::method a shorthand for it.
This is most of #16293, except that chaining multiple associated types is not yet supported.
It also fixes #22563 as impls are no longer treated as modules in resolve.

Unfortunately, this is still a [breaking-change]:

If you used a global path to a primitive type, i.e. ::bool, ::i32 etc. - that was a bug I had to fix.
Solution: remove the leading ::.
If you passed explicit impl-side type parameters to an inherent method, e.g.:

struct Foo<T>(T);
impl<A, B> Foo<(A, B)> {
    fn pair(a: A, b: B) -> Foo<(A, B)> { Foo((a, b)) }
}
Foo::<A, B>::pair(a, b)
// Now that is sugar for:
<Foo<A, B>>::pair(a, b)
// Which isn't valid because `Foo` has only one type parameter.
// Solution: replace with:
Foo::<(A, B)>::pair(a, b)
// And, if possible, remove the explicit type param entirely:
Foo::pair(a, b)

If you used the QPath-related AstBuilder methods @hugwijst added in Add QPath construction to ExtCtxt for UFCS support. #21943.
The methods still exist, but QPath was replaced by QSelf, with the actual path stored separately.
Solution: unpack the pair returned by cx.qpath to get the two arguments for cx.expr_qpath.

rust-highfive · 2015-02-11T08:32:21Z

r? @nikomatsakis

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

nikomatsakis · 2015-02-11T10:41:32Z

very cool, I will read.

nikomatsakis · 2015-02-13T10:41:49Z

@eddyb sorry for being slow. I spent some time reading into this two days back but yesterday got very busy. I expect to finish up today.

nikomatsakis · 2015-02-13T16:28:43Z

First off, some nits here: nikomatsakis@b6461f0

nikomatsakis · 2015-02-13T16:59:56Z

r+, awesome! But I did have some conditions and comments :)

Conditions:

Nits at nikomatsakis@b6461f0
More new tests! Surely there are new constructions to test here? Some thoughts:
- the fact that inherent takes precedence over trait
- distinction between Trait::method and <Trait>::method
- inherent impls are, I guess, extensively tested already, but there are probably some variations on them that you enable that didn't work right before, right?
- can you rename const-polymorphic-paths to something like ufcs-trait-relative-paths or just anything that begins with ufcs? I'm pretty sure I'd never find it with the current name, and (iiuc) it tests more than just consts.
A comment or two about PartialDef and what the heck depth means (in resolve_path and friends) would be nice.

Comments/Questions:

I still feel like a reworking of resolve and the data structures around resolve (notably, Path and Def) could yield substantially cleaner code, but this seems to be overall a pretty reasonable approach.

One thing I noticed that stuck out at me: in resolve, if parsing a::b::c::d, it seems that you first try to parse the entire path, then you drop segments from the RHS one by one (so next try a::b::c, then a::b, etc) until you find a hit. This seems remarkably inefficient. I am assuming that this was the easiest way to shoehorn this work on top of resolve -- is there a deeper reason for this approach?

I also found the "suppress errors" flag a little bit suboptimal, compared to passing back an error as a result, but it's a relatively minor thing. In general I'd like to see resolution refactored into a "side-effect-free" lookup that returns errors / successful results, leaving it to the caller to decide what to do with those.

I was happy to see you integrated the lookup into probe.

nikomatsakis · 2015-02-18T14:36:01Z

@eddyb ok, so I finished reviewing as of 43bac21. Let's call them concerns, for the most part -- more than nits, but certainly not blockers. Anyway, I'd appreciate clarification about the potential for ICEs etc. I'd like to have a change to look over the patch once you address them -- or tell me why I am wrong --- but promise to make it quick presuming major changes aren't needed.

eddyb · 2015-02-19T01:55:09Z

@bors r=nikomatsakis 7af58b8

bors · 2015-02-19T10:21:18Z

⌛ Testing commit 7af58b8 with merge de86968...

bors · 2015-02-19T10:54:07Z

💔 Test failed - auto-linux-64-x-android-t

bors · 2015-02-19T10:54:07Z

💔 Test failed - auto-mac-32-opt

eddyb · 2015-02-19T11:18:50Z

@bors r=nikomatsakis 6e58a42

bors · 2015-02-19T13:08:31Z

⌛ Testing commit 6e58a42 with merge bf9938f...

bors · 2015-02-19T14:01:33Z

💔 Test failed - auto-mac-64-nopt-t

eddyb · 2015-02-19T20:29:16Z

@bors r=nikomatsakis e979cf4

eddyb · 2015-02-24T12:19:55Z

@bors r=nikomatsakis 0c6d1f3

bors · 2015-02-24T12:38:06Z

⌛ Testing commit 0c6d1f3 with merge f33e2e2...

bors · 2015-02-24T14:45:23Z

💔 Test failed - auto-win-32-nopt-t

Manishearth · 2015-02-24T14:46:41Z

@bors: retry force

Manishearth · 2015-02-24T14:47:42Z

@bors: force

bors · 2015-02-24T14:51:04Z

⌛ Testing commit 0c6d1f3 with merge 695bde9...

bors · 2015-02-24T14:51:06Z

⌛ Testing commit 0c6d1f3 with merge 0b975cb...

bors · 2015-02-24T14:55:51Z

💔 Test failed - auto-win-32-nopt-t

Manishearth · 2015-02-24T15:25:13Z

@bors: retry

bors · 2015-02-24T17:56:10Z

⌛ Testing commit 0c6d1f3 with merge 0bd1565...

@hugwijst

Adds `<module::Type>::method` support and makes `module::Type::method` a shorthand for it. This is most of #16293, except that chaining multiple associated types is not yet supported. It also fixes #22563 as `impl`s are no longer treated as modules in resolve. Unfortunately, this is still a *[breaking-change]*: * If you used a global path to a primitive type, i.e. `::bool`, `::i32` etc. - that was a bug I had to fix. Solution: remove the leading `::`. * If you passed explicit `impl`-side type parameters to an inherent method, e.g.: ```rust struct Foo<T>(T); impl<A, B> Foo<(A, B)> { fn pair(a: A, b: B) -> Foo<(A, B)> { Foo((a, b)) } } Foo::<A, B>::pair(a, b) // Now that is sugar for: <Foo<A, B>>::pair(a, b) // Which isn't valid because `Foo` has only one type parameter. // Solution: replace with: Foo::<(A, B)>::pair(a, b) // And, if possible, remove the explicit type param entirely: Foo::pair(a, b) ``` * If you used the `QPath`-related `AstBuilder` methods @hugwijst added in #21943. The methods still exist, but `QPath` was replaced by `QSelf`, with the actual path stored separately. Solution: unpack the pair returned by `cx.qpath` to get the two arguments for `cx.expr_qpath`.

bors · 2015-02-24T20:48:50Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

Now possible thanks to rust-lang/rust#22172

rust-highfive assigned nikomatsakis Feb 11, 2015

eddyb force-pushed the almost-there branch from 6cb01ea to e76176b Compare February 11, 2015 19:01

eddyb force-pushed the almost-there branch from 71ac4be to 88fcf83 Compare February 13, 2015 10:53

eddyb mentioned this pull request Feb 16, 2015

Redefining built-in types does not work #20427

Closed

eddyb force-pushed the almost-there branch from 88fcf83 to 46cddac Compare February 17, 2015 10:26

eddyb changed the title ~~Desugar module::Type::method to <module::Type>::method.~~ Partial implementation for UFCS trait-less associated paths. Feb 17, 2015

eddyb force-pushed the almost-there branch 3 times, most recently from b7dd4cc to 43bac21 Compare February 17, 2015 17:29

This was referenced Feb 18, 2015

Permit T::Item based on bounds that appear in where clauses #22512

Merged

Resolve T::A-style associated types based solely on type, instead of a Def #22519

Open

eddyb force-pushed the almost-there branch 2 times, most recently from dc4043a to 7af58b8 Compare February 19, 2015 01:54

eddyb force-pushed the almost-there branch from 6e58a42 to e979cf4 Compare February 19, 2015 20:28

eddyb added 6 commits February 24, 2015 14:16

Fix fallout from correct stability handling in UFCS.

866a5ee

Fix fallout from allowing impls outside of the type's definition module.

9ac0736

Update trans/save's span hacks for fully qualified UFCS paths.

f0efa2d

rustc_typeck: unify the impl type with the UFCS path prefix type.

8501c9d

Fix fallout from rebasing.

72d5f39

syntax: update pretty-printer for the <T>::method shorthand.

0c6d1f3

eddyb force-pushed the almost-there branch from 4b59f96 to 0c6d1f3 Compare February 24, 2015 12:19

pnkfelix mentioned this pull request Feb 24, 2015

Avoid cat_expr Erred notes when already in error state. #22767

Merged

bors merged commit 0c6d1f3 into rust-lang:master Feb 24, 2015

eddyb deleted the almost-there branch February 24, 2015 20:52

pnkfelix mentioned this pull request Feb 24, 2015

Projecting <Type as Trait>::N should check that N is part of Trait #22731

Closed

SSheldon added a commit to SSheldon/rust-objc that referenced this pull request Feb 27, 2015

Removed lots of type annotations.

6069aed

Now possible thanks to rust-lang/rust#22172

SSheldon added a commit to SSheldon/rust-objc-foundation that referenced this pull request Mar 23, 2015

Removed lots of type annotations.

dec0d9c

Now possible thanks to rust-lang/rust#22172

SSheldon added a commit to SSheldon/rust-objc-id that referenced this pull request May 2, 2015

Removed lots of type annotations.

602d2d1

Now possible thanks to rust-lang/rust#22172

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial implementation for UFCS trait-less associated paths. #22172

Partial implementation for UFCS trait-less associated paths. #22172

eddyb commented Feb 11, 2015

rust-highfive commented Feb 11, 2015

nikomatsakis commented Feb 11, 2015

nikomatsakis commented Feb 13, 2015

nikomatsakis commented Feb 13, 2015

nikomatsakis commented Feb 13, 2015

nikomatsakis commented Feb 18, 2015

eddyb commented Feb 19, 2015

bors commented Feb 19, 2015

bors commented Feb 19, 2015

bors commented Feb 19, 2015

eddyb commented Feb 19, 2015

bors commented Feb 19, 2015

bors commented Feb 19, 2015

eddyb commented Feb 19, 2015

eddyb commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

Manishearth commented Feb 24, 2015

Manishearth commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

Manishearth commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

Partial implementation for UFCS trait-less associated paths. #22172

Partial implementation for UFCS trait-less associated paths. #22172

Conversation

eddyb commented Feb 11, 2015

rust-highfive commented Feb 11, 2015

nikomatsakis commented Feb 11, 2015

nikomatsakis commented Feb 13, 2015

nikomatsakis commented Feb 13, 2015

nikomatsakis commented Feb 13, 2015

nikomatsakis commented Feb 18, 2015

eddyb commented Feb 19, 2015

bors commented Feb 19, 2015

bors commented Feb 19, 2015

bors commented Feb 19, 2015

eddyb commented Feb 19, 2015

bors commented Feb 19, 2015

bors commented Feb 19, 2015

eddyb commented Feb 19, 2015

eddyb commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

Manishearth commented Feb 24, 2015

Manishearth commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

Manishearth commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015