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

Add test checking that Index<T: ?Sized> works #59527

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

matklad
Copy link
Member

@matklad matklad commented Mar 29, 2019

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

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2019
@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Mar 29, 2019
@Centril
Copy link
Contributor

Centril commented Mar 29, 2019

So this was not the PR I expected.

My guess is that it was meant to be removed in #23601, but was overlooked.

Perhaps. But if so, this is to me a happy accident.

run crater with the bound removed to check if there are any regressions, and merge this, to be consistent with other operator traits

I don't mind running crater, but I would not want to remove the bound when unsized_locals isn't stable and because its a future-compat hazard. Consistency is not a sufficient justification for that imo even if you find zero regressions.

decide, in the light of by-value DSTs, that this is a feature rather than a bug, and add a test

We can add a test now and later remove it if we change our minds.

@Centril
Copy link
Contributor

Centril commented Mar 29, 2019

@bors try

@bors
Copy link
Contributor

bors commented Mar 29, 2019

⌛ Trying commit a6b130e with merge 47f4f94...

bors added a commit that referenced this pull request Mar 29, 2019
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
@bors
Copy link
Contributor

bors commented Mar 29, 2019

☀️ Try build successful - checks-travis
Build commit: 47f4f94

@Centril
Copy link
Contributor

Centril commented Mar 29, 2019

@craterbot run mode=check-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-59527 created and queued.
🤖 Automatically detected try build 47f4f94
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2019
@craterbot
Copy link
Collaborator

🚧 Experiment pr-59527 is now running on agent aws-3-tmp.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@sfackler
Copy link
Member

What concrete problem is this solving, exactly? It's not clear to me why we should invest crater/person time on removing this bound.

@Centril
Copy link
Contributor

Centril commented Mar 29, 2019

@matklad
Copy link
Member Author

matklad commented Mar 29, 2019

@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.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-59527 is completed!
📊 0 regressed and 0 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 30, 2019
@Dylan-DPC-zz
Copy link

ping from triage @joshtriplett any updates?

@joshtriplett
Copy link
Member

r? @Centril

Can you please review the crater results and evaluate this?

@rust-highfive rust-highfive assigned Centril and unassigned joshtriplett Apr 8, 2019
@Centril
Copy link
Contributor

Centril commented Apr 8, 2019

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 ?Sized here.
So imo let's treat this as a happy accident and add a test.

@matklad Can you add such a test? I'm happy to r+ the PR in that state.

@Centril Centril removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 8, 2019
@Centril Centril added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 8, 2019
@dtolnay
Copy link
Member

dtolnay commented Apr 8, 2019

Adding back ?Sized later if necessary is guaranteed entirely backward compatible right? Is there code that could be broken by that?

@Centril
Copy link
Contributor

Centril commented Apr 8, 2019

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.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 12, 2019

@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.

@pnkfelix
Copy link
Member

(If you leave off #![feature(unsized_locals)], you get an error at the trait definition site, that looks like this:

error[E0277]: the size for values of type `(dyn Trait + 'static)` cannot be known at compilation time
  --> src/main.rs:12:21
   |
12 |     fn index(&self, index: Trait) -> &i32 {
   |                     ^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn Trait + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: all local variables must have a statically known size
   = help: unsized locals are gated as an unstable feature

I'm assuming that the feedback one gets when the : ?Sized anti-bound removed isn't amazingly better than that. If it is much better than the above, that could be a reason to consider landing this PR as is.)

@Centril
Copy link
Contributor

Centril commented Apr 12, 2019

@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.

@matklad
Copy link
Member Author

matklad commented Apr 17, 2019

Added the test!

@@ -0,0 +1,21 @@
// `std::ops::Index` has an `: ?Sized` bound on the `Idx` type param. This is
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Contributor

@Centril Centril Apr 17, 2019

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

Copy link
Member Author

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

Copy link
Contributor

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

@matklad matklad force-pushed the sized-index branch 4 times, most recently from f9dd09b to 12e921f Compare April 17, 2019 10:36
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:030ad52c:start=1555497453158385892,finish=1555497453911678869,duration=753292977
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[01:16:55] failures:
[01:16:55] 
[01:16:55] ---- [run-pass] run-pass/unsized-locals/unsized-index.rs stdout ----
[01:16:55] 
[01:16:55] error: test compilation failed although it shouldn't!
[01:16:55] status: exit code: 1
[01:16:55] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/unsized-locals/unsized-index.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/unsized-locals/unsized-index/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/unsized-locals/unsized-index/auxiliary"
[01:16:55] ------------------------------------------
[01:16:55] 
[01:16:55] ------------------------------------------
[01:16:55] stderr:
[01:16:55] stderr:
[01:16:55] ------------------------------------------
[01:16:55] {"message":"method `index` is not a member of trait `ops::IndexMut`","code":{"code":"E0407","explanation":"\nA definition of a method not in the implemented trait was given in a trait\nimplementation.\n\nErroneous code example:\n\n```compile_fail,E0407\ntrait Foo {\n    fn a();\n}\n\nstruct Bar;\n\nimpl Foo for Bar {\n    fn a() {}\n    fn b() {} // error: method `b` is not a member of trait `Foo`\n}\n```\n\nPlease verify you didn't misspell the method name and you used the correct\ntrait. First example:\n\n```\ntrait Foo {\n    fn a();\n    fn b();\n}\n\nstruct Bar;\n\nimpl Foo for Bar {\n    fn a() {}\n    fn b() {} // ok!\n}\n```\n\nSecond example:\n\n```\ntrait Foo {\n    fn a();\n}\n\nstruct Bar;\n\nimpl Foo for Bar {\n    fn a() {}\n}\n\nimpl Bar {\n    fn b() {}\n}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/run-pass/unsized-locals/unsized-index.rs","byte_start":567,"byte_end":620,"line_start":18,"line_end":18,"column_start":5,"column_end":58,"is_primary":true,"text":[{"text":"    fn index(&self, _: str) -> &Self::Output { panic!() }","highlight_start":5,"highlight_end":58}],"label":"not a member of trait `ops::IndexMut`","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0407]: method `index` is not a member of trait `ops::IndexMut`\n  --> /checkout/src/test/run-pass/unsized-locals/unsized-index.rs:18:5\n   |\nLL |     fn index(&self, _: str) -> &Self::Output { panic!() }\n   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not a member of trait `ops::IndexMut`\n\n"}
[01:16:55] {"message":"not all trait items implemented, missing: `index_mut`","code":{"code":"E0046","explanation":"\nItems are missing in a trait implementation. Erroneous code example:\n\n```compile_fail,E0046\ntrait Foo {\n    fn foo();\n}\n\nstruct Bar;\n\nimpl Foo for Bar {}\n// error: not all trait items implemented, missing: `foo`\n```\n\nWhen trying to make some type implement a trait `Foo`, you must, at minimum,\nprovide implementations for all of `Foo`'s required methods (meaning the\nmethods that do not have default implementations), as well as any required\ntrait items like associated types or constants. Example:\n\n```\ntrait Foo {\n    fn foo();\n}\n\nstruct Bar;\n\nimpl Foo for Bar {\n    fn foo() {} // ok!\n}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/run-pass/unsized-locals/unsized-index.rs","byte_start":531,"byte_end":560,"line_start":17,"line_end":17,"column_start":1,"column_end":30,"is_primary":true,"text":[{"text":"impl ops::IndexMut<str> for A {","highlight_start":1,"highlight_end":30}],"label":"missing `index_mut` in implementation","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"`index_mut` from trait: `fn(&mut Self, Idx) -> &mut <Self as std::ops::Index<Idx>>::Output`","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0046]: not all trait items implemented, missing: `index_mut`\n  --> /checkout/src/test/run-pass/unsized-locals/unsized-index.rs:17:1\n   |\nLL | impl ops::IndexMut<str> for A {\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `index_mut` in implementation\n   |\n   = note: `index_mut` from trait: `fn(&mut Self, Idx) -> &mut <Self as std::ops::Index<Idx>>::Output`\n\n"}
[01:16:55] {"message":"aborting due to 2 previous errors","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 2 previous errors\n\n"}
[01:16:55] {"message":"Some errors occurred: E0046, E0407.","code":null,"level":"","spans":[],"children":[],"rendered":"Some errors occurred: E0046, E0407.\n"}
[01:16:55] 
[01:16:55] ------------------------------------------
[01:16:55] 
[01:16:55] 
---
[01:16:55] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:517:22
[01:16:55] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:16:55] 
[01:16:55] 
[01:16:55] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:16:55] 
[01:16:55] 
[01:16:55] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:16:55] Build completed unsuccessfully in 0:11:28
[01:16:55] Build completed unsuccessfully in 0:11:28
[01:16:55] make: *** [check] Error 1
[01:16:55] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0117c30a
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Apr 17 11:54:39 UTC 2019
---
travis_time:end:016dd705:start=1555502081392724865,finish=1555502081397698946,duration=4974081
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1ad25719
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:26b06c1d
travis_time:start:26b06c1d
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0429ef82
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@Centril
Copy link
Contributor

Centril commented Apr 17, 2019

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 17, 2019

📌 Commit cc3abc4 has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2019
@jethrogb
Copy link
Contributor

Please rename the PR?

@matklad matklad changed the title remove ?Sized bounds from Index Add test for ?Sized bound in ops::Index and ops::IndexMut Apr 17, 2019
@Centril Centril changed the title Add test for ?Sized bound in ops::Index and ops::IndexMut Add test checking that Index<T: ?Sized> works Apr 17, 2019
bors added a commit that referenced this pull request Apr 17, 2019
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
@bors
Copy link
Contributor

bors commented Apr 17, 2019

⌛ Testing commit cc3abc4 with merge 3c3d3c1...

@bors
Copy link
Contributor

bors commented Apr 17, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Centril
Pushing 3c3d3c1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 17, 2019
@bors bors merged commit cc3abc4 into rust-lang:master Apr 17, 2019
@matklad matklad deleted the sized-index branch July 9, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet