Expose on statement handler in runtime config

[devbugging]

Issue to be solved

Providing custom statement handlers to the runtime is currently not possible.

Suggested Solution

Exposing the statement handler to config could be beneficial for passing any kind of custom statement handlers, which one such use case would be the execution debugger I'm working on, but also I feel the coverage reporter is another use case. Currently, the coverage report was added to the config https://github.com/onflow/cadence/blob/cc5644b671468ee11075b1a81cd1d7797a895b2a/runtime/config.go#L37 and then the runtime environment depends on that type and has control to decide what to do with it https://github.com/onflow/cadence/blob/ce732a847ddf31e9f07ea3d5414ea5d0707e7450/runtime/environment.go#L609 I think the inversion of control could be done and have config define a statement handlers slice, which would be defined by an interface having OnStatement, then the newOnStatementHandler could just make sure the handlers are wrapped inside of the returning interpreter.OnStatementFunc. This would also make the runtime config more configurable and contain less exposed members, as if adding different members for each use case where we need to have an action on execution of a statement.

[turbolent]

Yeah, this could be added and would allow for more flexibility for embedders.

So far the principle was to only expose high-level features to the embedder, e.g. dedicated functions to execute scripts and transactions, debugging functionality, etc. instead of exposing low-level functionality, as that keeps the surface area smaller and allows the implementation to be library refactored.

For example, the runtime API does not* expose the interpreter or sema package, which allows them to be implementation details. This allows them to be refactored or even replaced (compiler/VM) in the future, without causing downstream breakage.

I'm a little bit wary of breaking with this principle and exposing implementation details, but it is likely not a big deal.

[devbugging]

I understand that makes sense. What would you say is a better/alternative way to implement it? by it, I mean the ability to have a way to gather more information about the execution of each statement. In the case of the debugger that would be the usage of computation efforts per statement, and more. You can see the current implementation of that here https://github.com/onflow/execution-debugger/blob/main/remote_debugger.go#L141 Which is based on a Cadence branch that allows providing the on statement handler. We could even have a branch with this feature if it's something you don't feel comfortable having on master.

[m-Peter]

@turbolent Could you assign this one to me?

[devbugging]

@m-Peter I might need to be involved in this one as I need it for the execution debugger. We can work on it together if you want to. But, I believe @turbolent has valid concerns here, so I wouldn't want to push for this, we need to align or agree on possible approaches to solving this.

[devbugging]

Also @turbolent and I might be off on this one, but couldn't debugger then just implement this pattern as well, providing its statement handler instead of calling it here https://github.com/onflow/cadence/blob/master/runtime/interpreter/interpreter_statement.go#L49

[m-Peter]

@sideninja Fair enough, I was just offering to do this, as part of the ongoing grant proposal that we have regarding code coverage.

[devbugging]

@sideninja Fair enough, I was just offering to do this, as part of the ongoing grant proposal that we have regarding code coverage.

No for sure, I just wanted to say I would like to first agree with @turbolent on how to even proceed, and then for us to agree on how it should be implemented because I also have some requirements for my use case. Also don't let this block you as it might take some time to align. Let's keep the convo and see where we land. Is that good with you?

[m-Peter]

Yes, of course! Sounds like a plan! It's not even a blocker for the grant proposal, it's just something that Deniz had suggested in another PR and I was going to take a look at it.

[devbugging]

it's just something that Deniz had suggested in another PR

Nice, didn't know he had a similar idea.

[m-Peter]

I think it resembles what is described here: https://github.com/onflow/flow-emulator/pull/332#issuecomment-1491757454. Just for reference, I am posting it here too.

[devbugging]

After the discussion on Friday, we've agreed it could be beneficial to extend the Runtime interface to expose handlers for handling statements, functions, and expressions. The handlers shouldn't however expose implementation details, which expose the interpreter. Bellow, I will just make a draft outline of the possible methods but of course, I feel @turbolent and @SupunS should provide define it further:

OnStatement(statement ast.Statement)
OnExpression(expression ast.Expression)
OnFunctionDeclaration(function ast.FunctionDeclaration)

Some follow-up:

• should the function be part of the runtime interface or config runtime uses?
• should we add activations as an argument to the above handlers?

[devbugging]

Reviving this thread a bit so we come to a final proposal for changes in the interface. cc @m-Peter @bluesign @m-Peter and I briefly discussed the idea of including some metadata in the handlers that would provide additional information about execution (such as metering data).

[bluesign]

I think exposing stuff from FVM can be very nice here instead of Cadence.

[devbugging]

Yes, but how do you propose that would happen? FVM exposing handler methods?

[bluesign]

I was more in line thinking with FVM allowing me to set up my custom meter. And exposing default meter implementation.

[m-Peter]

Btw totally off topic but it would be nice to expose CoverageReport via interface, something like:

type LineHitReporter interface {
  AddLineHit(location Location, line int) 
}

so people can add different coverage modes. (without making PR to cadence) But actually what would be better is to put OnStatement handler to runtime config. https://github.com/onflow/cadence/blob/5bfce97a565740cd0257311a4bb339e4c258b75a/runtime/environment.go#LL147C3-L147C14

This way coverage report doesn't need to use this : https://github.com/onflow/cadence/blob/5bfce97a565740cd0257311a4bb339e4c258b75a/runtime/environment.go#L608

can take this function to coverageReport and hook it to OnStatement handler. https://github.com/onflow/cadence/blob/5bfce97a565740cd0257311a4bb339e4c258b75a/runtime/environment.go#L613-L622

I am posting here, for ease of reference, the feedback of @bluesign, made on a past PR for Flow Emulator.

[devbugging]

I was more in line thinking with FVM allowing me to set up my custom meter. And exposing default meter implementation.

After talking with @janezpodhostnik it's clear we could get access to the meter and possibly other FVM env stuff. Having this and the provided handlers as well as the metering fixes done in this https://github.com/dapperlabs/cadence-private-issues/issues/64 we could profile costs per statement.

@turbolent are you ok with the suggested interface changes, I can make a PR.