feat(ssa): Generic ValueId to show resolved status

[aakoshh]

Description

Problem*

Followup for #6448

Currently the ValueId is an alias for Id<Value>. Throughout the codebase we either call let id = dfg.resolve(id); before comparing them, or using them to look up values in dictionaries, or we don't. It's hard to tell when it's okay not to call it, and it's easy to forget to do so.

Summary*

• The PR makes the ValueId wrapper of Id instead of an alias, and adds a generic phantom type to it which is either Resolved or Unresolved, so that we can only directly compare ValueId<Resolved> instances but not ValueId<Unresolved> ones.
• An unresolved_eq method is added to ValueId<Unresolved> to make it easy to find in the code where we are currently comparing raw values.
• The DataFlowGraph can at the moment be indexed by ValueId<Unresolved> because that's how the code did it, without resolution. Not sure if this is correct, but removing this indexer would bring to light all the cases for investigation, or we can add auto-resolve in the indexer.
• ValueId is used in the data structures such as Instruction and Expression, with the default Unresolved value, so that the IDs always have to be resolved before equality checks.
• Hash and Eq are derived for ValueId if the generic parameter implements them.
• Instruction and Expression were also made generic, and with their map_values methods they can map over the ValueIds in them to become e.g. Instruction<Resolved>, which makes them implement Hash and Eq, so they can be directly compared to each other.
• There is a Resolution trait implemented for Resolved and Unresolved so we can treat them generically and decide at runtime whether we need to resolve the ID or not. This is used when the same method is called both with ValueId and ResolvedValueId; this way we can save an extra resolution (although it would be a short one).

In this PR I tried to preserve the current behaviour of the program, assuming that what it does is correct, but I hope that with the new ability to differentiate between these states we can spot more easily when we're doing something wrong, and we can gradually move towards more safety.

Additional Context

There are a lot of little changes to go from ValueId to RawValueId (which is just Id<Value>, used in maps), which is annoying, but I hope there is some tangible benefits to this anyway. It's difficult to reason about what should be turned into ResolvedValueId (sometimes resolutions are defensively done by the caller and the callee as well), so I left most APIs as-is.

To help with the review, the meaningful changes are in value and dfg, with the rest just correcting compilation errors. The changes in instruction are mostly just moving methods around in a new generic impl to make them available for both statuses, not just the default one.

One pervasive pattern is accessing dfg[*id] with an unresolved ID and using the returned Value. Ostensibly this value could have already been replaced by a successor. It's hard to tell whether this is intentional or not, but we now have the option to weed out these and replace them either with a call to dfg.resolve(id) or forcing a promotion with id.resolved() if we're sure it's the right thing.

Alternatives

Removing set_value_from_id

Another option would be to remove the ability to just replace a value with a successor and leaving a forward reference to the new value, which will later have to be followed recursively every time we encounter a potentially stale ID, and instead to visit every occurrence of the old ID and replace it with the new one.

Without generics

Earlier I tried a different approach where ValueId was a non-generic wrapper around Id<Value> without implementing Hash, and the ResolvedValueId was the alias for what it is today. Crucially the data structures carry ValueId, because they can be stale. I worked for hours to go through the compilation errors, then hit a spot where either Instruction or Expression is compared with assert_eq! or used in maps as keys, which didn't compile because I had to remove the Hash, Eq, PartialEq as they contain ValueId. Implementing them just for these types manually is possible, but cumbersome.

Then I discovered the various map_values methods and thought it would make sense to add the generic parameter to everything that can carry a ValueId, and then they can be compared after resolving all IDs first, because Rust can still derive Hash and Eq as long as the marker implements it, which is what happens in this PR.

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[asterite]

Looks great! 👏

I left one question about unresolved values, and some suggestions to use ResolvedValueId a bit more but that could be left as a follow-up PR. It's great that now we can see where we are comparing raw values so we can later more easily spot bugs (and prevent bugs) or tackle them before-hand before finding those bugs.

[jfecher]

How do we handle the case where we resolve a value id to get a ResolvedValueId, with id 42 lets say, then we call dfg.set_value_from_id to map value 42 to value 100? I think we'd still have a value id that says it is resolved but actually needs to be resolved again.

