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

Relax str::get_unchecked precondition to permit empty slicing #69385

Merged
merged 1 commit into from Feb 24, 2020

Conversation

@ridiculousfish
Copy link
Contributor

ridiculousfish commented Feb 22, 2020

Prior to this commit, str documented that get_unchecked had
the precondition that "begin must come before end". This would appear
to prohibit empty slices (i.e. begin == end).

In practice, get_unchecked is called often with empty slices. Let's relax
the precondition so as to allow them.

Prior to this commit, `str` documented that `get_unchecked` had
the precondition that "`begin` must come before `end`". This would appear
to prohibit empty slices (i.e. begin == end).

In practice, get_unchecked is called often with empty slices. Let's relax
the precondition so as to allow them.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 22, 2020

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Feb 23, 2020

Surely get_unchecked is invalid on an empty slice, with any index? (Possibly modulo ZSTs, but even there I'm not sure).

It was always my understanding that get_unchecked is UB exactly when get would return None; but this sort of implies that's not correct?

Could you provide an example where this would be fine?

@ridiculousfish

This comment has been minimized.

Copy link
Contributor Author

ridiculousfish commented Feb 23, 2020

An example is:

unsafe { "abc".get_unchecked(0..0) }

I would expect this to work, despite violating the precondition that "the starting index must come before the ending index"

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Feb 23, 2020

            let ptr = slice.as_ptr().add(self.start);
            let len = self.end - self.start;
            super::from_utf8_unchecked(slice::from_raw_parts(ptr, len))

==>

            let ptr = "abc".as_ptr();
            let len = 0 - 0;
            super::from_utf8_unchecked(slice::from_raw_parts(ptr, len))

==>

            super::from_utf8_unchecked(&[])

==>

            ""
@ridiculousfish

This comment has been minimized.

Copy link
Contributor Author

ridiculousfish commented Feb 23, 2020

Yes, that is the current observed behavior. However it is undefined, because it violates the precondition.

My change adjusts the precondition to make this defined. Note it is purely a documentation change.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Feb 23, 2020

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

📌 Commit 9e41c4b has been approved by nagisa

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2020
… r=nagisa

Relax str::get_unchecked precondition to permit empty slicing

Prior to this commit, `str` documented that `get_unchecked` had
the precondition that "`begin` must come before `end`". This would appear
to prohibit empty slices (i.e. begin == end).

In practice, get_unchecked is called often with empty slices. Let's relax
the precondition so as to allow them.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2020
… r=nagisa

Relax str::get_unchecked precondition to permit empty slicing

Prior to this commit, `str` documented that `get_unchecked` had
the precondition that "`begin` must come before `end`". This would appear
to prohibit empty slices (i.e. begin == end).

In practice, get_unchecked is called often with empty slices. Let's relax
the precondition so as to allow them.
bors added a commit that referenced this pull request Feb 24, 2020
Rollup of 5 pull requests

Successful merges:

 - #69372 (Updates links in various Compiler Error Index entries)
 - #69385 (Relax str::get_unchecked precondition to permit empty slicing)
 - #69386 (Fix minor error in `MaybeUninit::get_mut()` doc example)
 - #69394 (Clean up E0367 explanation)
 - #69405 (docs: Stdin::read_line: mention the appending)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Feb 24, 2020
Rollup of 5 pull requests

Successful merges:

 - #69372 (Updates links in various Compiler Error Index entries)
 - #69385 (Relax str::get_unchecked precondition to permit empty slicing)
 - #69386 (Fix minor error in `MaybeUninit::get_mut()` doc example)
 - #69394 (Clean up E0367 explanation)
 - #69405 (docs: Stdin::read_line: mention the appending)

Failed merges:

r? @ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2020
…albini

Rollup of 5 pull requests

Successful merges:

 - rust-lang#69372 (Updates links in various Compiler Error Index entries)
 - rust-lang#69385 (Relax str::get_unchecked precondition to permit empty slicing)
 - rust-lang#69386 (Fix minor error in `MaybeUninit::get_mut()` doc example)
 - rust-lang#69394 (Clean up E0367 explanation)
 - rust-lang#69405 (docs: Stdin::read_line: mention the appending)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Feb 24, 2020
Rollup of 5 pull requests

Successful merges:

 - #69372 (Updates links in various Compiler Error Index entries)
 - #69385 (Relax str::get_unchecked precondition to permit empty slicing)
 - #69386 (Fix minor error in `MaybeUninit::get_mut()` doc example)
 - #69394 (Clean up E0367 explanation)
 - #69405 (docs: Stdin::read_line: mention the appending)

Failed merges:

r? @ghost
@bors bors merged commit d73aa67 into rust-lang:master Feb 24, 2020
4 checks passed
4 checks passed
pr Build #20200222.49 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Feb 25, 2020

(In hindsight, I missed that this was referring to a range index vs. a usize index).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.