use `try_mark_green` for type-checking

[nikomatsakis]

Right now, the type-checker works by iterating over over every body in the crate and invoking typeck_tables_of on them; it then proceeds to discard the answer, since it doesn't actually care about the result, it just wants to ensure that the result has been computed (and in particular that any errors that may result have been reported).

In general, for cases where we are not interested in the result, I think that we should instead be using the try_mark_green operation, though we probably want a mildly cleaner interface to it first. The idea here is that try_mark_green will re-execute the query, but only if its inputs have changed; otherwise, it can mark the query as green without executing it. In particular, try_mark_green does not guarantee that the result of the query is "encached" in the tables when it completes.

This is useful for typeck in particular because I would like to at least have the option of skipping the typeck for particular items without saving the results onto disk. The current setup, where we actually do the query, means that we must have the typeck tables saved to disk, or else we will have to re-execute the typeck to regenerate them.

Now, even once we make this change, we will need to do more work to enable skipping typeck (particularly if we don't want to save the typeck tables to disk). We basically have to ensure that there are no other places in the code that will invoke typeck_tables_of unnecessarily (ideally: if the function or some dependent of it has not changed). In the shorter term, it may indeed be easier to just save the typeck results.

Still, this seems like a simple step we can take. Longer term, I think we will want to be able to minimize our disk and memory space footprint, while still achieving the best incremental results we can, and the ability to "lazilly" recompute things like typeck-tables will be an important factor in that.

I think I would break this issue into two parts:

Step 1. Package up the try_mark_green operation in some nicer way. I would do this by modifying the plumbing macros for the query infrastructure. I think we could add an accessor called try_mark_green, somewhat like force. It would look something like this:

/// Determines whether the query's result for `key` is up-to-date,
/// without necessarily executing the query. In particular, if all the inputs
/// to the query are green, then the query will not be re-executed. If however
/// some inputs have changed, then the query will be re-executed, and its
/// results cached (after being compared against the hash of the previous results).
///
/// This function is particularly useful when executing passes for their side-effects --
/// e.g., in order to report errors for erroneous programs.
///
/// Because the query will not execute unless it has changed, this function returns unit.
pub fn try_mark_green(
    tcx: TyCtxt<'a, $tcx, 'lcx>,
    key: $K)
{
    let dep_node = Self::to_dep_node(tcx, &key);
    tcx.dep_graph.try_mark_green(tcx, &dep_node);
}

Step 2. Convert code in librustc_typeck to use try_mark_green. Notably, change this line from self.tcx.typeck_tables_of(body_owner_def_id) to ty::maps::queries::typeck_tables_of::try_mark_green(self.tcx, body_owner_def_id).

cc @michaelwoerister, who may disagree forcefully with my premise here, I don't know

[Dylan-DPC-zz]

Hey Niko

I can take this up but I don't have much experience with the Rust compiler. So is the 1st step enough or do you expect me to add anything else to it?

[nikomatsakis]

@Dylan-DPC I'd be inclined to do both steps together, particularly since step 2 is just a single line.

[Dylan-DPC-zz]

i think you misunderstood me. I will do both steps. I was asking if step 1 is just that or if anything further needs to be done.

[michaelwoerister]

@nikomatsakis Some thoughts:

• Making decisions on whether try_mark_green fails or succeeds seems problematic if it is done inside a query. The state of the previous compilation session would then be an implicit input to the query, which seems like really not a good idea!
• Calling try_mark_green from the outside of the query system (like we do for translation) is harmless though.
• Just calling try_mark_green without inspecting its return value or checking whether the node is actually green afterwards will not do anything. try_mark_green does not re-run the query it is invoked on, it just re-runs dependencies. So calling try_mark_green(DepNode::TypeckTables(xxx)) would never do any type-checking, be it whether marking fails or succeeds.

As a consequence I would suggest the following strategy:

1. Make typeck_item_bodies a non-query
2. Make it call try_mark_green on everything. IFF such a try_mark_green call fails, run the corresponding query in order to force recomputing the result and emitting the desired error-messages.

That would safely make type-checking as lazy as it can be while still ensuring that the whole crate is checked.

[nikomatsakis]

@michaelwoerister

Just calling try_mark_green without inspecting its return value or checking whether the node is actually green afterwards will not do anything

OK, I misunderstood. =) Perhaps we need a different operation. I will call it ensure().

