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.

Sign up for GitHub

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

Remove problematic specialization from RangeInclusive #68835

Merged
merged 2 commits into from
Feb 10, 2020
+44 −69
Merged

Remove problematic specialization from RangeInclusive #68835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e115b6
Remove problematic specialization from RangeInclusive
CAD97 Feb 4, 2020
136008c
Disable failing codegen test
CAD97 Feb 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 12 additions & 22 deletions src/libcore/iter/range.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,16 +341,15 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {

#[inline]
fn next(&mut self) -> Option<A> {
self.compute_is_empty();
if self.is_empty.unwrap_or_default() {
if self.is_empty() {
return None;
}
let is_iterating = self.start < self.end;
self.is_empty = Some(!is_iterating);
Some(if is_iterating {
let n = self.start.add_one();
mem::replace(&mut self.start, n)
} else {
self.exhausted = true;
self.start.clone()
})
}
Expand All @@ -369,8 +368,7 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {

#[inline]
fn nth(&mut self, n: usize) -> Option<A> {
self.compute_is_empty();
if self.is_empty.unwrap_or_default() {
if self.is_empty() {
return None;
}

Expand All @@ -379,21 +377,20 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {

match plus_n.partial_cmp(&self.end) {
Some(Less) => {
self.is_empty = Some(false);
self.start = plus_n.add_one();
return Some(plus_n);
}
Some(Equal) => {
self.is_empty = Some(true);
self.start = plus_n.clone();
self.exhausted = true;
return Some(plus_n);
}
_ => {}
}
}

self.start = self.end.clone();
self.is_empty = Some(true);
self.exhausted = true;
None
}

Expand All @@ -404,8 +401,6 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
F: FnMut(B, Self::Item) -> R,
R: Try<Ok = B>,
{
self.compute_is_empty();

if self.is_empty() {
return Try::from_ok(init);
}
Expand All @@ -418,7 +413,7 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
accum = f(accum, n)?;
}

self.is_empty = Some(true);
self.exhausted = true;

if self.start == self.end {
accum = f(accum, self.start.clone())?;
Expand Down Expand Up @@ -447,24 +442,22 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
#[inline]
fn next_back(&mut self) -> Option<A> {
self.compute_is_empty();
if self.is_empty.unwrap_or_default() {
if self.is_empty() {
return None;
}
let is_iterating = self.start < self.end;
self.is_empty = Some(!is_iterating);
Some(if is_iterating {
let n = self.end.sub_one();
mem::replace(&mut self.end, n)
} else {
self.exhausted = true;
self.end.clone()
})
}

#[inline]
fn nth_back(&mut self, n: usize) -> Option<A> {
self.compute_is_empty();
if self.is_empty.unwrap_or_default() {
if self.is_empty() {
return None;
}

Expand All @@ -473,21 +466,20 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {

match minus_n.partial_cmp(&self.start) {
Some(Greater) => {
self.is_empty = Some(false);
self.end = minus_n.sub_one();
return Some(minus_n);
}
Some(Equal) => {
self.is_empty = Some(true);
self.end = minus_n.clone();
self.exhausted = true;
return Some(minus_n);
}
_ => {}
}
}

self.end = self.start.clone();
self.is_empty = Some(true);
self.exhausted = true;
None
}

Expand All @@ -498,8 +490,6 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
F: FnMut(B, Self::Item) -> R,
R: Try<Ok = B>,
{
self.compute_is_empty();

if self.is_empty() {
return Try::from_ok(init);
}
Expand All @@ -512,7 +502,7 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
accum = f(accum, n)?;
}

self.is_empty = Some(true);
self.exhausted = true;

if self.start == self.end {
accum = f(accum, self.start.clone())?;
Expand Down
45 changes: 14 additions & 31 deletions src/libcore/ops/range.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,24 +340,21 @@ pub struct RangeInclusive<Idx> {
// support that mode.
pub(crate) start: Idx,
pub(crate) end: Idx,
pub(crate) is_empty: Option<bool>,

// This field is:
// - `None` when next() or next_back() was never called
// - `Some(false)` when `start < end`
// - `Some(true)` when `end < start`
// - `Some(false)` when `start == end` and the range hasn't yet completed iteration
// - `Some(true)` when `start == end` and the range has completed iteration
// The field cannot be a simple `bool` because the `..=` constructor can
// accept non-PartialOrd types, also we want the constructor to be const.
// - `false` upon construction
// - `false` when iteration has yielded an element and the iterator is not exhausted
// - `true` when iteration has been used to exhaust the iterator
//
// This is required to support PartialEq and Hash without a PartialOrd bound or specialization.
pub(crate) exhausted: bool,
}

#[stable(feature = "inclusive_range", since = "1.26.0")]
impl<Idx: PartialEq> PartialEq for RangeInclusive<Idx> {
#[inline]
fn eq(&self, other: &Self) -> bool {
self.start == other.start
&& self.end == other.end
&& self.is_exhausted() == other.is_exhausted()
self.start == other.start && self.end == other.end && self.exhausted == other.exhausted
}
}

Expand All @@ -369,8 +366,7 @@ impl<Idx: Hash> Hash for RangeInclusive<Idx> {
fn hash<H: Hasher>(&self, state: &mut H) {
self.start.hash(state);
self.end.hash(state);
// Ideally we would hash `is_exhausted` here as well, but there's no
// way for us to call it.
self.exhausted.hash(state);
}
}

Expand All @@ -389,7 +385,7 @@ impl<Idx> RangeInclusive<Idx> {
#[rustc_promotable]
#[rustc_const_stable(feature = "const_range_new", since = "1.32.0")]
pub const fn new(start: Idx, end: Idx) -> Self {
Self { start, end, is_empty: None }
Self { start, end, exhausted: false }
}

/// Returns the lower bound of the range (inclusive).
Expand Down Expand Up @@ -465,18 +461,13 @@ impl<Idx: fmt::Debug> fmt::Debug for RangeInclusive<Idx> {
self.start.fmt(fmt)?;
write!(fmt, "..=")?;
self.end.fmt(fmt)?;
if self.exhausted {
write!(fmt, " (exhausted)")?;
}
Ok(())
}
}

impl<Idx: PartialEq<Idx>> RangeInclusive<Idx> {
// Returns true if this is a range that started non-empty, and was iterated
// to exhaustion.
fn is_exhausted(&self) -> bool {
Some(true) == self.is_empty && self.start == self.end
}
}

impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
/// Returns `true` if `item` is contained in the range.
///
Expand Down Expand Up @@ -544,15 +535,7 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
#[unstable(feature = "range_is_empty", reason = "recently added", issue = "48111")]
#[inline]
pub fn is_empty(&self) -> bool {
self.is_empty.unwrap_or_else(|| !(self.start <= self.end))
}

// If this range's `is_empty` is field is unknown (`None`), update it to be a concrete value.
#[inline]
pub(crate) fn compute_is_empty(&mut self) {
if self.is_empty.is_none() {
self.is_empty = Some(!(self.start <= self.end));
}
self.exhausted || !(self.start <= self.end)
}
}

Expand Down
34 changes: 18 additions & 16 deletions src/test/codegen/issue-45222.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,30 @@ pub fn check_foo2() -> u64 {
}

// Simplified example of #45222

fn triangle_inc(n: u64) -> u64 {
let mut count = 0;
for j in 0 ..= n {
count += j;
}
count
}

// CHECK-LABEL: @check_triangle_inc
#[no_mangle]
pub fn check_triangle_inc() -> u64 {
// CHECK: ret i64 5000050000
triangle_inc(100000)
}
//
// Temporarily disabled in #68835 to fix a soundness hole.
//
// fn triangle_inc(n: u64) -> u64 {
// let mut count = 0;
// for j in 0 ..= n {
// count += j;
// }
// count
// }
//
// // COMMENTEDCHECK-LABEL: @check_triangle_inc
// #[no_mangle]
// pub fn check_triangle_inc() -> u64 {
// // COMMENTEDCHECK: ret i64 5000050000
// triangle_inc(100000)
// }

// Demo in #48012

fn foo3r(n: u64) -> u64 {
let mut count = 0;
(0..n).for_each(|_| {
(0 ..= n).rev().for_each(|j| {
(0..=n).rev().for_each(|j| {
count += j;
})
});
Expand Down