My main worry is that this change makes ValueIds more cumbersome to use in the future while also not entirely preventing the issue - although I think it'd at least help prevent against forgetting to call resolve.

[aakoshh]

I suppose we could remove Copy from ResolvedValueId and the set_value_from_id method can consume it.

[jfecher]

I don't think that solves the issue, e.g. if the ValueId in question is stored in a map to be used later

[aakoshh]

You are right, that is a fundamental conundrum of storing value IDs in data structures. We had this discussion with @asterite as well: it seems like in brillig_gen it's okay to use ResolvedValueId in maps because we're no longer replacing IDs, but before that it doesn't make sense, which is why I use RawValueId everywhere else. We could introduce a FinalValueId that has hash I suppose, but Expression is used as a cache key in mem2reg and Instruction in constant_folding; here I assume we know that the IDs won't be replaced during the lifetime of these maps. They are probably not final, though, depends on what passes come later.

It's not a silver bullet, but as you said it is a step in the right direction of preventing certain bugs, and as I tried to say, it at least gives us an option for tightening the screws.

I have spent almost 2 hours resolving the recent merge conflicts though; perhaps we can improve the solution in a later PR? Or do you think it's more cumbersome than worth?

I'll try to see what removing the Copy does; at least it would prevent a variable from being used again in the same method when we just learned that it's no longer the up-to-date one.

[aakoshh]

Trying to remove Copy results in over 400 compilation errors, because to make it work I started changing methods like DataFlowGraph::get_numeric_constant to take references, to not move values for read-only purposes, but then everywhere we pass dfg.get_numeric_constant(*value_ref) has to change to remove the *. I would do it, but only if we think that solves anything.

Also checked that set_value_from_id is only called with unresolved value IDs at the moment, so we wouldn't see the benefit for that particular example. Looking at the call sites, sometimes it's called with something returned by make_constant and other make_ methods which I suppose are resolved when they are created, but this isn't always obvious, so I'm not sure it should ask for ResolvedValueId.

I wonder if there was a way to limit via lifetimes how long a ResolvedValueId can be used for 🤔

[aakoshh]

Perhaps we can try something like this:

• Remove Hash from Resolved so it cannot in general be used in data structures, but leave Eq so that we can compare instances
• Where it needs to be used in data structures, we can add module specific status types that implement Hash, but are inaccessible elsewhere. For example we can use one exclusively in constant_folding::Context and another one inside brillig_gen. They would need some conversions, but it would prevent accidentally using the one returned by dfg.resolve in maps and risk them becoming stale.

[aakoshh]

Removed Hash from Resolved. Now you really have to go out of your way to use them as keys." Of course if you store them in fields and then compare stale values then it can't help with that. I'll check if I could return a reference from resolve which is more difficult to store.

Later added them back in favour of the lifetimes. I figured there is no difference in using them as keys vs values in maps; the lifetime on the other hand makes you work hard to store it in any data structure not tied to the same reference.

[aakoshh]

Added a lifetime to Resolved as well, so it's very difficult to accidentally store it even in non-key positions.

[jfecher]

Or do you think it's more cumbersome than worth?

I am thinking that, unfortunately. It is more cumbersome to have to shuffle around the types like this and I don't think the lifetimes solve the root of the issue either. Ultimately it's still up to the dev to ensure they're handling them correctly but this is made a bit more difficult I think by most passes still claiming to store RawValueIds when they should be conceptually resolved (e.g. the normalize_value_ids pass), so I'm not sure the extra types help readability or understandability of the passes. It feels bad to reject such large work but I do think it was a valuable exploration into this issue.

[aakoshh]

Ultimately it's still up to the dev to ensure they're handling them correctly

Agreed, we cannot stop a determined developer to store the IDs. I merely wanted to stop them to forget to do the right thing by accident.

but this is made a bit more difficult I think by most passes still claiming to store RawValueIds when they should be conceptually resolved

I used RawValueId because I lack the context to tell which pass should use ResolvedValueId and which ones should not. I was hoping that further iterations can build on this foundation and improve the code by opting into ResolvedValueId where possible.

Oh well, at least I can stop keeping it in sync 😌