Skip to content

Rename dep node "fingerprints" to distinguish key and value hashes#152751

Open
Zalathar wants to merge 2 commits intorust-lang:mainfrom
Zalathar:fingerprint
Open

Rename dep node "fingerprints" to distinguish key and value hashes#152751
Zalathar wants to merge 2 commits intorust-lang:mainfrom
Zalathar:fingerprint

Conversation

@Zalathar
Copy link
Member

In the query system's dependency graph, each node is associated with two fingerprints: one that is typically a hash of the query key, and one that is typically a hash of the query's return value when called with that key.

Unfortunately, many identifiers and comments fail to clearly distinguish between these two kinds of fingerprint, which have very different roles in dependency tracking. This is a frequent source of confusion.

This PR therefore tries to establish a clear distinction between:

  • Key fingerprints that help to uniquely identify a node (along with its DepKind), and are typically a hash of the query key
  • Value fingerprints that help to determine whether a node can be marked green (despite having red dependencies), and are typically a hash of the query value

There should be no change to compiler behaviour.

r? nnethercote (or compiler)

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 17, 2026
@Zalathar
Copy link
Member Author

@rust-bors

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2026

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.

Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good distinction to make. I want to check my understanding.

  • This PR distinguishes key fingerprints from value fingerprints primarily by changing the names of variables and fields, rather than by types.
  • In practice, PackedFingerprint is usually (always?) used for key fingerprints and Fingerprint is usually (always?) used for value fingerprints.
  • KeyFingerprintStyle does encode key-ness in the type system, because it only applies to key fingerprints.

Is that right? Is it worth pushing further to distinguish key vs. value in the type system? I see that Fingerprint has uses outside of the query system, so maybe that makes it difficult?

View changes since this review

/// previous incremental-compilation session.
///
/// Some nodes don't have a meaningful value hash (e.g. queries with `no_hash`),
/// so they store a dummy value here instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the dummy value zero?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice I think it is, but I wasn't sure if I should commit to that in this comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up adding (e.g. Fingerprint::ZERO) to give context while also hedging against it not being accurate in the future.

@Zalathar
Copy link
Member Author

  • This PR distinguishes key fingerprints from value fingerprints primarily by changing the names of variables and fields, rather than by types.

Correct.

  • In practice, PackedFingerprint is usually (always?) used for key fingerprints and Fingerprint is usually (always?) used for value fingerprints.

Roughly true, but mostly for circumstantial reasons.

PackedFingerprint exists to eliminate alignment-padding in large lists of structures that would otherwise contain a Fingerprint, which has 8-byte alignment.

Currently it is only used in DepNode, which stores a key fingerprint alongside DepKind, and key fingerprints are almost never manipulated outside of a DepNode.

There are currently no uses of PackedFingerprint for value fingerprints, but only because there are currently no known-profitable places to use it.

  • KeyFingerprintStyle does encode key-ness in the type system, because it only applies to key fingerprints.

Yes, KeyFingerprintStyle is mostly (exclusively?) used to determine whether and how we can recover a key value from a key fingerprint, so it would make no sense for value fingerprints.

Is that right? Is it worth pushing further to distinguish key vs. value in the type system? I see that Fingerprint has uses outside of the query system, so maybe that makes it difficult?

If we were to try this, I would probably just add wrapper newtypes for key and value fingerprints.

@nnethercote
Copy link
Contributor

If we were to try this, I would probably just add wrapper newtypes for key and value fingerprints.

Might be worth doing in a follow-up.

r=me after my first two comments above are considered.

@Zalathar
Copy link
Member Author

Zalathar commented Feb 18, 2026

  • I did a trial rebase of Bring back enum DepKind. #152747 on top of this, and the few textual conflicts were easy to fix, so I think it should be OK to put this one in the queue now.

@bors r=nnethercote

@rust-bors
Copy link
Contributor

rust-bors bot commented Feb 18, 2026

📌 Commit b015d57 has been approved by nnethercote

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2026
Zalathar added a commit to Zalathar/rust that referenced this pull request Feb 18, 2026
Rename dep node "fingerprints" to distinguish key and value hashes

In the query system's dependency graph, each node is associated with two *fingerprints*: one that is typically a hash of the query key, and one that is typically a hash of the query's return value when called with that key.

Unfortunately, many identifiers and comments fail to clearly distinguish between these two kinds of fingerprint, which have very different roles in dependency tracking. This is a frequent source of confusion.

This PR therefore tries to establish a clear distinction between:

- **Key fingerprints** that help to uniquely identify a node (along with its `DepKind`), and are typically a hash of the query key
- **Value fingerprints** that help to determine whether a node can be marked green (despite having red dependencies), and are typically a hash of the query value

There should be no change to compiler behaviour.

r? nnethercote (or compiler)
rust-bors bot pushed a commit that referenced this pull request Feb 18, 2026
Rollup of 20 pull requests

Successful merges:

 - #145399 (Unify wording of resolve error)
 - #150473 (tail calls: fix copying non-scalar arguments to callee)
 - #152637 (Add a note about elided lifetime)
 - #152729 (compiler: Don't mark `SingleUseConsts` MIR pass as "required for soundness")
 - #152751 (Rename dep node "fingerprints" to distinguish key and value hashes)
 - #152753 (remove the explicit error for old `rental` versions)
 - #152758 (Remove ShallowInitBox.)
 - #151530 (Fix invalid `mut T` suggestion for `&mut T` in missing lifetime error)
 - #152179 (Add documentation note about signed overflow direction)
 - #152474 (Implement opt-bisect-limit for MIR)
 - #152509 (tests/ui/test-attrs: add annotations for reference rules)
 - #152672 (std: use libc version of `_NSGetArgc`/`_NSGetArgv`)
 - #152711 (resolve: Disable an assert that no longer holds)
 - #152725 (Rework explanation of CLI lint level flags)
 - #152732 (add regression test for 147958)
 - #152745 (Fix ICE in `suggest_param_env_shadowing` with incompatible args)
 - #152749 (make `rustc_allow_const_fn_unstable` an actual `rustc_attrs` attribute)
 - #152756 (Miri: recursive validity: also recurse into Boxes)
 - #152770 (carryless_mul: mention the base)
 - #152778 (Update tracking issue number for final_associated_functions)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants