Simplify wildcard datastructure. #4851

royAmmerschuber · 2026-02-05T14:14:56Z

Makes it so that each node only needs to count all local exposed nodes instead of also keeping track of foreign nodes. This makes the update logic significantly simpler.

rustbot · 2026-02-05T14:15:00Z

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

RalfJung

That's a very nice overall code reduction :)

View changes since this review

RalfJung · 2026-02-09T11:09:58Z

src/borrow_tracker/tree_borrows/tree.rs

-    ///
-    /// NOTE: same guarantees on entry initialization as for `perms`.
-    pub wildcard_accesses: UniValMap<WildcardState>,
+    /// Stores how nodes are related to exposed nodes,


Suggested change

/// Stores how nodes are related to exposed nodes,

/// Stores how nodes are related to exposed nodes.

RalfJung · 2026-02-09T11:11:58Z

src/borrow_tracker/tree_borrows/tree.rs

@@ -1180,14 +1186,7 @@ impl<'tcx> LocationTree {
        // as a foreign access, and if not, then we can error.
        // In practice, all wildcard trees accept foreign accesses, but the main tree does
        // not, so this catches UB when none of the nodes in the main tree allows this access.


This comment is outdated now. Why did you change this to instead pass around a boolean?

Previously we checked the max_foreign_access field which would be set to Write on wildcard roots. This PR removes max_foreign_access from WildcardState so we can't use that anymore.

Thanks, makes sense. Please update the comment then.

RalfJung · 2026-02-09T11:13:01Z

src/borrow_tracker/tree_borrows/tree.rs

+                }
+            } else {
+                relatedness
+            }


This seems to move some of the logic of determining the relatedness here into tree.rs. Seems better to have that in a helper in wildcard.rs like before.

RalfJung · 2026-02-09T11:14:25Z

src/borrow_tracker/tree_borrows/wildcard.rs

-    /// that is foreign to this node.
+/// Only gets allocated if we add an exposed node.
+#[derive(Clone, Debug, Default, PartialEq, Eq)]
+pub struct WildcardData(UniValMap<WildcardState>);


Please generally define types before using them. WildcardState as at the end of the file so currently one has to jump around a lot to understand this code.

RalfJung · 2026-02-09T11:14:39Z

src/borrow_tracker/tree_borrows/wildcard.rs

+    /// How many local nodes are exposed with read permissions.
+    local_reads: u16,
+}
 impl Tree {


Suggested change

impl Tree {

impl Tree {

RalfJung · 2026-02-09T11:21:20Z

src/borrow_tracker/tree_borrows/wildcard.rs

+            // When the first tag gets exposed then we initialize the
+            // wildcard state for every node and location in the tree.


This doesn't seem to be correct? We're only initializing it from the exposed node upwards.

RalfJung · 2026-02-09T11:23:29Z

src/borrow_tracker/tree_borrows/wildcard.rs

+                let old_access_type = perm.strongest_allowed_local_access(true);
+                let access_type = perm.strongest_allowed_local_access(false);


Please explain those boolean arguments.

RalfJung · 2026-02-09T11:24:53Z

src/borrow_tracker/tree_borrows/wildcard.rs

+                    .map(|id| wildcard_accesses.0.get(id).cloned().unwrap_or_default())
+                    .fold((0, 0), |acc, wc| (acc.0 + wc.local_reads, acc.1 + wc.local_writes));
+                let expected_reads =
+                    child_reads + Into::<u16>::into(exposed_as >= WildcardAccessLevel::Read);


Suggested change

child_reads + Into::<u16>::into(exposed_as >= WildcardAccessLevel::Read);

child_reads + u16::from(exposed_as >= WildcardAccessLevel::Read);

Same below

RalfJung · 2026-02-09T11:28:00Z

src/borrow_tracker/tree_borrows/tree.rs

@@ -451,19 +445,6 @@ impl<'tcx> Tree {
            }
        }



Suggested change

// We don't have to update `wildcard_accesses` as the new node is not exposed and

// has no children so the default counts of 0 are correct.

RalfJung · 2026-02-09T11:32:11Z

src/borrow_tracker/tree_borrows/tree.rs

-    /// NOTE: same guarantees on entry initialization as for `perms`.
-    pub wildcard_accesses: UniValMap<WildcardState>,
+    /// Stores how nodes are related to exposed nodes,
+    pub wildcard_accesses: WildcardData,


The name of the field and the type are inconsistent.

And generally this is more about exposed nodes than about wildcard pointers, so maybe the name should reflect that? Something like exposed_cache: ExposedNodeCache or so? It's a cache in the sense that it just caches information that could also be recomputed.

I think it makes sense to change it to wildcard_cache as a name. I feel like exposed is a too general term considering this specifically tracks the relatedness for a wildcard access.

This also means the doc-comment is wrong as we only care about exposed nodes through which an access is possible.

This specifically tracks how many accessible exposed children every node has. So having "exposed" in the name seems quite apt to me?

simplify wildcard datastructure

2eed733

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Feb 5, 2026

royAmmerschuber added 2 commits February 5, 2026 16:14

fix consistency check

616fd84

remove as conversion

2d0044f

RalfJung reviewed Feb 9, 2026

View reviewed changes

changes because of code review

6ee1a97

	/// Stores how nodes are related to exposed nodes,
	/// Stores how nodes are related to exposed nodes.

		// When the first tag gets exposed then we initialize the
		// wildcard state for every node and location in the tree.

		let old_access_type = perm.strongest_allowed_local_access(true);
		let access_type = perm.strongest_allowed_local_access(false);

	child_reads + Into::<u16>::into(exposed_as >= WildcardAccessLevel::Read);
	child_reads + u16::from(exposed_as >= WildcardAccessLevel::Read);



	// We don't have to update `wildcard_accesses` as the new node is not exposed and
	// has no children so the default counts of 0 are correct.

Simplify wildcard datastructure. #4851

Are you sure you want to change the base?

Simplify wildcard datastructure. #4851

Conversation

royAmmerschuber commented Feb 5, 2026

Uh oh!

rustbot commented Feb 5, 2026

Uh oh!

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royAmmerschuber Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RalfJung left a comment •

edited by rustbot

Loading

royAmmerschuber Feb 10, 2026 •

edited

Loading