ensure() guarantees that one of two things is true after it returns:

• The query has all green inputs.
• The query has been executed.

Moreover, once we properly save/restore diagnostic output, then ensure() would also replay that diagnostic output if all the inputs are green.

ensure() has a unit return, to avoid one making decisions on the basis of its result. (In other words, you don't know if the query was up to date or not.)

This would then be implemented in terms of mark-green roughly as you suggested: execute try_mark_green and check the return value, executing the query if needed (eventually: loading the diagnostic output if needed).

Also, executing query::ensure(D) should be considered a read of the dep-node D.

Calling try_mark_green from the outside of the query system (like we do for translation) is harmless though.

The concern here I presume that try-mark-green is not considered to "read" the query it applies to, right? I think however that if we use the ensure() that I wrote above -- which in particular is considered to be a read of the query that is being ensured -- then it should be fine either way, right?

That said, I have no particular problem with making typeck-item-bodies not be a query. I don't think it's a particularly useful one. I was however predicting that we would want to eventually be able to use this operation from inside queries -- e.g., we might want to be able to have a query that (from an IDE) checks that all errors are reported. But perhaps that should just be a subroutine instead, now that I think about it. I don't know how much it really matters.

[theotherjimmy]

@nikomatsakis implementing ensure may not be a great idea as typeck_item_bodies, const_eval and layout_raw all return Result types. However, these are queries, and the Result is saved implicitly.

Implementing ensure then involves using the:

let _ = tcx.$name(key)

anti-pattern, which left a bad taste in my mouth. My prototype branch does not seem to fail though...

Once we have ensure, we should use it for various other queries that are executed for their side effects. coherient_trait, borrowck, mir_borrowck and crate_inherent_impls_overlap_check are immediate candidates, as they return ().

[michaelwoerister]

I think that ensure() would be an additional primitive in the evaluation model and, while I guess that it would be a rather harmless one, I am very hesitant to add new primitives to the model. Especially since there is a clean, well-understood alternative.

I think that the queries that are just invoked by the driver (like check_item_bodies) should either be converted to simple procedures or, if they are needed as a real queries somewhere else, the driver should have an additional non-query version that does what ensure() would do. (That could be also be realized by implementing ensure() but forbidding it to be called from a query provider).

I'll think about this some more. Maybe calling ensure() from a query provider would be harmless. I just don't want to do ad-hoc changes to the system.

[nikomatsakis]

@michaelwoerister

Especially since there is a clean, well-understood alternative.

The alternatives I see to ensure(Q) where Q is a query are:

• You can make a new query Q' with unit result type that invokes Q and then discards its result. The result of Q' can be serialized (and/or we can specialize unit results) so that -- if the query is green -- it will not be re-executed. In that case, performing query Q' is equivalent to ensure(Q).
• You can use try_mark_green directly from outside of any query. This feels considerably more ad-hoc to me -- it seems to me now that try_mark_green is really a kind of "hidden primitive" that ought to be private to the dep-graph and query system, since it can easily be mis-used (unlike ensure() or a unit query).
  • It is particularly annoying that it cannot assert that it is not used from inside a query, since of course the system itself must do so.

I would therefore prefer not add wrappers into the driver that use try-mark-green to invoke compilation only for side-effects. Unit queries seem ok, particularly if we start to optimize the query representative more -- at some point, the maximally optimized version just becomes what I described as ensure() (that is, we don't have a hashmap, since there is no value, and we instead use the color of the dep-node to decide whether the query needs to be executed).