pyret-lang icon indicating copy to clipboard operation
pyret-lang copied to clipboard

Add reporting of LHS values when error occurs in RHS predicate

Open dbp opened this issue 3 years ago • 1 comments

This, at least partly, addresses #1633.

In particular, it adds a new case of TestResult, failure-exn-satisfies-rhs, which is used when it a satisfies test where an exception was trigger in the RHS. In those cases, we have a value on the LHS, so we should show it along with the exception. I didn't see tests of this kind of behavior, but please point me if there is a way to do it. The four cases that will trigger the new behavior are:

check:
  1 satisfies lam(v): raise("error") end
end

check:
  1 satisfies lam(v): raise("error") end because 1
end

check:
  1 violates lam(v): raise("error") end
end

check:
  1 violates lam(v): raise("error") end because 1
end

And the new error message looks like:

Screen Shot 2021-12-14 at 1 58 40 PM

I noticed, as I was doing this, that there may be other cases where there is more information that could be shown, e.g., if an error is triggered by the refinement of an is test, neither the left nor right value is shown, which it seems like they should be:

check:
  1 is%(lam(v1,v2): raise("error") end) 1
end

Of course, there are still more variations -- e.g., if the right side of an is test errors, the left side value could be shown, etc. Of course, maybe there is a way to make this visible in the stack trace which would subsume all of this? Though with the stack manipulation that's going on, perhaps that is expecting too much.

dbp avatar Dec 14 '21 19:12 dbp

Trying to reconstruct the original design logic: I think we'd thought that "if a test case unexpectedly errors, that itself is a bug and is the most salient issue to present", and didn't really consider the case where a weird LHS input could cause a logic error in the RHS and trigger an unexpected error. I think we might have to workshop the rendering of these error a bit -- your proposed rendering above is good in that the first thing you see is the red unexpected-error box, and the "given the LHS" part is secondary and less salient.

Given that design logic, I'm less keen on making lhs is rhs-that-errors render the lhs value, since there's no possible meaningful scenario where rhs-that-errors should error in an is test! @jpolitz agreed?

blerner avatar May 01 '24 22:05 blerner

@jpolitz just skimming this again -- worth merging as is, or should we workshop it a bit more?

blerner avatar Feb 14 '25 15:02 blerner

I think this improves things, no reason to delay merging it.

jpolitz avatar Feb 14 '25 20:02 jpolitz