DepGraph::with_ignore misuse

[tgnottingham]

DepGraph::with_ignore(closure) either doesn't work as intended, or some clients are confused about what it does. Some clients are under the impression that this prevents the closure from modifying the dep graph, but that's not the case.

For example, in OnDiskCache::serialize, we serialize the query result cache, and we apparently don't want the process of serializing the cache to modify the graph itself (seems reasonable):

// Serializing the `DepGraph` should not modify it.
// (The comment is off--we're actually serializing the result cache here.)
tcx.dep_graph.with_ignore(|| {
    ...
})

But it does. The calls to queries tcx.original_crate_name and tcx.crate_disambiguator within that closure can and often do modify the graph (I verified by adding some eprintlns to print the graph node count before and after these queries). So, when we later (or in parallel) serialize the actual dep graph, it's inconsistent with the result cache. Specifically, the graph has nodes for which no query result was serialized, though perhaps they should have been ("perhaps" because I'm not sure it matters for the particular queries involved here).

I haven't investigated the full ramifications of this specific use of with_ignore. It may account for some existing incr-comp bugs. It's likely an even bigger problem for parallel builds, since the result cache and graph are serialized in parallel.

But this is just one of many uses of with_ignore. If the above analysis is correct, I think we need to provide a way to prevent dep graph modification while executing some code, and vet uses of with_ignore to see whether or not they actually need the functionality provided by it, or need the new don't-modify-dep-graph functionality. We may also want to rename with_ignore so that it's more clear what it does.

@rustbot label A-incr-comp

[tgnottingham]

@michaelwoerister You can probably verify whether this is a real issue or I'm just misunderstanding something. :)

[michaelwoerister]

@tgnottingham I just saw this now. Interesting find!

The purpose for DepGraph::with_ignore() is indeed to allow for doing things that are exempt from dependency tracking. Maybe a bug has crept in at some point? This is definitely something we should look into (and maybe add some assertions that make sure this does not regress again).

How confident are you about your findings?

[tgnottingham]

How confident are you about your findings?

@michaelwoerister -- Well, the increase in node count before and after those query invocations was a clear indication that the dep graph was being modified while we were in the with_ignore closure. But I'm not totally certain what the ramifications are. I mentioned that the dep graph and result cache would have an inconsistency, but that was speculation based on my understanding, not something something I've verified.