-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Add test checking that Index<T: ?Sized> works #59527
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
So this was not the PR I expected.
Perhaps. But if so, this is to me a happy accident.
I don't mind running crater, but I would not want to remove the bound when
We can add a test now and later remove it if we change our minds. |
@bors try |
remove ?Sized bounds from Index I've noticed that we have an `Idx: ?Sized` bound on the **index** in the `Index`, which seems strange given that we accept index by value. My guess is that it was meant to be removed in #23601, but was overlooked. If I remove this bound, `./x.py src/libstd/ src/libcore/` passes, which means at least that this is not covered by test. I think there's three things we can do here: * run crater with the bound removed to check if there are any regressions, and merge this, to be consistent with other operator traits * run crater, get regressions, write a test for this with a note that "hey, we tried to fix it, its unfixable" * decide, in the light of by-value DSTs, that this is a feature rather than a bug, and add a test cc @rust-lang/libs EDIT: the forth alternative is that there exist a genuine reason why this is the case, but I failed to see it :D
☀️ Try build successful - checks-travis |
@craterbot run mode=check-only |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
What concrete problem is this solving, exactly? It's not clear to me why we should invest crater/person time on removing this bound. |
See discussion in https://internals.rust-lang.org/t/why-ops-index-idx-allows-unsized-idx/9738. |
@sfackler I agree that this is basically a non-issue, except for the mere fact that we have a random untested bound in std’s public API. |
🎉 Experiment
|
ping from triage @joshtriplett any updates? |
r? @Centril Can you please review the crater results and evaluate this? |
The crater results are clean so nothing would break as far as I can see (assuming that crater is representative). However, I'm inclined to say that there is nothing notable to be gained by removing @matklad Can you add such a test? I'm happy to r+ the PR in that state. |
Adding back |
Not sure; it might have consequences wrt. implied bounds later on... but I would prefer not to take the risk since there's not much value in doing the change. |
@Centril to be clear, was the PR you were expecting something that added a test that looks along these lines (play): #![feature(unsized_locals)]
use std::ops::Index;
trait Trait {
fn value(&self) -> usize;
}
struct DrStrange(Vec<i32>);
impl Index<Trait> for DrStrange {
type Output = i32;
fn index(&self, index: Trait) -> &i32 {
&self.0[index.value()]
}
}
impl Trait for usize {
fn value(&self) -> usize { *self }
}
fn main() {
let stephen = DrStrange(vec![1, 2, 3]);
// ???? pnkfelix doesn't know offhand how to write
// an input to fill into `stephen[...]`.
} Update: Ah, @Centril had an example of how to use the index operator on the linked internals post. |
(If you leave off
I'm assuming that the feedback one gets when the |
@pnkfelix I would be happy with just: #![feature(unsized_locals)]
use std::ops::Index;
pub struct A;
impl Index<str> for A {
type Output = ();
fn index(&self, _: str) -> &Self::Output { panic!() }
}
fn main() {} In other words, testing the static semantics is good enough. |
Added the test! |
@@ -0,0 +1,21 @@ | |||
// `std::ops::Index` has an `: ?Sized` bound on the `Idx` type param. This is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the test to test/ui/...
and add // compile-pass
to the top of the file;
Otherwise, r=me with green travis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file ended up in /test/ui/...
instead of /src/test/ui/...
:D Aim for https://github.com/rust-lang/rust/tree/master/src/test/ui/unsized-locals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That explains why I had to use -f
to add it. Should be better now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah; the rebase went wrong tho :P
f9dd09b
to
12e921f
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r+ rollup |
📌 Commit cc3abc4 has been approved by |
Please rename the PR? |
Add test checking that Index<T: ?Sized> works I've noticed that we have an `Idx: ?Sized` bound on the **index** in the `Index`, which seems strange given that we accept index by value. My guess is that it was meant to be removed in #23601, but was overlooked. If I remove this bound, `./x.py src/libstd/ src/libcore/` passes, which means at least that this is not covered by test. I think there's three things we can do here: * run crater with the bound removed to check if there are any regressions, and merge this, to be consistent with other operator traits * run crater, get regressions, write a test for this with a note that "hey, we tried to fix it, its unfixable" * decide, in the light of by-value DSTs, that this is a feature rather than a bug, and add a test cc @rust-lang/libs EDIT: the forth alternative is that there exist a genuine reason why this is the case, but I failed to see it :D
☀️ Test successful - checks-travis, status-appveyor |
I've noticed that we have an
Idx: ?Sized
bound on the index in theIndex
, which seems strange given that we accept index by value. My guess is that it was meant to be removed in #23601, but was overlooked.If I remove this bound,
./x.py src/libstd/ src/libcore/
passes, which means at least that this is not covered by test.I think there's three things we can do here:
cc @rust-lang/libs
EDIT: the forth alternative is that there exist a genuine reason why this is the case, but I failed to see it :D