[sruggier]

Here's a minimal fix for the problem documented in #35.

It looks like there are other potential problems with intersect_range_impl, but addressing them fully is more complicated, and would be simpler if I could use let chains. That would require updating the edition to 2024, so I decided it would probably make sense to defer those changes into a separate PR or two.

[sruggier]

I've pushed the rest of the changes into the pr/edition-2024 and pr/rewrite-intersect_ranges_impl branches on my fork. Here's a permalinked comparison. It's best reviewed as separate commits, to avoid having to filter out noise from cargo fix and cargo fmt.

I can submit these in 3 separate PRs, merge them all into one, or something in between, as needed.

[the-mikedavis]

Updating to edition 2024 would raise the MSRV significantly and libraries should avoid doing that unless there's something we really need in a higher Rust version.

[sruggier]

Updating to edition 2024 would raise the MSRV significantly and libraries should avoid doing that unless there's something we really need in a higher Rust version.

I really enjoyed being able to use let chains, but it sounds like it probably wouldn't fall into your definition of need, and it bumps the required Rust version up to 1.88.0.

Edit: I've backported my changes to the existing MSRV, and merged them into the branch for this PR.

[sruggier]

@the-mikedavis would you mind offering updated feedback about this, when you have time?

[the-mikedavis]

This is a complicated section of the code and I have a lot of other review on my plate so I probably won't be able to give this proper feedback for a while. At a quick glance though the new code looks much more complicated. I will probably feel more comfortable making a fix in this area of the code myself so I would recommending not investing too much time into this fix.

I can reproduce the invalid ranges by reducing the example code down to:

/**
 * foo
 */

impl Foo {
    /**
     * foo
     */

    /// foo
}

This gives the following ranges before the parsing errors out:

// Layer 0 root
LayerData {
  language: Language(0),
  ranges: [0..4294967295],
  parent: None,
  ..
}
// Layer 1
LayerData {
  language: Language(96),
  ranges: [3..12, 34..51, 62..67],
  parent: Some(Layer(0)),
  ..
}
// Layer 3
LayerData {
  language: Language(97),
  ranges: [42..46, 63..51, 62..66],
  parent: Some(Layer(1)),
  ..
}

I haven't yet dug into why intersect_ranges pushes the bad range 63..51.

[sruggier]

I haven't yet dug into why intersect_ranges pushes the bad range 63..51.

My memory's getting a bit vague, by now, but the first commit in this PR explains the problem and supplies a three-line fix.

Basically, after an exclusion, the start of the child range gets advanced to the end of excluded_range (63). The next iteration of the loop tries to consume the next parent range, and calls push_range(start..parent_range.end), which evaluates to 63..51. Then it updates start to point at the beginning of the next parent range, which is 62, so it also accidentally restores part of the last exclusion.

There's at least one other bug in the existing intersect_ranges_impl function: the first line assumes that the start of range is the same as the start of the next parent range:

let mut start = range.start;

That would be more correct like this:

let mut start = max(range.start, parent_range.start);

Also, it looks like an invalid range would be pushed if the first excluded range returned by the iterator were to start before range.start.

At a quick glance though the new code looks much more complicated.

Yeah, I understand. I'd probably feel the same way if I were on the other side of a diff like this. It looked cleaner in the 2024 edition, with let chains, but either way, there's more code here than there was before.

When I started looking at the != comparisons and getting nervous about the possibility of other bugs, it felt less error-prone to split it into two separate functions (to calculate exclusion and then intersection) than to try to reason about both in one loop. I suspect the original author may have tried to keep everything in a single loop for performance reasons, but the compiler is going to optimize all of it down to similar code, either way.

I split them because I was hoping to address not only the bug itself, but also this underlying cause: code that sacrifices readability and robustness to optimize something that the compiler is going to optimize anyway.

I will probably feel more comfortable making a fix in this area of the code myself so I would recommending not investing too much time into this fix.

Noted.

[the-mikedavis]

Ok I finally got around to giving this a proper look. Sorry for the delay. I agree the new approach moving exclude_ranges out is easier to reason about and shouldn't be a regression for performance. (I think so but I didn't do any benchmarking.)

I made a few edits for adding unit & regression tests and a change about merging adjacent ranges. I think adjacent ranges are ok in practice but we might as well merge as aggressively as we can. I also reverted the #[non_exhaustive] attribute on Range since I want to avoid a breaking change with this. I think if we really wanted to disallow Range from being constructed with backwards points we would have ::new return an Option<Self> or Result<Self>. Since it's not a critical failure for the library I think it's fine to allow constructing Range manually

[the-mikedavis]

Thanks @sruggier !