Use RangeInclusive in SpanData instead of lo/hi

[PrestonFrom]

trafficstars

Proposal

Span can be confusing since it requires the user to understand how to use lo and hi correctly. Instead of having discrete fields, it might be more intuitive to use RangeInclusive.

This would be a change with a big footprint, so I think it would look like:

1. Provide a function to return a range and to convert from a range to SpanData
2. Replace uses of lo/hi fields and functions with the range function
3. Change internal representation from lo + hi to a range

A different order might make sense as well:

1. Change internal representation and update direct uses of fields to use start/end
2. Update all function usages of lo()/hi() with start()/end() I think this would elide the need for conversions to/from RangeInclusive, but would probably require a larger first commit.

Mentors or Reviewers

@oli-obk (Reviewer -- this was originally Oli's idea!)

Process

The main points of the Major Change Process are as follows:

• [x] File an issue describing the proposal.
• [ ] A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
  • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
  • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
• [ ] Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[rustbot]

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

[PrestonFrom]

@oli-obk Thank you for suggesting this! Please let me know if I have gotten anything wrong.

[llogiq]

Concern: I seem to remember that spans could be zero-sized. A RangeInclusive on the other hand cannot be empty.

So why should it be inclusive?

[compiler-errors]

Yes @llogiq, for example all spans from Span::shrink_to_lo() are zero width

[compiler-errors]

Also worth noting is that the Span struct doesn't actually store its innards as lo: BytePos, hi: BytePos. It stores it in a packed representation (I think?) and only SpanData stores lo, hi as individual fields. It may be useful to think if we should just modify the API that Span exposes to work with ranges, instead of modifying any internal representation.

On the other hand, I'm not certain if there are more usages that would benefit from having a range-based API or ones that use .with_lo/.with_hi explicitly because they're only ever really adjusting one end of the span at a time.

[PrestonFrom]

@compiler-errors @llogiq thank you for sharing your insight! Would you mind posting them in the Zulip stream as well?

https://rust-lang.zulipchat.com/#narrow/stream/233931-xxx/topic/Use.20RangeInclusive.20in.20SpanData.20instead.20of.E2.80.A6.20compiler-team.23534

At the very least, it seems like RangeInclusive is incorrect.

[apiraino]

Closing MCP (reason: probably not useful to have this)