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

If traits-as-types no longer implicitly passed-by-reference, then rustc should reject passing them by value #5088

Closed
pnkfelix opened this issue Feb 22, 2013 · 6 comments
Labels
A-traits Area: Trait system A-typesystem Area: The type system

Comments

@pnkfelix
Copy link
Member

�If traits no longer implicitly passed-by-reference, then rustc should reject passing traits by value.

My understanding from #3635 is that traits were once implicitly boxed with @, but now you must explicitly include an pointer-type annotation when you use a trait as an object type.

I illustrate some of the cases that arise in the code sample below; the comments indicate how I think each case should be handled.

The heart of this ticket is that I am suggesting that rustc be changed so that fromV below would be rejected, as it does not make sense to pass ByVal by-value, since e.g. it is a trait and therefore we do not know what the underlying object-size is. (Alternatively, rustc could treat it as an implicitly polymorphic function, analogous to fromT in the example. But I think that would be a mistake -- if the user wants polymorphism, they should the syntax for it.)

trait ByRef         { fn ref_to_int(&self) -> int; }

trait ByVal         { fn val_to_int(self) -> int; }

// Ideally this would carry a default implementation that DELegates,
// but default methods are experimental and signal error from rustc.
trait ByDel : ByRef { fn del_to_int(self) -> int; }

impl ByRef for int  { fn ref_to_int(&self) -> int { 2 } }

impl ByVal for int  { fn val_to_int( self) -> int { 3 } }

impl ByDel for int {  fn del_to_int(self) -> int { self.ref_to_int() } }

// One can do this (passing trait-by-reference == object type).  Good.
fn fromH(_a:@ByRef) -> int {
    if false { _a.ref_to_int() } else { 5 };
    5
}

// Probably should *not* be able even to declare this function
// (passing trait by value is senseless).  It does compile currently;
// presumably this is compiling into implicit reference.):
fn fromV(_a:ByVal) -> int {
    // if false { _a.val_to_int() } else { 6 }; // (leave in comment, senseless)
    6
}

// But one *should* be able to do this (passing concrete T by value is sane):
fn fromT<T:ByVal>(_a: T) -> int {
    // Unfortunately, if you uncomment this, you hit ICE #4406.
    // if false { _a.val_to_int() } else { 7 };
    7
}

// This is similar to fromT above; it is just meant to add a check of
// pass-self-by-value traits that extend object-compatible traits.
// Unfortunately, like fromT above, it hits same ICE, so cannot test.
fn fromD<T:ByDel>(_a: T) -> int {
    // Unfortunately, if you uncomment this, you hit ICE #4406.
    // if false { _a.del_to_int() } else { 8 };
    8
}

fn main() {
    io::println(fmt!("%?", fromH(@10 as @ByRef)));
    io::println(fmt!("%?", fromV(10 as ByVal)));
    io::println(fmt!("%?", fromT(10)));
    io::println(fmt!("%?", fromD(10)));
}

The ByDel example is my attempt to implement a suggestion that Niko gave me, where he was suggesting dividing a trait carrying both object-compatible and object-incompatible methods (see #5086) into two traits, where the object-incompatible trait would inherit from the object-compatible one. I see now that #5086 is closed; I do not yet know what that implies for the state of Niko's suggestion.

@pnkfelix
Copy link
Member Author

see also #4406

@pnkfelix
Copy link
Member Author

I guess we also need to reject polymorphic-instantiations like fromT::<ByDel>, since we cannot pass ByDel by-value any more than we can pass ByVal by-value. That scenario was not explicitly explored above.

@nikomatsakis
Copy link
Contributor

I'm not quite sure what you were going for here... it's not quite what I meant to suggest, anyhow. I meant to suggest that if you have:

trait Both {
    fn foo(&self);
    fn bar(self);
}

Then under the rules I proposed (and later retracted) in #5086, it would be illegal to have an object type @Both, because we can't compile handle the by-value bar() method since we don't know the type of the receiver. Under today's rules, it would be legal to have the trait type @Both but @Both is not considered to implement the trait Both.

Now, in either scenario, suppose I wanted to have a function like

fn do_something<B: Both>(b: &B) { ... b.foo() ... }

Here the point is that it only calls foo() and not bar(), so in principle it should be fine to use an object type for B.

In that case, I can refactor my trait Both as follows:

trait Foo { fn foo(&self); }
trait Both : Foo { fn bar(self); }

and I can rewrite do_something() as:

fn do_something<F: Foo>(f: &F) { ... f.foo() ... }

This will work fine.

Of course, as I proposed in #5087, I'd like to say that no object-type @T implements the trait T by default. But this kind of refactoring may still be relevant. After all, it is possible to write:

impl Foo for @Foo {
    fn foo(&self) { self.foo(); }
}

but it is not legal to write

impl Both for @Foo {
    fn foo(&self) { self.foo(); }
    fn bar(self) { self.bar(); } // illegal: cannot invoke by-val-self method on object
}

@pnkfelix
Copy link
Member Author

Just to clarify my motivation to Niko: ByRef in my code is meant to be analogous to your Foo, and ByDel is analogous to your Both ; that is, the trait ByDel is an extension of the ByRef trait.

(The rest of the code surrounding this was either

  1. me exploring the space of possible expressions, or
  2. my attempting to dig into a scenario for why one would ever want a by-value trait to extend an object-compatible trait. In particular, the scenario I was intending to illustrate is that the default implementation of ByDel would use the implementation provided by ByRef, by invoking it. But I could not test whether this works, due to the aformentioned ICE.)

@catamorphism
Copy link
Contributor

Seems non-critical for 0.7. Nominating for milestone 2 (well-defined).

@graydon
Copy link
Contributor

graydon commented May 2, 2013

trait-as-type with no sigil is an error; this should be fixed. declining nomination as we think it's fixed.

@graydon graydon closed this as completed May 2, 2013
bors added a commit to rust-lang-ci/rust that referenced this issue May 2, 2020
Switch to GitHub Actions - Part 2 - From within

This is a continuation of rust-lang#5071. This time from a branch inside the rust-lang/rust-clippy repo, not from my fork, since secrets are not available in PRs from forks.

Copying the description of rust-lang#5071 to here:

Closes rust-lang#4577

~~This is just an experiment. I don't think we have a consensus _if_ we should move away from travis/appveyor.~~ We have consensus: rust-lang/rust-clippy#5071 (comment)

~~GHA would let us run up to 20 concurrent jobs. Since we have 15 integration tests and 4 (linux, linux 32-bit, macos, windows) basic tests, we would be able to run everything concurrently.~~ The org has a limit of 60 jobs across the org, so we limit the matrix of the integration tests to 6 concurrent jobs.

~~Also IIUC we only have to build Clippy once for every initegration test and then only check the repos.~~ Nope, dependent jobs exist, but they won't keep the artifacts (not even the checkout).

TODO before merge:

- [x] Add `DEPLOY_KEY` secret to github repo
- [x] test deployment on test branch `gh-test`#
  - [x] Test normal deployment
  - [x] Test deployment no changes
  - [x] Test deployment of tag
- [x] talk with `@rust-lang/infra` for bors, `@rust-lang/infra` is good with the move (crater also uses GHA+bors)
- [x] ~~Get remark + clippy_dev check to work on pushes (https://github.community/t5/GitHub-Actions/~Builds-are-not-triggered-with-on-paths/m-p/44075; I contacted GH support about this) ~~That seems to start working again yesterday. Let's hope it keeps working.~~ Or not: df9be48. Now it works again: 723786a. I think we should wait, until this is reliable. It appears, that it doesn't work on force pushes (sometimes?): 5814142~~ We need to run the bors tests unconditionally anyway (47138d1) so it doesn't really matter.
- [x] ~~impl homu checks for GHA rust-lang/rust-clippy#5071 (comment) -- I prepared: flip1995/rust-central-station@f40230d. I'd suggest to first add GHA and keep the travis and appveyor checks for a few days and to remove them in a second pass. The bors dummy jobs are added in rust-lang/rust-clippy@1a83b7a and work as expected: rust-lang/rust-clippy#5088 (comment). I opened rust-lang/rust-central-station#578 See rust-lang/rust-clippy#5088 (comment)
- [x] ~Add GHA badge to Cargo.toml (blocked on rust-lang/crates.io # 1838)~ Added a FIXME in
2332b57
- [x] ~Maybe we should also wait until GHA supports yaml anchors. https://github.community/t5/GitHub-Actions/Support-for-YAML-anchors/td-p/30336/~ WIll probably not be implemented in the near future.
- [x] Add back travis + appveyor files for transition period (!)

changelog: none
bors added a commit to rust-lang-ci/rust that referenced this issue May 2, 2020
…shearth,llogiq,flip1995

I like to move it, move it

GHA now runs in the background for 6 days (rust-lang#5088)

Since then ~~15~~ 19 PRs were successfully merged and Travis+Appveyor agreed on the status in every case. ([GitHub PR search query](https://github.com/rust-lang/rust-clippy/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged+merged%3A%3E%3D2020-02-12T15%3A42%3A00+sort%3Aupdated-desc+NOT+%5Bgh-pages%5D+in%3Atitle))

Some PRs were:
- rust-lang#5163
- rust-lang#5170
- rust-lang#5168
- rust-lang#5173
- rust-lang#5171
- rust-lang#5156
- rust-lang#4809
- rust-lang#5177
- rust-lang#5182
- rust-lang#5183
- rust-lang#5184
- rust-lang#5185
- rust-lang#5186
- rust-lang#5181
- rust-lang#5189

Bug with GHA:
- When a rustc PR gets merged between the `integration_build` and the `integration` job, the `integration` job will fail. This happened once in rust-lang#5162, but not in the past 6 days. Even if it would happen every 4th PR we would save time, since splitting up the integration build and tests saves 5-7 minutes per run and a complete run takes 15-17 minutes
- Sometimes the MacOS build takes up to an hour to download the master toolchain. Until now, this happend 2 or 3 times and can be resolved by a `@bors r3try`+canceling the previous run (restarting single jobs is not supported yet)

## Before merging this, rust-lang/rust-central-station#578 has to get merged

This PR is for starting the discussion and to get consensus (@rust-lang/clippy) on a final move to GHA. If we're ready, I'll contact Pietro, to finalize the move.

changelog: Clippy completely runs on GHA now 🎉

---

BTW: The deployment already runs on GHA, instead of Travis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system A-typesystem Area: The type system
Projects
None yet
Development

No branches or pull requests

4 participants