Map crate numbers between compilations

[nikomatsakis]

It appears we are not checking for crate-numbers having changed. The best thing here would probably be to find the new crate-number. The easiest thing, however, would be to store -- for each crate-number we have -- the name/discriminant and then double-check that nothing has changed. I'll probably start with the easy fix for now.

[nikomatsakis]

I have a simple fix for that where, on startup, the compiler just validates that the crate numbers belong to the same crate as they used to (it doesn't try to find a new crate number if they don't match). This is suboptimal, clearly, but not terrible.

[nikomatsakis]

Removing from milestone since the current solution is good enough for the time being, I believe.

[nikomatsakis]

Tagging with E-mentor. This would be a good entry point for people looking to help out with incremental compilation. The remaining work is to edit the code in use crate-name/disambiguator in src/librustc_incremental/persist/directory.rs so that instead of calling krate_still_valid to check whether a given crate-number is still valid, it creates a mapping from the old crate numbers to the new ones.

We already save (and re-load) the crate-name and disambiguator (two bits of data that together identify a crate) for each crate-number, so this is basically a matter of re-loading them in the new state and comparing the old against the new.

[mikhail-m1]

I'd like to work on it

[mikhail-m1]

I've briefly review your commit. I should create map from old CrateNum to an new base on name and disambiguator pair. And if old crate has found return retraced path or None in other case. I'm right?

Is it any way to check this issue?

[mikhail-m1]

I replaced Vec by HashMap (for debug purpose, HasSet is enough)
and check at

#[cfg(rpass1)]
extern crate a;
#[cfg(rpass1)]
extern crate b;

#[cfg(rpass2)]
extern crate b;
#[cfg(rpass2)]
extern crate a;

use a::A;
use b::B;

pub fn main() { 
    A + B;
}

But test tests/incremental/krate_reassign_34991 fails, I reverted all my changes and add just trace which outputs crate name and disambiguator, and it fails too. Currently it works because module index is lager than kates vec len.

[nikomatsakis]

@mikhail-m1 Argh, sorry for not responding, I've been behind on GH notifications the last few weeks apparently. Can you give me a bit more details on how the krate_reassign_34991 test failed? (What message did you get, etc?) Also, if you could link to a commit (perhaps on your repo) with your changes, it would be helpful. That said, I think that your test case is a pretty good one. :)

One of the trickier parts of this issue is indeed that you must be wary that an extern crate may no longer exist. The way I had envisioned handling this is to iterate over all the new crate numbers -- e.g., by calling tcx.sess.cstore.crates() -- and creating a map from the pair (crate-name, disambiguator) to ast::CrateNum (also insert the local crate). We already have the field krates which provides a mapping from "old crate number" to that same information. So then when we handle an old path, we can:

1. Convert the old crate number to a create-name / disambiguator.
2. Lookup in the mapping to find the new crate number, if any.
3. As you suggested, if no mapping is found, we return None.
4. Otherwise, we make a copy of the DefPath, change krate to the new crate, and retrace.

So this loop that we have today:

        let ids = self.paths.iter()
                            .map(|path| {
                                if self.krate_still_valid(tcx, max_current_crate, path.krate) {
                                    tcx.retrace_path(path)
                                } else {
                                    debug!("crate {} changed from {:?} to {:?}/{:?}",
                                           path.krate,
                                           self.krates[path.krate as usize],
                                           tcx.crate_name(path.krate),
                                           tcx.crate_disambiguator(path.krate));
                                    None
                                }
                            })
                            .collect();

might become:

        let ids = self.paths.iter()
                            .map(|path| {
                                if let Some(new_krate) = self.map_crate(path.krate) {
                                    let mut new_path = DefPath { krate: new_krate, data: path.data.clone() };
                                    tcx.retrace_path(&new_path)
                                } else {
                                    // ...just as before...
                                }
                            })
                            .collect();

Here map_crate does the remapping I talked about above.

Now, for bonus points, you might change the interface to retrace_path to not take a DefPath but rather take a CrateNum and &[DisambiguatedDefPathData], so that we don't have to clone the old def-path, but can rather call tcx.retrace_path(new-krate, &path.data). But I'd do that as a follow-up commit. =)

[mikhail-m1]

this trace mikhail-m1@6aaf566 breaks krate_reassign_34991. I'll send full change list later, I need some time to clear it

[nikomatsakis]

@mikhail-m1 ah! yes, that would cause a problem, since invoking tcx.crate_name() expects a "new crate number" but you are passing an "old crate number" (which may be out of range). This is why the code below calls krate_still_valid() :)

[mikhail-m1]

@nikomatsakis I missed max check, now it works but I don't know how to write check in test
mikhail-m1@8a17f7a

[mikhail-m1]

and bonus mikhail-m1@738c026
I'll do PR as soon as fix test

[nikomatsakis]

@mikhail-m1 getting closer =) I left some comments here, let me know what you think.

[mikhail-m1]

@nikomatsakis thanks for comments, I'll send you reworked version soon, but I still don't know how write test for it

[mikhail-m1]

@nikomatsakis Next try mikhail-m1@16b7e56 :)

[nikomatsakis]

@mikhail-m1 that looks great! :) I left a few nits.

[ghost]

Looks like this problem is solved, and the issue could be closed.

[nikomatsakis]

Agreed.