Assert redundant incremental check

[zetanumbers]

So to incrementally execute tcx.ensure_done().query() we first do query graph coloring to determine if nothing has changed and maybe color our query green. In that case for ensure_ok we skip further execution and return. However for ensure_done we do this check:

rust/compiler/rustc_query_impl/src/execution.rs

Lines 601 to 608 in 7c3c88f

	// In ensure-done mode, we can only skip execution for this key
	// if there's a disk-cached value available to load later if
	// needed, which guarantees the query provider will never run
	// for this key.
	EnsureMode::Done => {
	(query.will_cache_on_disk_for_key_fn)(key)
	&& loadable_from_disk(tcx, serialized_dep_node_index)
	}

Which in turn looks up a serialized query value:

rust/compiler/rustc_middle/src/query/on_disk_cache.rs

Lines 338 to 343 in 7c3c88f

	/// Returns true if there is a disk-cached query return value for the given node.
	#[inline]
	pub fn loadable_from_disk(&self, dep_node_index: SerializedDepNodeIndex) -> bool {
	self.query_values_index.contains_key(&dep_node_index)
	// with_decoder is infallible, so we can stop here
	}

However, we only serialize query values if the pure (I've checked) function will_cache_on_disk_for_key_fn from above returns true:

rust/compiler/rustc_query_impl/src/plumbing.rs

Lines 95 to 99 in 7c3c88f

	query.cache.for_each(&mut |key, value, dep_node| {
	if (query.will_cache_on_disk_for_key_fn)(*key) {
	encoder.encode_query_value::<V>(dep_node, &erase::restore_val::<V>(*value));
	}
	});

As such loadable_from_disk should be able to simply check if tcx.query_system.on_disk_cache is loaded and assume query value is present, assuming that will_cache_on_disk_for_key_fn returned true.

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, query-system
• compiler, query-system expanded to 73 candidates
• Random selection from 17 candidates

[zetanumbers]

I think we can clarify this change with a comment or by outright inlining that function. Not sure what to pick tho.

[zetanumbers]

Also that check is covered by tests, so I making it unconditionally panic would trigger following incremental tests failures:

• change_crate_dep_kind.rs
• const-generics/change-const-param-type.rs
• const-generics/change-const-param-gat.rs
• hashes/consts.rs
• define-opaques.rs
• hashes/closure_expressions.rs
• feature_gate.rs
• commandline-args.rs
• hashes/call_expressions.rs
• hashes/enum_constructors.rs
• hashes/if_expressions.rs
• hashes/loop_expressions.rs
• hashes/function_interfaces.rs
• hashes/for_loops.rs
• hashes/inherent_impls.rs
• hashes/struct_constructors.rs
• hashes/while_loops.rs
• hashes/statics.rs
• hashes/while_let_loops.rs
• hashes/trait_impls.rs
• ich_nested_items.rs
• issue-100521-change-struct-name-assocty.rs
• issue-49595/issue-49595.rs
• reorder_vtable.rs
• spans_significant_w_panic.rs
• spans_significant_w_debuginfo.rs
• issue-110457-same-span-closures/main.rs
• static_cycle/b.rs
• static_refering_to_other_static2/issue.rs
• static_refering_to_other_static3/issue.rs
• thinlto/cgu_keeps_identical_fn.rs
• static_stable_hash/issue-49301.rs
• thinlto/cgu_invalidated_when_export_added.rs
• unrecoverable_query.rs

[zetanumbers]

I think we could change it to debug_assert and check if perf is affected.

[chenyukang]

@rustbot reroll

[Zalathar]

I think we can clarify this change with a comment or by outright inlining that function. Not sure what to pick tho.

Making the existing helper function assert seems like a hazard if any other code starts calling it, so I think it's clearer to inline and remove the function.

After inlining I think we can also get rid of the else arm and call on_disk_cache.as_ref().unwrap(), because the disk-cache struct should always be present in incremental sessions, even if no previous session was loaded.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[cjgillot]

What if we restart an incremental compilation with a working dep-graph, but the on-disk cache has been deleted?

[zetanumbers]

What if we restart an incremental compilation with a working dep-graph, but the on-disk cache has been deleted?

I think that shouldn't matter:

rust/compiler/rustc_incremental/src/persist/save.rs

Lines 61 to 63 in 783eb8c

	// The on-disk cache struct is always present in incremental mode,
	// even if there was no previous session.
	let on_disk_cache = tcx.query_system.on_disk_cache.as_ref().unwrap();

[Zalathar]

Indeed, here is where the OnDiskCache gets created:

rust/compiler/rustc_incremental/src/persist/load.rs

Lines 122 to 147 in 783eb8c

	/// Attempts to load the query result cache from disk
	///
	/// If we are not in incremental compilation mode, returns `None`.
	/// Otherwise, tries to load the query result cache from disk,
	/// creating an empty cache if it could not be loaded.
	pub fn load_query_result_cache(sess: &Session) -> Option<OnDiskCache> {
	if sess.opts.incremental.is_none() {
	return None;
	}
	let _prof_timer = sess.prof.generic_activity("incr_comp_load_query_result_cache");
	let path = query_cache_path(sess);
	match file_format::open_incremental_file(sess, &path) {
	Ok(OpenFile { mmap, start_pos }) => {
	let cache = OnDiskCache::new(sess, mmap, start_pos).unwrap_or_else(|()| {
	sess.dcx().emit_warn(errors::CorruptFile { path: &path });
	OnDiskCache::new_empty()
	});
	Some(cache)
	}
	Err(OpenFileError::NotFoundOrHeaderMismatch | OpenFileError::IoError { .. }) => {
	Some(OnDiskCache::new_empty())
	}
	}
	}

As long as we're in incremental mode, we fall back to creating an empty disk-cache struct if nothing could be loaded. So the field is only None in non-incremental mode.

[zetanumbers]

Should be good now I think.

[Zalathar]

Oh wait, I think I misunderstood the concern.

If the disk-cache was deleted since the last session, the OnDiskCache will be present but empty ... so even values that should be in the disk-cache will be absent, violating the assertion.

So removing the check might be wrong after all.

[zetanumbers]

Oh wait, I think I misunderstood the concern.

If the disk-cache was deleted since the last session, the OnDiskCache will be present but empty ... so even values that should be in the disk-cache will be absent, violating the assertion.

So removing the check might be wrong after all.

Right, I forgot about that. So can we make it a None for that case, just like I've assumed it in the beginning?

[zetanumbers]

Like that it should be ok, right?

[rust-bors]

☔ The latest upstream changes (presumably #157149) made this pull request unmergeable. Please resolve the merge conflicts.