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

Give SliceIndex impls a test suite of girth befitting the implementation #50010

Merged
merged 8 commits into from
May 11, 2018

Conversation

ExpHP
Copy link
Contributor

@ExpHP ExpHP commented Apr 16, 2018

So one day I was writing something in my codebase that basically amounted to impl SliceIndex for (Bound<usize>, Bound<usize>), and I said to myself:

Boy, gee, golly! I never realized bounds checking was so tricky!

At some point when I had around 60 lines of tests for it, I decided to go see how the standard library does it to see if I missed any edge cases. ...That's when I discovered that libcore only had about 40 lines of tests for slicing altogether, and none of them even used ..=.


This PR includes:

  • Literally the first appearance of the word get_unchecked_mut in any directory named test or tests.
  • Likewise the first appearance of get_mut used with any type of range argument in these directories.
  • Tests for the panics on overflow with ..=.
    • I wanted to test on [(); usize::MAX] as well but that takes linear time in debug mode </3
  • A horrible and ugly test-generating macro for the should_panic tests that increases the DRYness by a single order of magnitude (which IMO wasn't enough, but I didn't want to go any further and risk making the tests inaccessible to next guy).
  • Same stuff for str!
    • Actually, the existing str tests were pretty good. I just helped filled in the holes.
  • A fix for the bug it caught. (only one sadly)

@rust-highfive
Copy link
Collaborator

r? @bluss

(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 Apr 16, 2018
@ExpHP ExpHP changed the title Give IndexSlice a test suite of girth befitting its implementation (and fix a unicode validation bug) Give IndexSlice a test suite of girth befitting its implementation (and fix a UTF8 boundary check) Apr 16, 2018
@ExpHP ExpHP changed the title Give IndexSlice a test suite of girth befitting its implementation (and fix a UTF8 boundary check) Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check) Apr 16, 2018
@rust-highfive
Copy link
Collaborator

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.
Resolving deltas: 100% (605067/605067), completed with 4761 local objects.
---
[00:00:44] configure: rust.quiet-tests     := True
---
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:642: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:651: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:660: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:669: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:678: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:687: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:696: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:705: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:706: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:707: line longer than 100 chars
[00:04:46] some tidy checks failed
[00:04:46]
[00:04:46]
[00:04:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:46] expected success, got: exit code: 1
[00:04:46]
[00:04:46]
[00:04:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:46] Build completed unsuccessfully in 0:01:53
[00:04:46] Makefile:79: recipe for target 'tidy' failed
[00:04:46] make: *** [tidy] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:0258ba80:start=1523916453831324825,finish=1523916453837939675,duration=6614850
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:03250e15
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:03250e15:start=1523916453843528410,finish=1523916453849607489,duration=6079079
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1f619d66
$ dmesg | grep -i kill
[   10.250665] init: failsafe main process (1092) killed by TERM signal

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)

@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 18, 2018

On second thought I think I'm going to pull the bug fix into a separate PR to make it easier to backport to beta

@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 21, 2018

Note: After #50039 merges, this will need to be rebased/force-pushed (I couldn't cherry-pick the commit from this PR).

When I do so I will likely:

so any review might want to hold off until then.

bors added a commit that referenced this pull request Apr 21, 2018
smaller PR just to fix #50002

I pulled this out of #50010 to make it easier to backport to beta if necessary, considering that inclusive range syntax is stabilizing soon (?).

