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

Trait reform #5527

Closed
3 of 5 tasks
nikomatsakis opened this issue Mar 24, 2013 · 17 comments
Closed
3 of 5 tasks

Trait reform #5527

nikomatsakis opened this issue Mar 24, 2013 · 17 comments
Labels
A-traits Area: Trait system metabug Issues about issues themselves ("bugs about bugs")
Milestone

Comments

@nikomatsakis
Copy link
Contributor

CURRENT STATUS:

A lot of work has been done and there are only a few remaining tasks.

EXAMPLES:

Here are some issues that contain test cases that ought to work once this is done:

ORIGINAL TEXT:

This is an umbrella issue for collecting together issues with the current trait system. We need to figure out precisely what the solutions will be and discuss at the retreat, but it's at least useful to know what the problems are.

@nikomatsakis
Copy link
Contributor Author

One issue is that we assume in some parts of the code (but not all) that you can only implement a trait with one given set of type parameters (added FIXMEs).

@brendanzab
Copy link
Member

Associated items, as outlined in @nikomatsakis' post, would be very useful for #4819 (Bikeshed Numeric Traits). I'm in the process of drawing up a proposal for the numeric trait hierarchy which shows off how these could be used. I realize however that this might have to be post-1.0.

bors added a commit that referenced this issue Apr 25, 2013
As part of the numeric trait reform (see issue #4819), I have added the following traits to `core::num` and implemented them for floating point types:

~~~rust
pub trait Round {
    fn floor(&self) -> Self;
    fn ceil(&self) -> Self;
    fn round(&self) -> Self;
    fn trunc(&self) -> Self;
    fn fract(&self) -> Self;
}

pub trait Fractional: Num
                    + Ord
                    + Round
                    + Quot<Self,Self> {
    fn recip(&self) -> Self;
}

pub trait Real: Signed
              + Fractional {
    // Common Constants
    fn pi() -> Self;
    fn two_pi() -> Self;
    fn frac_pi_2() -> Self;
    fn frac_pi_3() -> Self;
    fn frac_pi_4() -> Self;
    fn frac_pi_6() -> Self;
    fn frac_pi_8() -> Self;
    fn frac_1_pi() -> Self;
    fn frac_2_pi() -> Self;
    fn frac_2_sqrtpi() -> Self;
    fn sqrt2() -> Self;
    fn frac_1_sqrt2() -> Self;
    fn e() -> Self;
    fn log2_e() -> Self;
    fn log10_e() -> Self;
    fn log_2() -> Self;
    fn log_10() -> Self;

    // Exponential functions
    fn pow(&self, n: Self) -> Self;
    fn exp(&self) -> Self;
    fn exp2(&self) -> Self;
    fn expm1(&self) -> Self;
    fn ldexp(&self, n: int) -> Self;
    fn log(&self) -> Self;
    fn log2(&self) -> Self;
    fn log10(&self) -> Self;
    fn log_radix(&self) -> Self;
    fn ilog_radix(&self) -> int;
    fn sqrt(&self) -> Self;
    fn rsqrt(&self) -> Self;
    fn cbrt(&self) -> Self;

    // Angular conversions
    fn to_degrees(&self) -> Self;
    fn to_radians(&self) -> Self;

    // Triganomic functions
    fn hypot(&self, other: Self) -> Self;
    fn sin(&self) -> Self;
    fn cos(&self) -> Self;
    fn tan(&self) -> Self;

    // Inverse triganomic functions
    fn asin(&self) -> Self;
    fn acos(&self) -> Self;
    fn atan(&self) -> Self;
    fn atan2(&self, other: Self) -> Self;

    // Hyperbolic triganomic functions
    fn sinh(&self) -> Self;
    fn cosh(&self) -> Self;
    fn tanh(&self) -> Self;
}

/// Methods that are harder to implement and not commonly used.
pub trait RealExt: Real {
    // Gamma functions
    fn lgamma(&self) -> (int, Self);
    fn tgamma(&self) -> Self;

    // Bessel functions
    fn j0(&self) -> Self;
    fn j1(&self) -> Self;
    fn jn(&self, n: int) -> Self;
    fn y0(&self) -> Self;
    fn y1(&self) -> Self;
    fn yn(&self, n: int) -> Self;
} 
~~~

The constants in `Real` could be [associated items](http://smallcultfollowing.com/babysteps/blog/2013/04/03/associated-items-continued/) in the future (see issue #5527). At the moment I have left the constants in `{float|f32|f64}::consts` in case folks need to access these at compile time. There are also instances of `int` in `Real` and `RealExt`. In the future these could be replaced with an associated `INTEGER` type on `Real`.

`Natural` has also been renamed to `Integer`. This is because `Natural` normally means 'positive integer' in mathematics. It is therefore strange to implement it on signed integer types. `Integer` is probably a better choice.

I have also switched some of the `Integer` methods to take borrowed pointers as arguments. This brings them in line with the `Quot` and `Rem` traits, and is be better for large Integer types like `BigInt` and `BigUint` because they don't need to be copied unnecessarily.

There has also been considerable discussion on the mailing list and IRC about the renaming of the `Div` and `Modulo` traits to `Quot` and `Rem`. Depending on the outcome of these discussions they might be renamed again.
@nikomatsakis
Copy link
Contributor Author

Nominated for backwards compat and/or well-defined

@nikomatsakis
Copy link
Contributor Author

We currently limit types from implementing the same trait more than once with distinct type parameters. I once proposed this limitation but have come to the conclusion that it is too limiting, and we should go more in the direction of haskell multiparameter type classes + associated types. There are various FIXMEs scattered throughout the code. Here is an example (from bjz) that fails because of this: https://gist.github.com/bjz/fbf54b214b36aeed471e

@graydon
Copy link
Contributor

graydon commented May 16, 2013

accepted for well-defined milestone

@catamorphism
Copy link
Contributor

Bug triage; it's all good.

@catamorphism
Copy link
Contributor

De-milestoning because it's a metabug and its sub-bugs are milestoned.

@nikomatsakis
Copy link
Contributor Author

Nominating -- in particular there are some open questions.

@pcwalton
Copy link
Contributor

Nominating to remove from blocker list because this is a metabug. If the scope of this bug is "everything that's weird with traits" then we'll never hit 1.0. Firefox has a policy of not blocking on metabugs for this reason too.

@nikomatsakis
Copy link
Contributor Author

I don't consider that the scope of the bug exactly. Let me write up a concrete proposal.

@erickt
Copy link
Contributor

erickt commented Nov 25, 2013

@nikomatsakis said this example falls under this metabug. I tried to add this implementation to Iterator:

impl<'self, A, T: Iterator<T>> Iterator<T> for &'self mut T {
    fn next(&mut self) -> Option<A> {
        (*self).next()
    }

    fn size_hint(&self) -> (uint, Option<uint>) {
        (*self).size_hint()
    }
}

But it hit an odd conflict within vec.rs, where rustc is now finding Iterator::len (which vecs do not directly implement) is conflicting with Container::len (which vecs do implement):

/Users/erickt/rust/rust/src/libstd/vec.rs:1433:29: 1433:41 error: failed to find an implementation of trait iter::Iterator<~[T]> for ~[T]
/Users/erickt/rust/rust/src/libstd/vec.rs:1433         if self.capacity() - self.len() < n {
                                                                            ^~~~~~~~~~~~

bors added a commit that referenced this issue Jun 13, 2014
… r=pcwalton

The current setup is to have a single vector of type parameters in
scope at any one time. We then have to concatenate the parameters from
the impl/trait with those of the method. This makes a lot of things
awkward, most notably associated fns ("static fns"). This branch
restructures the substitutions into three distinct namespaces (type,
self, fn). This makes most of the "type parameter management"
trivial. This also sets us up to support UFCS (though I haven't made
any particular changes in that direction in this patch).

Along the way, this patch fixes a few miscellaneous bits of code cleanup:

1. Patch resolve to detect references to out-of-scope type parameters,
   rather than checking for "out of bound" indices during substitution
   (fixes #14603).

2. Move def out of libsyntax into librustc where it belongs. I should have
   moved DefId too, but didn't.
   
3. Permit homogeneous tuples like `(T, T, T)` to be used as fixed-length
   vectors like `[T, ..3]`. This is awfully handy, though public facing.
   I suppose it requires an RFC.

4. Add some missing tests.

cc #5527

r? @pcwalton or @pnkfelix
bors added a commit that referenced this issue Jun 24, 2014
…=pnkfelix

This is just a cleanup of the code. Doesn't really change anything deep about the way we operate. This is a prelude to implementing a good solution for one-way matching for #5527.

r? @pnkfelix (we were just crawling about this code, after all)
@sinistersnare
Copy link
Contributor

If #14802 was closed in favor of this, can we mark this as I-nominated and B-RFC-approved as that issue was?

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Sep 10, 2014
…detecting them in resolve. This simplifies logic elsewhere in the compiler. cc rust-lang#5527
bors added a commit that referenced this issue Sep 11, 2014
Avoid ever constructing cyclic types in the first place, rather than detecting them in resolve. This simplifies logic elsewhere in the compiler, in particular on the trait reform branch.

r? @pnkfelix or @pcwalton 

cc #5527
bors added a commit that referenced this issue Sep 16, 2014
…sited, r=pcwalton

This patch does not make many functional changes, but does a lot of restructuring towards the goals of #5527. This is the biggest patch, basically, that should enable most of the other patches in a relatively straightforward way.

Major changes:

- Do not track impls through trans, instead recompute as needed.
- Isolate trait matching code into its own module, carefully structure to distinguish various phases (selection vs confirmation vs fulfillment)
- Consider where clauses in their more general form
- Integrate checking of builtin bounds into the  trait matching process, rather than doing it separately in kind.rs (important for opt-in builtin bounds)

What is not included:

- Where clauses are still not generalized. This should be a straightforward follow-up patch.
- Caching. I did not include much caching. I have plans for various kinds of caching we can do. Should be straightforward. Preliminary perf measurements suggested that this branch keeps compilation times roughly what they are.
- Method resolution. The initial algorithm I proposed for #5527 does not work as well as I hoped. I have a revised plan which is much more similar to what we do today.
- Deref vs deref-mut. The initial fix I had worked great for autoderef, but not for explicit deref. 
- Permitting blanket impls to overlap with specific impls. Initial plan to consider all nested obligations before considering an impl to match caused many compilation errors. We have a revised plan but it is not implemented here, should be a relatively straightforward extension.
bors added a commit that referenced this issue Sep 16, 2014
…sited, r=pcwalton

This patch does not make many functional changes, but does a lot of restructuring towards the goals of #5527. This is the biggest patch, basically, that should enable most of the other patches in a relatively straightforward way.

Major changes:

- Do not track impls through trans, instead recompute as needed.
- Isolate trait matching code into its own module, carefully structure to distinguish various phases (selection vs confirmation vs fulfillment)
- Consider where clauses in their more general form
- Integrate checking of builtin bounds into the  trait matching process, rather than doing it separately in kind.rs (important for opt-in builtin bounds)

What is not included:

- Where clauses are still not generalized. This should be a straightforward follow-up patch.
- Caching. I did not include much caching. I have plans for various kinds of caching we can do. Should be straightforward. Preliminary perf measurements suggested that this branch keeps compilation times roughly what they are.
- Method resolution. The initial algorithm I proposed for #5527 does not work as well as I hoped. I have a revised plan which is much more similar to what we do today.
- Deref vs deref-mut. The initial fix I had worked great for autoderef, but not for explicit deref. 
- Permitting blanket impls to overlap with specific impls. Initial plan to consider all nested obligations before considering an impl to match caused many compilation errors. We have a revised plan but it is not implemented here, should be a relatively straightforward extension.
bors added a commit that referenced this issue Oct 10, 2014
Implement multidispatch and conditional dispatch. Because we do not attempt to preserve crate concatenation, this is a backwards compatible change. This is not yet fully integrated into method dispatch, so "UFCS"-style wrappers must be used to take advantage of the new features (see the run-pass tests).

cc #17307 (multidispatch)
cc #5527 (trait reform -- conditional dispatch)

Because we no longer preserve crate concatenability, this deviates slightly from what was specified in the RFC. The motivation for this change is described in [this blog post](http://smallcultfollowing.com/babysteps/blog/2014/09/30/multi-and-conditional-dispatch-in-traits/). I will post an amendment to the RFC in due course but do not anticipate great controversy on this point -- particularly as the RFCs more important features (e.g., conditional dispatch) just don't work without the change.
@aturon
Copy link
Member

aturon commented Jan 8, 2015

Nominating for closure. I don't think there's value in this bug any more.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 8, 2015

closing; the related work is either done or has been filed as a polish/follow-up ticket

@pnkfelix pnkfelix closed this as completed Jan 8, 2015
oli-obk pushed a commit to oli-obk/rust that referenced this issue May 2, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#5408 (Downgrade match_bool to pedantic)
 - rust-lang#5505 (Avoid running cargo+internal lints when not enabled)
 - rust-lang#5516 (Add a note to the beta sections of release.md)
 - rust-lang#5517 (Deploy time travel)
 - rust-lang#5523 (Add lifetime test case for `new_ret_no_self`)

Failed merges:

r? @ghost

changelog: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system metabug Issues about issues themselves ("bugs about bugs")
Projects
None yet
Development

No branches or pull requests

9 participants