Fix the bug of next_point in source_map

[chenyukang]

There is a bug in next_point, the new span won't move to next position when be called in the first time.

For this reason, our current code is working like this:

1. When we really want to move to the next position, we called two times of next_point
2. Some code which use next_point actually done the same thing with shrink_to_hi

This fix make sure when next_point is called, span will move with the width at least 1, and also work correctly in the scenario of multiple bytes.

Ref: #103140 (comment)

r? @davidtwco

[davidtwco]

@bors r+

[bors]

📌 Commit 0af255a has been approved by davidtwco

It is now in the queue for this repository.

[ehuss]

I've always found this bit confusing, but I'm wondering if this comment is now out-of-date?

The next_point() function used to return an empty span (when the "width is 1" as in the comment) pointing just before the next character. This change seems to change it so that it is a non-empty span containing the next character.

I'm also wondering if the name of the function is also a bit misleading? The original next_point function always returned an empty span (a "point", not a region). This was changed in #47420 to have the somewhat confusing behavior where an empty span would move forward to span the next character (non-empty), and a non-empty span would return an empty span pointing just past the end. It's not clear from that PR why that behavior was chosen.

I also think this function is still somewhat buggy with respect to multi-byte characters. Below is a unittest which illustrates some oddities. Perhaps it might be good to add this unittest?

#[test]
fn next_point() {
    let sm = SourceMap::new(FilePathMapping::empty());
    sm.new_source_file(PathBuf::from("example.rs").into(), "a…b".to_string());
    let span = Span::with_root_ctxt(BytePos(0), BytePos(0));
    // This confusingly does not advance the span?
    let span = sm.next_point(span);
    assert_eq!(span.lo().0, 0);
    assert_eq!(span.hi().0, 0); // ERROR: This should probably be 1?

    let span = Span::with_root_ctxt(BytePos(0), BytePos(1));
    let span = sm.next_point(span);
    assert_eq!(span.lo().0, 1);
    assert_eq!(span.hi().0, 4);

    // This creates an invalid span, slicing in the middle of a multi-byte char?
    let span = Span::with_root_ctxt(BytePos(1), BytePos(1));
    let span = sm.next_point(span);
    assert_eq!(span.lo().0, 1);
    assert_eq!(span.hi().0, 2); // ERROR: This should probably be 4?

    let span = Span::with_root_ctxt(BytePos(1), BytePos(4));
    let span = sm.next_point(span);
    assert_eq!(span.lo().0, 4);
    assert_eq!(span.hi().0, 5);
    // This creates an invalid span, pointing past the end of the file?
    let span = sm.next_point(span);
    assert_eq!(span.lo().0, 5);
    assert_eq!(span.hi().0, 6); // ERROR: This should probably be 5?

    // Empty span pointing just past the last byte.
    let span = Span::with_root_ctxt(BytePos(5), BytePos(5));
    let span = sm.next_point(span);
    assert_eq!(span.lo().0, 5);
    assert_eq!(span.hi().0, 6); // ERROR: This should probably be 5?
}

[chenyukang]

This unit test implied another issue:

// This creates an invalid span, slicing in the middle of a multi-byte char?
    let span = Span::with_root_ctxt(BytePos(1), BytePos(1));
    let span = sm.next_point(span);
    assert_eq!(span.lo().0, 1);
    assert_eq!(span.hi().0, 2);

When lo and hi are same, we just reutrn the width with 1:

rust/compiler/rustc_span/src/source_map.rs

Line 877 in 194140b

if sp.lo == sp.hi {

So the result is span.hi().0 will be 2.

[chenyukang]

Thanks @ehuss,

I fixed all other issues in the test cases, except for this one:

 let span = sm.next_point(span);
    assert_eq!(span.lo().0, 0);
    assert_eq!(span.hi().0, 0); // ERROR: This should probably be 1?

It return the same span because it's a dummy span, something special for macros..

[matthiaskrgr]

@bors rollup=iffy might cause some clippy output change?
#103207 (comment)

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[chenyukang]

@bors rollup=iffy might cause some clippy output change? #103207 (comment)

Fixed by:

- let hi = cx.sess().source_map().next_point(remove_span).hi();
- let fmpos = cx.sess().source_map().lookup_byte_offset(hi);
+ let fmpos = cx.sess().source_map().lookup_byte_offset(remove_span.hi());

r? @davidtwco

[davidtwco]

@bors r-

[davidtwco]

Please avoid pushing to the pull request after it has been approved, or ping someone to unapprove it if you have to, otherwise it can get merged with only the earlier changes.

[davidtwco]

@bors r+

[bors]

📌 Commit 249b1d54a8c1c3238cb131433642a34dcc24ef39 has been approved by davidtwco

It is now in the queue for this repository.

[ehuss]

Also, FWIW, there is a dedicated test file for SourceMap in rustc_span/src/source_map/tests.rs, might be better to put this test there since it is primarily for SourceMap.

[davidtwco]

@bors r-

[chenyukang]

r? @davidtwco

[davidtwco]

@bors r+

[bors]

📌 Commit eb8aa97 has been approved by davidtwco

It is now in the queue for this repository.

[bors]

⌛ Testing commit eb8aa97 with merge 53728ff...

[bors]

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 53728ff to master...

[rust-timer]

Finished benchmarking commit (53728ff): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

1. the arithmetic mean of the percent change ↩

2. number of relevant changes ↩

[jruderman]

This may have introduced a compiler hang: #103451