It fixes a bug in `<str>::index_mut` with `(..=end)` ranges (#50002), which prior to this fix was not only unusable but also UB in the cases where it "worked" (it gave improperly truncated UTF-8).

(not that I can imagine why anybody would *use* `<str>::index_mut`... but I'm not here to judge)
@bors
Copy link
Contributor

bors commented Apr 21, 2018

☔ The latest upstream changes (presumably #50039) made this pull request unmergeable. Please resolve the merge conflicts.

@pietroalbini pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2018
@pietroalbini
Copy link
Member

Ping from triage @ExpHP! #50039 was merged, this needs the rebase.

@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 30, 2018

Oops, sorry! I did something locally, but then started having second thoughts due to how uneven the PR makes this part of the test suite appear, and chickened out. I guess I should follow through and just get it back up here, to let others be the judge of that.

(also, my local branch currently has the cardinal sin of a later commit that broadly changes decisions made in an earlier commit, but it seemed too difficult to clean up due to intervening commits, so I'm afraid I might have to leave it like that. Blechh)

A previous PR fixed one method that was legitimately buggy;
this cleans up the rest to be less diverse, mirroring the
corresponding impls on [T] to the greatest extent possible
without introducing any unnecessary UTF-8 boundary checks at 0.
GitHub users: I think you can add ?w=1 to the url
for a vastly cleaner whitespace-ignoring diff
m*n lines of implementation deserves m*n lines of tests
@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 30, 2018

rebased, force-pushed, and put into correct chronological order using black magic.

Notes:

  • Be aware that the macro syntax introduced in ce66f5d is revised in later commits.
    • (Crap, I just noticed now that I actually could have squashed these easily, thanks to having removed some of the intervening commits at the last minute. Sorry)
  • I tried not to include any substantial changes to existing test cases in ce66f5d, 030aa9b, or f1d7b45 beyond the format (i.e. I tried to preserve their inputs and expected outputs/modes of failure).
  • The concern addressed by 02b3da1 is purely anticipatory/not backed by evidence

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2018
@shepmaster
Copy link
Member

Ping from triage, @bluss! Will you have time to review this in the near future?

@pietroalbini pietroalbini added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 10, 2018
@pietroalbini
Copy link
Member

Ping from triage! This PR needs a review, can @bluss or someone else from @rust-lang/libs review this?

@alexcrichton
Copy link
Member

Egad sorry for the delay @ExpHP! Tihs all looks great to me, thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented May 10, 2018

📌 Commit f1d7b45 has been approved by alexcrichton

@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-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2018
@ExpHP ExpHP changed the title Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check) Give SliceIndex impls a test suite of girth befitting the implementation May 10, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request May 10, 2018
Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)

So one day I was writing something in my codebase that basically amounted to `impl SliceIndex for (Bound<usize>, Bound<usize>)`, and I said to myself:

*Boy, gee, golly!  I never realized bounds checking was so tricky!*

At some point when I had around 60 lines of tests for it, I decided to go see how the standard library does it to see if I missed any edge cases. ...That's when I discovered that libcore only had about 40 lines of tests for slicing altogether, and none of them even used `..=`.

---

This PR includes:

* **Literally the first appearance of the word `get_unchecked_mut` in any directory named `test` or `tests`.**
* Likewise the first appearance of `get_mut` used with _any type of range argument_ in these directories.
* Tests for the panics on overflow with `..=`.
    * I wanted to test on `[(); usize::MAX]` as well but that takes linear time in debug mode </3
* A horrible and ugly test-generating macro for the `should_panic` tests that increases the DRYness by a single order of magnitude (which IMO wasn't enough, but I didn't want to go any further and risk making the tests inaccessible to next guy).
* Same stuff for str!
    * Actually, the existing `str` tests were pretty good. I just helped filled in the holes.
* [A fix for the bug it caught](rust-lang#50002).  (only one ~~sadly~~)
@bors
Copy link
Contributor

bors commented May 10, 2018

⌛ Testing commit f1d7b45 with merge 5f566f2ae20052cc0fdb384120ee56ab4ff32b0d...

@alexcrichton
Copy link
Member

@bors: retry r-

prioritizing rollup which includes this in it

I think the simple_big test may also have been the cause of https://travis-ci.org/rust-lang/rust/jobs/377407894, so I've ignored the test for Emscripten specifically in the rollup and we can see how that goes

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 10, 2018
@ExpHP
Copy link
Contributor Author

ExpHP commented May 10, 2018

Makes sense. The test was present before this PR, but my PR adds a to_owned() (hidden in the assert_range_eq! macro) that might be pushing it just over the limit.

bors added a commit that referenced this pull request May 10, 2018
Rollup of 18 pull requests

Successful merges:

 - #49423 (Extend tests for RFC1598 (GAT))
 - #50010 (Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check))
 - #50447 (Fix update-references for tests within subdirectories.)
 - #50514 (Pull in a wasm fix from LLVM upstream)
 - #50524 (Make DepGraph::previous_work_products immutable)
 - #50532 (Don't use Lock for heavily accessed CrateMetadata::cnum_map.)
 - #50538 ( Make CrateNum allocation more thread-safe. )
 - #50564 (Inline `Span` methods.)
 - #50565 (Use SmallVec for DepNodeIndex within dep_graph.)
 - #50569 (Allow for specifying a linker plugin for cross-language LTO)
 - #50572 (Clarify in the docs that `mul_add` is not always faster.)
 - #50574 (add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (#49022))
 - #50575 (std: Avoid `ptr::copy` if unnecessary in `vec::Drain`)
 - #50588 (Move "See also" disambiguation links for primitive types to top)
 - #50590 (Fix tuple struct field spans)
 - #50591 (Restore RawVec::reserve* documentation)
 - #50598 (Remove unnecessary mutable borrow and resizing in DepGraph::serialize)
 - #50606 (Retry when downloading the Docker cache.)

Failed merges:

 - #50161 (added missing implementation hint)
 - #50558 (Remove all reference to DepGraph::work_products)
@bors bors merged commit f1d7b45 into rust-lang:master May 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants