Implement lazy loading of external crates' sources. #42593

ibabushkin · 2017-06-11T13:27:42Z

Fixes #38875. This is a follow-up to #42507. When a (now correctly translated) span from an external crate is referenced in a error, warning or info message, we still don't have the source code being referenced.
Since stuffing the source in the serialized metadata of an rlib is extremely wasteful, the following scheme has been implemented:

File maps now contain a source hash that gets serialized as well.
When a span is rendered in a message, the source hash in the corresponding file map(s) is used to try and load the source from the corresponding file on disk. If the file is not found or the hashes don't match, the failed attempt is recorded (and not retried).
The machinery fetching source lines from file maps is augmented to use the lazily loaded external source as a secondary fallback for file maps belonging to external crates.

This required a small change to the expected stderr of one UI test (it now renders a span, where previously was none).

Further work can be done based on this - some of the machinery previously used to hide external spans is possibly obsolete and the hashing code can be reused in different places as well.

r? @eddyb

We can use these to perform lazy loading of source files belonging to external crates. That way we will be able to show the source code of external spans that have been translated.

The rationale is that BOM stripping is needed for lazy source loading for external crates, and duplication can be avoided by moving the corresponding functionality to libsyntax_pos.

They are now handled in their own member to prevent mutating access to the `src` member. This way, we can safely load external sources, while keeping the mutation of local source strings off-limits.

We now fetch source lines from the `external_src` member as a secondary fallback if no regular source is present, that is, if the file map belongs to an external crate and the source has been fetched from disk.

rust-highfive · 2017-06-11T13:27:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eddyb · 2017-06-11T14:23:41Z

src/librustc_data_structures/stable_hasher.rs

+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);


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

eddyb · 2017-06-11T14:24:32Z

cc @jseyfried @nrc

eddyb · 2017-06-12T10:48:50Z

src/libsyntax/codemap.rs

@@ -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) {


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

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.

eddyb · 2017-06-12T10:49:34Z

src/libsyntax_pos/lib.rs

@@ -532,26 +604,55 @@ impl FileMap {
        lines.push(pos);
    }

+    /// add externally loaded source.


Sentences in doc comment should start with a capital letter.

Oh and this comment should probably mention how it shouldn't be called except from CodeMap::load_source_for_filemap.

eddyb · 2017-06-12T10:49:59Z

src/libsyntax_pos/lib.rs

+
+        false
+    }
+
    /// get a line from the list of pre-computed line-beginnings.


Heh, I can see where you got the bad style from though (this is very old code).

eddyb · 2017-06-12T10:56:26Z

src/librustc_errors/lib.rs

@@ -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;


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.

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.

eddyb · 2017-06-12T10:59:58Z

src/librustc_errors/emitter.rs

@@ -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));


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

eddyb · 2017-06-12T11:00:56Z

src/librustc_errors/emitter.rs

@@ -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() {


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

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

* The lazy loading mechanism has been moved to a more appropriate place. * Return values from the functions invoked there are properly used. * Documentation has gotten some minor improvements. * Possibly some larger restructuring will need to take place still.

* In other places where the `src` member of a file map is accessed, we now load and possibly work with external source as well.

nikomatsakis · 2017-06-13T12:43:52Z

@ibabushkin this is awesome!

A bug has been discovered and fixed in the process.

Mark-Simulacrum · 2017-06-17T19:41:12Z

@eddyb Looks like this is ready for another review pass.

eddyb · 2017-06-17T19:47:36Z

@bors r+

bors · 2017-06-17T19:47:37Z

📌 Commit bd4fe45 has been approved by eddyb

eddyb · 2017-06-17T19:48:45Z

@ibabushkin Oh you might want to add that this fixes #38875.

bors · 2017-06-18T00:04:21Z

⌛ Testing commit bd4fe45 with merge c3b67e6d46a93b7a26a43ba743ccef84a1f24368...

bors · 2017-06-18T03:38:04Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-06-18T03:41:40Z

@bors retry

os x timed out

bors · 2017-06-18T10:41:14Z

⌛ Testing commit bd4fe45 with merge 28cc0c5...

@eddyb

Implement lazy loading of external crates' sources. Fixes #38875 Fixes #38875. This is a follow-up to #42507. When a (now correctly translated) span from an external crate is referenced in a error, warning or info message, we still don't have the source code being referenced. Since stuffing the source in the serialized metadata of an rlib is extremely wasteful, the following scheme has been implemented: * File maps now contain a source hash that gets serialized as well. * When a span is rendered in a message, the source hash in the corresponding file map(s) is used to try and load the source from the corresponding file on disk. If the file is not found or the hashes don't match, the failed attempt is recorded (and not retried). * The machinery fetching source lines from file maps is augmented to use the lazily loaded external source as a secondary fallback for file maps belonging to external crates. This required a small change to the expected stderr of one UI test (it now renders a span, where previously was none). Further work can be done based on this - some of the machinery previously used to hide external spans is possibly obsolete and the hashing code can be reused in different places as well. r? @eddyb

bors · 2017-06-18T15:08:50Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 28cc0c5 to master...

ibabushkin added 8 commits June 10, 2017 12:21

Added a StableHasherResult impl for u128.

70fa1fb

Added source hashes to FileMap

3d2cff0

We can use these to perform lazy loading of source files belonging to external crates. That way we will be able to show the source code of external spans that have been translated.

Moved FileMap construction to it's own constructor.

dd8f7cd

The rationale is that BOM stripping is needed for lazy source loading for external crates, and duplication can be avoided by moving the corresponding functionality to libsyntax_pos.

Added external crates' sources to FileMap.

c2c31b2

They are now handled in their own member to prevent mutating access to the `src` member. This way, we can safely load external sources, while keeping the mutation of local source strings off-limits.

Improved lazy external source loading and inserted calls.

c04aa4e

Added consumption logic for external sources in FileMap

a5b8851

We now fetch source lines from the `external_src` member as a secondary fallback if no regular source is present, that is, if the file map belongs to an external crate and the source has been fetched from disk.

Added hash verification to external source loading.

9a8bbe9

Updated UI tests to include rendered external spans.

634cd2c

rust-highfive assigned eddyb Jun 11, 2017

eddyb reviewed Jun 11, 2017

View reviewed changes

ibabushkin changed the title ~~Implement lazy loading of external crate's sources~~ Implement lazy loading of external crates' sources Jun 11, 2017

External spans: fixed unit tests and addressed review.

afe8415

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2017

eddyb reviewed Jun 12, 2017

View reviewed changes

ibabushkin added 2 commits June 12, 2017 15:37

External spans: added lazy source loading elsewhere

d11973a

* In other places where the `src` member of a file map is accessed, we now load and possibly work with external source as well.

External spans: Added a test for rust-lang#38875.

bd4fe45

A bug has been discovered and fixed in the process.

ibabushkin changed the title ~~Implement lazy loading of external crates' sources~~ Implement lazy loading of external crates' sources. Fixes #38875 Jun 17, 2017

eddyb changed the title ~~Implement lazy loading of external crates' sources. Fixes #38875~~ Implement lazy loading of external crates' sources. Jun 18, 2017

bors merged commit bd4fe45 into rust-lang:master Jun 18, 2017

ibabushkin mentioned this pull request Jun 19, 2017

Fix inter-crate diagnostic messages rust-lang/rust-semverver#5

Closed

workingjubilee mentioned this pull request May 18, 2024

Move tests #125256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement lazy loading of external crates' sources. #42593

Implement lazy loading of external crates' sources. #42593

ibabushkin commented Jun 11, 2017 •

edited

Loading

rust-highfive commented Jun 11, 2017

eddyb Jun 11, 2017

eddyb commented Jun 11, 2017

eddyb Jun 12, 2017

ibabushkin Jun 12, 2017

eddyb Jun 12, 2017

eddyb Jun 12, 2017

eddyb Jun 12, 2017

eddyb Jun 12, 2017

ibabushkin Jun 12, 2017

eddyb Jun 12, 2017 •

edited

Loading

eddyb Jun 12, 2017

ibabushkin Jun 12, 2017

nikomatsakis commented Jun 13, 2017

Mark-Simulacrum commented Jun 17, 2017

eddyb commented Jun 17, 2017

bors commented Jun 17, 2017

eddyb commented Jun 17, 2017

bors commented Jun 18, 2017

bors commented Jun 18, 2017

Mark-Simulacrum commented Jun 18, 2017

bors commented Jun 18, 2017

bors commented Jun 18, 2017

Implement lazy loading of external crates' sources. #42593

Implement lazy loading of external crates' sources. #42593

Conversation

ibabushkin commented Jun 11, 2017 • edited Loading

rust-highfive commented Jun 11, 2017

Choose a reason for hiding this comment

eddyb commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jun 13, 2017

Mark-Simulacrum commented Jun 17, 2017

eddyb commented Jun 17, 2017

bors commented Jun 17, 2017

eddyb commented Jun 17, 2017

bors commented Jun 18, 2017

bors commented Jun 18, 2017

Mark-Simulacrum commented Jun 18, 2017

bors commented Jun 18, 2017

bors commented Jun 18, 2017

ibabushkin commented Jun 11, 2017 •

edited

Loading

eddyb Jun 12, 2017 •

edited

Loading