Skip to content

Implement lazy loading of external crates' sources. #42593

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.

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

Merged
merged 12 commits into from
Jun 18, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
1 change: 1 addition & 0 deletions src/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/librustc/ich/impls_syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,8 @@ impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for FileMa
crate_of_origin,
// Do not hash the source as it is not encoded
src: _,
src_hash,
external_src: _,
start_pos,
end_pos: _,
ref lines,
Expand All @@ -350,6 +352,8 @@ impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for FileMa
index: CRATE_DEF_INDEX,
}.hash_stable(hcx, hasher);

src_hash.hash_stable(hcx, hasher);

// We only hash the relative position within this filemap
let lines = lines.borrow();
lines.len().hash_stable(hcx, hasher);
Expand Down
11 changes: 11 additions & 0 deletions src/librustc_data_structures/stable_hasher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,17 @@ impl StableHasherResult for [u8; 20] {
}
}

impl StableHasherResult for u128 {
fn finish(mut hasher: StableHasher<Self>) -> Self {
let hash_bytes: &[u8] = hasher.finalize();
assert!(hash_bytes.len() >= mem::size_of::<u64>() * 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be size_of::<u128>().


unsafe {
::std::ptr::read_unaligned(hash_bytes.as_ptr() as *const u128)
}
}
}

impl StableHasherResult for u64 {
fn finish(mut hasher: StableHasher<Self>) -> Self {
hasher.state.finalize();
Expand Down
11 changes: 8 additions & 3 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use RenderSpan::*;
use snippet::{Annotation, AnnotationType, Line, MultilineAnnotation, StyledString, Style};
use styled_buffer::StyledBuffer;

use std::borrow::Cow;
use std::io::prelude::*;
use std::io;
use std::rc::Rc;
Expand Down Expand Up @@ -131,7 +132,7 @@ impl EmitterWriter {
}
}

fn preprocess_annotations(&self, msp: &MultiSpan) -> Vec<FileWithAnnotatedLines> {
fn preprocess_annotations(&mut self, msp: &MultiSpan) -> Vec<FileWithAnnotatedLines> {
fn add_annotation_to_file(file_vec: &mut Vec<FileWithAnnotatedLines>,
file: Rc<FileMap>,
line_index: usize,
Expand Down Expand Up @@ -175,6 +176,9 @@ impl EmitterWriter {
if span_label.span == DUMMY_SP {
continue;
}

cm.load_source_for_filemap(cm.span_to_filename(span_label.span));
Copy link
Member

@eddyb eddyb Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should do this where the src.is_none() check was.


let lo = cm.lookup_char_pos(span_label.span.lo);
let mut hi = cm.lookup_char_pos(span_label.span.hi);

Expand Down Expand Up @@ -908,7 +912,8 @@ impl EmitterWriter {
// Print out the annotate source lines that correspond with the error
for annotated_file in annotated_files {
// we can't annotate anything if the source is unavailable.
if annotated_file.file.src.is_none() {
if annotated_file.file.src.is_none()
&& annotated_file.file.external_src.borrow().is_absent() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a single method on FileMap. I'd even put the attempt to load the external source in that method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do that, since said method would have to be defined in a place where the FileLoader doesn't exist.

continue;
}

Expand Down Expand Up @@ -1009,7 +1014,7 @@ impl EmitterWriter {
} else if line_idx_delta == 2 {
let unannotated_line = annotated_file.file
.get_line(annotated_file.lines[line_idx].line_index)
.unwrap_or("");
.unwrap_or_else(|| Cow::from(""));

let last_buffer_line_num = buffer.num_lines();

Expand Down
12 changes: 7 additions & 5 deletions src/librustc_errors/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use self::Level::*;

use emitter::{Emitter, EmitterWriter};

use std::borrow::Cow;
use std::cell::{RefCell, Cell};
use std::{error, fmt};
use std::rc::Rc;
Expand Down Expand Up @@ -103,6 +104,7 @@ pub trait CodeMapper {
fn span_to_filename(&self, sp: Span) -> FileName;
fn merge_spans(&self, sp_lhs: Span, sp_rhs: Span) -> Option<Span>;
fn call_span_if_macro(&self, sp: Span) -> Span;
fn load_source_for_filemap(&self, file: FileName) -> bool;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit ridiculous that CodeMap has to be abstracted away because rustc_errors doesn't contain it and FileMap can't even see this trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the long term solution would be to move CodeMapper to libsyntax_pos? Or better yet, move CodeMap there and remove it entirely? I'm not sure which other consequences this might have though.

}

impl CodeSuggestion {
Expand All @@ -121,7 +123,7 @@ impl CodeSuggestion {
use syntax_pos::{CharPos, Loc, Pos};

fn push_trailing(buf: &mut String,
line_opt: Option<&str>,
line_opt: Option<&Cow<str>>,
lo: &Loc,
hi_opt: Option<&Loc>) {
let (lo, hi_opt) = (lo.col.to_usize(), hi_opt.map(|hi| hi.col.to_usize()));
Expand Down Expand Up @@ -183,13 +185,13 @@ impl CodeSuggestion {
let cur_lo = cm.lookup_char_pos(sp.lo);
for (buf, substitute) in bufs.iter_mut().zip(substitutes) {
if prev_hi.line == cur_lo.line {
push_trailing(buf, prev_line, &prev_hi, Some(&cur_lo));
push_trailing(buf, prev_line.as_ref(), &prev_hi, Some(&cur_lo));
} else {
push_trailing(buf, prev_line, &prev_hi, None);
push_trailing(buf, prev_line.as_ref(), &prev_hi, None);
// push lines between the previous and current span (if any)
for idx in prev_hi.line..(cur_lo.line - 1) {
if let Some(line) = fm.get_line(idx) {
buf.push_str(line);
buf.push_str(line.as_ref());
buf.push('\n');
}
}
Expand All @@ -205,7 +207,7 @@ impl CodeSuggestion {
for buf in &mut bufs {
// if the replacement already ends with a newline, don't print the next line
if !buf.ends_with('\n') {
push_trailing(buf, prev_line, &prev_hi, None);
push_trailing(buf, prev_line.as_ref(), &prev_hi, None);
}
// remove trailing newline
buf.pop();
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_metadata/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,7 @@ impl<'a, 'tcx> CrateMetadata {
// containing the information we need.
let syntax_pos::FileMap { name,
name_was_remapped,
src_hash,
start_pos,
end_pos,
lines,
Expand All @@ -1173,6 +1174,7 @@ impl<'a, 'tcx> CrateMetadata {
let local_version = local_codemap.new_imported_filemap(name,
name_was_remapped,
self.cnum.as_u32(),
src_hash,
source_length,
lines,
multibyte_chars);
Expand Down
39 changes: 20 additions & 19 deletions src/libsyntax/codemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,29 +158,13 @@ impl CodeMap {

/// Creates a new filemap without setting its line information. If you don't
/// intend to set the line information yourself, you should use new_filemap_and_lines.
pub fn new_filemap(&self, filename: FileName, mut src: String) -> Rc<FileMap> {
pub fn new_filemap(&self, filename: FileName, src: String) -> Rc<FileMap> {
let start_pos = self.next_start_pos();
let mut files = self.files.borrow_mut();

// Remove utf-8 BOM if any.
if src.starts_with("\u{feff}") {
src.drain(..3);
}

let end_pos = start_pos + src.len();

let (filename, was_remapped) = self.path_mapping.map_prefix(filename);

let filemap = Rc::new(FileMap {
name: filename,
name_was_remapped: was_remapped,
crate_of_origin: 0,
src: Some(Rc::new(src)),
start_pos: Pos::from_usize(start_pos),
end_pos: Pos::from_usize(end_pos),
lines: RefCell::new(Vec::new()),
multibyte_chars: RefCell::new(Vec::new()),
});
let filemap =
Rc::new(FileMap::new(filename, was_remapped, src, Pos::from_usize(start_pos)));

files.push(filemap.clone());

Expand Down Expand Up @@ -210,6 +194,7 @@ impl CodeMap {
filename: FileName,
name_was_remapped: bool,
crate_of_origin: u32,
src_hash: u128,
source_len: usize,
mut file_local_lines: Vec<BytePos>,
mut file_local_multibyte_chars: Vec<MultiByteChar>)
Expand All @@ -233,6 +218,8 @@ impl CodeMap {
name_was_remapped: name_was_remapped,
crate_of_origin: crate_of_origin,
src: None,
src_hash: src_hash,
external_src: RefCell::new(ExternalSource::AbsentOk),
start_pos: start_pos,
end_pos: end_pos,
lines: RefCell::new(file_local_lines),
Expand Down Expand Up @@ -572,6 +559,20 @@ impl CodeMapper for CodeMap {
}
sp
}
fn load_source_for_filemap(&self, filename: FileName) -> bool {
let file_map = if let Some(fm) = self.get_filemap(&filename) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea - a caller would already have the FileMap, and not from a string lookup, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only caller I've inserted has a MultiSpan, iterates over .span_labels() and gets the filenames after looking up the byte pos in the span. I guess this can be cleaned up.

fm
} else {
return false;
};

if *file_map.external_src.borrow() == ExternalSource::AbsentOk {
let src = self.file_loader.read_file(Path::new(&filename)).ok();
return file_map.add_external_src(src);
}

false
}
}

#[derive(Clone)]
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ impl DiagnosticSpanLine {
h_end: usize)
-> DiagnosticSpanLine {
DiagnosticSpanLine {
text: fm.get_line(index).unwrap_or("").to_owned(),
text: fm.get_line(index).map_or(String::new(), |l| l.into_owned()),
highlight_start: h_start,
highlight_end: h_end,
}
Expand Down
1 change: 1 addition & 0 deletions src/libsyntax_pos/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ crate-type = ["dylib"]

[dependencies]
serialize = { path = "../libserialize" }
rustc_data_structures = { path = "../librustc_data_structures" }
Loading