Closed sauclovian-g closed 2 months ago
To respond to the four concerns you raise in your commit message:
- The type matching in indexToFOV in FirstOrder.hs isn't working correctly.
I presume you're referring to this comment?
If so, then I think you can (and should) make the types the same. That being, this function will still need to be partial, since IndexLit
only has cases for BaseIntegerType
and BaseBVType
. (The fact that there aren't IndexLit
s for other types is a bit curious, but I don't think we need to solve that problem just yet.)
There is similar code in what4
dealing with IndexLit
s that is also partial (e.g., here), so would be in line with how this is handled elsewhere.
- Fixing FirstOrderValue arrays (FOVArray) to contain actual values changes the associated JSON schema and it's not clear what the ramifications of that are.
I think this is fine. It is not at all clear to me what the previous implementation of FOVArray
was meant to do, and I think your version is a strict improvement. From that perspective, I think it's fine to update the JSON schema for FOVArray
s as well. The JSON schema is (AFAIU) only used as part of SAW's caching mechanism.
- Fixing the conversion to Term changes the behavior of anything that exercised that code path. It's not clear what ramifications that has either, although since the prior behavior was to mysteriously generate types instead of values, it's fairly unlikely anything important depends on it.
Similarly, I have no idea what the previous code path was trying to do—it seems rather broken. I'd be much happier with something resembling the new code in this PR.
- Currently if we get an ArrayMapping GroundValue array (one where you can only get values out by calling a lookup function) we fail. It's not clear if these ever appear or what should actually be done if one does.
It is absolutely possible for what4
to produce an ArrayMapping
value when producing a GroundArray
. Here is a proof of concept:
Running this yields:
Satisfiable, with model:
ArrayMapping
idx27 := 27
arr27 := True
idx42 := 42
arr42 := False
That being said, I don't have a clear idea of how we'd represent ArrayMapping
values in SAWCore. I think it would be fine to fail for now if we encounter one.
I force-pushed to squash the fix commit. This can be reviewed now, though it shouldn't be merged until the release goes out.
(also I think all the commits should get squashed before being merged; let me know if you disagree)
First force was to rebase on head, second to squash it all together. This should be ready to go unless it randomly explodes in the CI.
hmm, I tagged this "needs test" and then didn't write a test, guess I should do that
I have squashed it again and I'll merge it after I make lunch unless it blows up for some reason.
See commit for notes. Fixes #2049.