rustc_span: improve bounds checks in byte_pos_to_line_and_col

compiler/rustc_span/src/caching_source_map_view.rs

@@ -1,13 +1,25 @@
 use crate::source_map::SourceMap;
 use crate::{BytePos, SourceFile};
 use rustc_data_structures::sync::Lrc;
+use std::ops::Range;
 
 #[derive(Clone)]
 struct CacheEntry {
  time_stamp: usize,
  line_number: usize,
- line_start: BytePos,
- line_end: BytePos,
+ // The line's byte position range in the `SourceMap`. This range will fail to contain a valid
+ // position in certain edge cases. Spans often start/end one past something, and when that
+ // something is the last character of a file (this can happen when a file doesn't end in a
+ // newline, for example), we'd still like for the position to be considered within the last
+ // line. However, it isn't according to the exclusive upper bound of this range. We cannot
+ // change the upper bound to be inclusive, because for most lines, the upper bound is the same
+ // as the lower bound of the next line, so there would be an ambiguity.
+ //
+ // Since the containment aspect of this range is only used to see whether or not the cache
+ // entry contains a position, the only ramification of the above is that we will get cache
+ // misses for these rare positions. A line lookup for the position via `SourceMap::lookup_line`
+ // after a cache miss will produce the last line number, as desired.
+ line: Range<BytePos>,
  file: Lrc<SourceFile>,
  file_index: usize,
 }
@@ -26,8 +38,7 @@ impl<'sm> CachingSourceMapView<'sm> {
  let entry = CacheEntry {
  time_stamp: 0,
  line_number: 0,
- line_start: BytePos(0),
- line_end: BytePos(0),
+ line: BytePos(0)..BytePos(0),
  file: first_file,
  file_index: 0,
  };
@@ -47,13 +58,13 @@ impl<'sm> CachingSourceMapView<'sm> {
 
  // Check if the position is in one of the cached lines
  for cache_entry in self.line_cache.iter_mut() {
- if pos >= cache_entry.line_start && pos < cache_entry.line_end {
+ if cache_entry.line.contains(&pos) {
  cache_entry.time_stamp = self.time_stamp;
 
  return Some((
  cache_entry.file.clone(),
  cache_entry.line_number,
- pos - cache_entry.line_start,
+ pos - cache_entry.line.start,
  ));
  }
  }
@@ -69,13 +80,13 @@ impl<'sm> CachingSourceMapView<'sm> {
  let cache_entry = &mut self.line_cache[oldest];
 
  // If the entry doesn't point to the correct file, fix it up
- if pos < cache_entry.file.start_pos || pos >= cache_entry.file.end_pos {
+ if !file_contains(&cache_entry.file, pos) {
  let file_valid;
  if self.source_map.files().len() > 0 {
  let file_index = self.source_map.lookup_source_file_idx(pos);
  let file = self.source_map.files()[file_index].clone();
 
- if pos >= file.start_pos && pos < file.end_pos {
+ if file_contains(&file, pos) {
  cache_entry.file = file;
  cache_entry.file_index = file_index;
  file_valid = true;
@@ -95,10 +106,19 @@ impl<'sm> CachingSourceMapView<'sm> {
  let line_bounds = cache_entry.file.line_bounds(line_index);
 
  cache_entry.line_number = line_index + 1;
- cache_entry.line_start = line_bounds.0;
- cache_entry.line_end = line_bounds.1;
+ cache_entry.line = line_bounds;
  cache_entry.time_stamp = self.time_stamp;
 
- Some((cache_entry.file.clone(), cache_entry.line_number, pos - cache_entry.line_start))
+ Some((cache_entry.file.clone(), cache_entry.line_number, pos - cache_entry.line.start))
  }
 }
+
+#[inline]
+fn file_contains(file: &SourceFile, pos: BytePos) -> bool {
+ // `SourceMap::lookup_source_file_idx` and `SourceFile::contains` both consider the position
+ // one past the end of a file to belong to it. Normally, that's what we want. But for the
+ // purposes of converting a byte position to a line and column number, we can't come up with a
+ // line and column number if the file is empty, because an empty file doesn't contain any
+ // lines. So for our purposes, we don't consider empty files to contain any byte position.
+ file.contains(pos) && !file.is_empty()
+}

compiler/rustc_span/src/lib.rs

@@ -52,7 +52,7 @@ use std::cell::RefCell;
 use std::cmp::{self, Ordering};
 use std::fmt;
 use std::hash::Hash;
-use std::ops::{Add, Sub};
+use std::ops::{Add, Range, Sub};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 
@@ -1426,24 +1426,33 @@ impl SourceFile {
  if line_index >= 0 { Some(line_index as usize) } else { None }
  }
 
- pub fn line_bounds(&self, line_index: usize) -> (BytePos, BytePos) {
- if self.start_pos == self.end_pos {
- return (self.start_pos, self.end_pos);
+ pub fn line_bounds(&self, line_index: usize) -> Range<BytePos> {
+ if self.is_empty() {
+ return self.start_pos..self.end_pos;
  }
 
  assert!(line_index < self.lines.len());
  if line_index == (self.lines.len() - 1) {
- (self.lines[line_index], self.end_pos)
+ self.lines[line_index]..self.end_pos
  } else {
- (self.lines[line_index], self.lines[line_index + 1])
+ self.lines[line_index]..self.lines[line_index + 1]
  }
  }
 
+ /// Returns whether or not the file contains the given `SourceMap` byte
+ /// position. The position one past the end of the file is considered to be
+ /// contained by the file. This implies that files for which `is_empty`
+ /// returns true still contain one byte position according to this function.
  #[inline]
  pub fn contains(&self, byte_pos: BytePos) -> bool {
  byte_pos >= self.start_pos && byte_pos <= self.end_pos
  }
 
+ #[inline]
+ pub fn is_empty(&self) -> bool {
+ self.start_pos == self.end_pos
+ }
+
  /// Calculates the original byte position relative to the start of the file
  /// based on the given byte position.
  pub fn original_relative_byte_pos(&self, pos: BytePos) -> BytePos {