IntersectMBO / plutus

The Plutus language implementation and tools
Apache License 2.0
1.56k stars 477 forks source link

The use of Builtins.error in PlutusTx.Ratio and PlutusTx.List causes horrible debugging experience #3003

Closed emiflake closed 5 months ago

emiflake commented 3 years ago

The PlutusTx.Builtins defines error:

{-# NOINLINE error #-}
-- | Aborts evaluation with an error.
error :: () -> a
error = mustBeReplaced "error"

However, the use of this error throughout the other PlutusTx functions, such as PlutusTx.Ratio.round causes what is essentially a black hole in error information. Should such an error arise anywhere, as a developer you get only this error, which points to mustBeReplaced:

This must be replaced by the core-to-plc plugin during compilation: error
CallStack (from HasCallStack):
  error, called at src/PlutusTx/Utils.hs:4:26 in plutus-tx-0.1.0.0-6EwthIoMENA5GRJ8xORyCp:PlutusTx.Utils

This is hard to debug, especially if there are multiple places that could be causing the error ... it's essentially a segfault. At the very least, the error information that was stripped from these base functions, should be added back in, even if it's only for off-chain code. Ideally, though, we would have a better stack trace than just pointing to this seemingly unrelated function, which may mislead the developer into thinking they are using a builtin they shouldn't be!

catch-21 commented 3 years ago

Thanks @emiflake. I have raised an issue internally to track this, albeit as low priority.

ghost commented 3 years ago

I guess it can be closed as #3618 is merged.

peter-mlabs commented 2 years ago

I am encountering this issue again; unless I'm mistaken, #3618 is targeting a different use case.

The error message

CallStack (from HasCallStack):
  error, called at src/PlutusTx/Utils.hs:4:26 in plutus-tx-0.1.0.0-6EwthIoMENA5GRJ8xORyCp:PlutusTx.Utils

is displayed when traceError is called in off-chain code; i.e., within the Contract monad. What this means is that a module like PlutusTx.Ratio, which uses traceError for things like division-by-zero, leads to the information black-hole as described by @emiflake. There is at least one other function that I assumed could be used off-chain that has this same behavior (unsafeFromBuiltinData).

Assuming that the intention for PlutusTx.Ratio is for it be able to be used off-chain as well, is there a way to have one of the following:

szg251 commented 2 years ago

Hey @ak3n ! It looks like this issue is still not fixed, could we reopen it?

ghost commented 2 years ago

@gege251 It seems it was fixed but I can spot error calls in PlutusTx.Ratio instead of traceError at the moment. New functions were added/old were updated I suppose. Yeah, I think it should be fixed again and we can reopen the ticket.

Speaking of @peter-mlabs's suggestions, maybe a better fit for them would be #3164 or a new ticket as it sounds a bit more general than fixes in PlutusTx.Ratio.

szg251 commented 2 years ago

@ak3n Just to make sure I understand it correctly, a primary solution would be to change wrong error calls such as this: https://github.com/input-output-hk/plutus/blob/master/plutus-tx/src/PlutusTx/Ratio.hs#L184 To traceError calls, such as this: https://github.com/input-output-hk/plutus/blob/master/plutus-tx/src/PlutusTx/List.hs#L139

peter-mlabs commented 2 years ago

So I don't believe switching traceError to error would resolve anything.

peter-mlabs commented 2 years ago

For instance:

ghci> PlutusTx.List.head []
*** Exception: This must be replaced by the core-to-plc plugin during compilation: error
CallStack (from HasCallStack):
  error, called at src/PlutusTx/Utils.hs:7:26 in plutus-tx-0.1.0.0-GXa9MFQdTJXE2N8Rh6GuAc:PlutusTx.Utils
michaelpj commented 2 years ago

I think it would be fine to provide a better implementation of error for off-chain code. The issue is trickier, in that it's hard to make traceError behave well both off-chain and on-chain. There isn't in fact an on-chain error function that takes a message - traceError is the combination of trace and error. We could, e.g. make trace call Debug.Trace off-chain, but that seems like it would be extremely annoying.

sjoerdvisscher commented 1 year ago

I got bitten by this today. Maybe the message can include something like: "If you see this, maybe you're using PlutusTx functions in off-chain code, and there has been an error".

Unisay commented 5 months ago

@gege251 It seems it was fixed but I can spot error calls in PlutusTx.Ratio instead of traceError at the moment. New functions were added/old were updated I suppose. Yeah, I think it should be fixed again and we can reopen the ticket.

The reason this ticket got re-opened is that PlutusTx.Ratio still contains error calls without a code (namely in the recip function). I am going to address this remaining concern, so that this task could be closed.

In case if there are other QoL suggestions coming out of this discussion I encourage participants to report them separately.

peter-mlabs commented 5 months ago

@Unisay, just want to re-iterate that #5880 won't improve the situation in the same way that #3618 didn't fix it (unless something else has changed in plutus-tx that im not aware of).

The problem is if you call the function in this offchain, you will still get the "must be replaced" message.

I think this is still important to keep open, because I've had three developers in the past 3 months find this issue via a search engine. The issue remains that if you error out in the offchain, you don't get a line number like you would with undefined or the error in base -- you just get a message that an error occured somewhere in your program, with no indication as to where.

This makes debugging something simple (like calling head on an empty list) into a very tedious task, and I think this issue should remain open until it is actually resolved.

Unisay commented 5 months ago

just want to re-iterate that #5880 won't improve the situation in the same way that #3618 didn't fix it

I see your point. Thanks for the clarification. I'll bring this "must be replaced" problem to the team's attention, stay tuned.

Unisay commented 5 months ago

The "horrible debugging experience" problem could be divided into these sub-problems: a) The stack trace isn't deep enough and doesn't lead to the user's code that called error. b) The error message isn't helpful as it doesn't reveal the reason.

We don't have a solution for a) as HasCallStack => is not supported by Plutus compiler (Unsupported feature: Kind: GHC.Types.Symbol) As for the b) - I have an improved error message that should help developers understand the underlying cause: https://github.com/IntersectMBO/plutus/pull/5883

Unisay commented 5 months ago

The improved error message PR got merged, I am closing this issue now.