Fix underflow in core::str::Searcher::new

[nham]

This incidentally fixes #16589, because it will cause MatchIndices to use NaiveSearcher instead of TwoWaySearcher, but I'm not sure #16589 should be closed until the underlying problem in TwoWaySearcher is found.

[alexcrichton]

Could you add a test for this as well?

[Gankra]

@alexcrichton this change only effects which internal algorithm is used to perform a search. If the underlying algorithms are correct (they're not), it should not effect the correctness, so in a sense this can't be tested.

[alexcrichton]

This states that #16589 is fixed, so couldn't a test for that be added?

[Gankra]

@alexcrichton sure, but it's more accurate to say the bug has been hidden by it.

"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxanana".contains("nana") should still fail.

[nham]

@alexcrichton I meant that while this technically resolves #16589, the real problem is that TwoWaySearcher is broken, as @gankro mentioned. I've gone ahead and added a test though.

[asterite]

In the D programming language every now and then someone comes to the newsgroup saying "I have this strangest bug, this is working wrong", only to get the answer "you are comparing a signed int with an unsigned one". This happens because array lengths are also unsigned ints there.

Be prepared to have these bugs and get similar questions from people using Rust. In my opinion, it makes sense that the length of a vector or an array is unsigned (it can't be negative), but if you make it a signed integer it isn't a big drawback and you will never have these kinds of bugs.

C#'s Array.length is signed: http://msdn.microsoft.com/en-us/library/system.array.length(v=vs.110).aspx

Java's Array length (and other types) are also ints (well, I think they don't have unsigned types).

I never heard anyone running into these bugs in Python, Ruby, Javascript, etc., either.

This program has an unexpected answer:

fn main() {
  let a = [1, 2, 3];
  println!("{}", a.len() > -1);
}

(and, as a side note, this program crashes the compiler:)

fn main() {
  println!("{}", [].len() > -1);
}

Please, consider using unsigned ints as least as possible. Thanks :-)

[alexcrichton]

@nham, tests such as this are normally added as unit tests, but the core tests are in the coretest library to get around bootstrapping concerns. Could you move the test to that library?

[nham]

@alexcrichton Done.

[pcwalton]

It is correct to use unsigned types for array lengths. Changing array lengths to be signed is the wrong solution. The right solution is to just warn against things like > -1.

[asterite]

But in this issue the line was this:

needle.len() > haystack.len() - 20

I don't think the compiler can warn against that...

[imbaczek]

shouldn't integer overflow (either signed or unsigned) be trapped by a safe language which rust aims to be?

[asterite]

@imbaczek I thought the same. But maybe Rust only aims to be a memory-safe language, not a non bug-prone language.

[thestinger]

@imbaczek: Rust aims to be competitive in performance with C and C++, and it wouldn't be if it branched on every integer operation. The complexity of the borrow checking and ownership don't make much sense if you have far bigger performance issues. It does expose intrinsics for efficient checked overflow and you could make checked types solely using those. In practice, not many people are going to use them. If you don't know the bounds it makes more sense to use a big integer.

[hatahet]

This was discussed on /r/rust, and it was pointed out that this still contains a bug if needle.len() > UINT_MAX - 20.

[Gankra]

@hatahet there is already a PR up for this issue: #16701