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

remove the into_vec method from &[T] #17916

Closed
thestinger opened this issue Oct 10, 2014 · 11 comments
Closed

remove the into_vec method from &[T] #17916

thestinger opened this issue Oct 10, 2014 · 11 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@thestinger
Copy link
Contributor

The corresponding method is deprecated on Vec<T> and always does the same thing as to_vec on slices. There's no reason to keep this around.

@thestinger thestinger added A-libs E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Oct 10, 2014
@JIghtuse
Copy link
Contributor

Looks like to_vec is deprecated and needs to be removed, not into_vec. Am I right?

priority_queue.rs:

#[deprecated="renamed to `into_vec`"]
fn to_vec(self) -> Vec<T> { self.into_vec() }

I'll try to do this.

@aturon
Copy link
Member

aturon commented Oct 10, 2014

@JIghtuse I think you're looking at the wrong method (into_owned).

The into_vec method should be deprecated on CloneableVector, and Vec should just not implement CloneableVector.

@JIghtuse
Copy link
Contributor

Ah, I see. Thanks for the explanation, @aturon!

So, simply saying, I need to remove into_vec in CloneableVector and impl<T: Clone> CloneableVector<T> for Vec<T> in vec and then fix code to make sure all builds fine?

@aturon
Copy link
Member

aturon commented Oct 10, 2014

@JIghtuse Please leave into_vec in CloneableVector for now, but mark it with #[deprecated = "use to_vec instead"] (and a similar change in its doc comment). That way people have a chance to migrate their code.

(We can remove the impl in Vec because it's already been deprecated for a while.)

You'll need to write [breaking-change] on your commit message, which is how we track changes that break code (and currently, deprecation is included here as a courtesy.)

@JIghtuse
Copy link
Contributor

@aturon, ah, yep. I already see problems with removing into_vec now. At least libgraphviz will be broken. Thanks again!

@aturon
Copy link
Member

aturon commented Oct 10, 2014

@JIghtuse No problem! Once you mark it deprecated, you should go ahead and update uses within the rust distribution to use to_vec instead. The deprecation warnings should help you with that.

@JIghtuse
Copy link
Contributor

make check failed, but not because of my changes:

make[1]: Leaving directory '/home/jightuse/code/projects/rust/src/test/run-make/interdependent-c-libraries'

------ stderr ---------------------------------------------
bar.rs:13:1: 13:18 error: found staticlib `foo` instead of rlib `foo` , please compile using --crate-type rlib instead.
bar.rs:13 extern crate foo;
          ^~~~~~~~~~~~~~~~~
          error: aborting due to previous error

I don't know if change in librustc/middle/traits/select.rs was needed and if I've done it correctly, but without it rust didn't built.

@JIghtuse
Copy link
Contributor

Ah, it was my change to fix #14416. My fault. Rerun check now.

@BurntSushi
Copy link
Member

Using CloneableVector was useful to express input as, "I want a Vec of things. If you give me a Vec, I'll use that, otherwise, I'll clone your slice into a Vec." Strings have this with the StrAllocating trait.

Is there anything in the standard lib that replaces this for Vec<T>, or should I define my own trait?

@aturon
Copy link
Member

aturon commented Oct 15, 2014

@BurntSushi The Cow (clone-on-write) pointer in rust-lang/rfcs#235 will likely cover these use cases, but it hasn't been added to std yet. Please have a look and let me know whether you think that would work for you. (One advantage of the Cow approach is that it's generic across all types, including vectors and strings.)

For now, probably best to define your own trait.

Thanks for the heads-up about the use case!

@BurntSushi
Copy link
Member

@aturon I had read your collections reform RFC and I largely think it to be beautiful. :-) I believe you're right that Cow would help me. But yeah, I'll just go with my own trait for now. Thank you!

lnicola pushed a commit to lnicola/rust that referenced this issue Aug 29, 2024
fix: Wrong BoundVar index when lowering impl trait parameter of parent generics

Fixes rust-lang#17711

From the following test code;

```rust
//- minicore: deref
use core::ops::Deref;

struct Struct<'a, T>(&'a T);

trait Trait {}

impl<'a, T: Deref<Target = impl Trait>> Struct<'a, T> {
    fn foo(&self) -> &Self { self }

    fn bar(&self) {
        let _ = self.foo();
    }

}
```

when we call `register_obligations_for_call` for `let _ = self.foo();`,

https://github.com/rust-lang/rust-analyzer/blob/07659783fdfd4ec0a0bffa93017e33e31e567e42/crates/hir-ty/src/infer/expr.rs#L1939-L1952

we are querying `generic_predicates` and it has `T: Deref<Target = impl Trait>` predicate from the parent `impl Struct`;

https://github.com/rust-lang/rust-analyzer/blob/07659783fdfd4ec0a0bffa93017e33e31e567e42/crates/hir-ty/src/lower.rs#L375-L399

but as we can see above, lowering `TypeRef = impl Trait` doesn't take into account the parent generic parameters, so the `BoundVar` index here is `0`, as `fn foo` has no generic args other than parent's,

But this `BoundVar` is pointing at `'a` in `<'a, T: Deref<Target = impl Trait>>`.
So, in the first code reference `register_obligations_for_call`'s L:1948 - `.substitute(Interner, parameters)`, we are substituting `'a` with `Ty`, not `Lifetime` and this makes panic inside the chalk.

This PR fixes this wrong `BoundVar` index in such cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants