interp: produce meaningful output for file statements

[firelizzard18]

This MR modifies interp.eval to produce meaningful result values for file statements. Currently, the return value of interp.eval for file-like input is effectively new(interface{}) (as a reflect.Value). With these changes, interp.eval for file-like input will return meaningful values.

• At the top level, a file-like input will produce a struct with an array of statement results
• Import specs will produce a struct with the import path, and name if specified
• Top-level function declarations will produce a struct with the function name
• Top-level type declarations will produce a struct with the type name

Variable and constant declarations do not currently produce any result.

Motivation

I am working on a notebook extension for Visual Studio Code, powered by Yaegi. This MR is a step towards enabling me to improve the user experience. Specifically, I want to be able to intelligently detect whether the user expects the evaluation result to be displayed. If the input is file-like, I will assume that the user does not wish to inspect/display some value. For my purposes, it doesn't matter exactly what result file-like inputs produce (as long as I can detect it), so I chose to implement something simple that could be extended in the future.

[mvertes]

I have the following concerns with your code modifications:

• They introduces new external dependencies, and at this moment we have none, beside the Go standard library. Unless an ultra compelling reason, we will not change that.
• The change you propose in eval() affects the behaviour of all eval() calls by adding a walk on the AST (which has a cost, specially for huge codebases), no matter the context, for no benefits outside of your use case. It's a sign that the approach is probably not the right one.

I understand that you need a better logic to act on the output of Eval, and decide what to do with the results. The current approach in yaegi is very naive, incomplete and is not always useful or accurate, but ok (to a point) for a simple REPL.

What I propose is to improve the API of the interpreter, by providing additional methods, functions and types (if necessary) to ease interpreter inspection, based on standard and stable types, without interfering with eval.
What is available now is the interp.Symbols() method which retrieves the map of exported symbols of an interpreter session. What is missing ? Created globals, types, namespaces (aka packages) ?

[firelizzard18]

@mvertes My purposes would be satisfied by an API that let me inspect the AST before it is executed. The simplest change would be a way to evaluate AST directly:

import "go/ast"
func EvalAST(ctx context.Context, file *ast.File) (reflect.Value, error)

Alternatively, exposing Yaegi's AST would open the door for significant future development - but it would be a much larger API change:

func Parse(src, name string, incremental bool) (pkgName string, root Node, err error)
func EvalAST(ctx context.Context, root Node) (reflect.Value, error)

This second approach would require exporting the node type, or creating a Node interface. For my purposes, I would need at a minimum:

type Node interface{
    Child() []Node
    Kind() NodeKind
}

Either of these approaches would obviate my need for FileResult (this PR) and BlockResult (#1081). That being said, I don't like (*interface {})(0xc0001162e0)(<nil>) as an output (effectively new(interface{}), as formatted by spew.Dump). Returning a pointer to a nil interface{} seems a rather weird thing to do. IMO, nil would be better.

They introduces new external dependencies, and at this moment we have none, beside the Go standard library. Unless an ultra compelling reason, we will not change that.

The dependency I added is test-only. Removing it would not be difficult.

adding a walk on the AST... It's a sign that the approach is probably not the right one.

I did not think about how that would affect large inputs. If I spend some time getting a better understanding of what the AST looks like, I could certainly rewrite this to be much lower impact.

[firelizzard18]

@mvertes Would you be open to a change like the one I mentioned? If so, would you prefer consuming go/ast or exposing Yaegi's AST?

[mvertes]

The internal *node type and ast stuff is not meant to be exported. They are still potentially subject to redesigns. Even the reliance on the go ast and parser package is something that I consider to be potentially replaced, and I'm reluctant to commit on it at API level.

Let me expose my view.

I consider the interpreter to be a virtual runtime (part of a larger runtime in the host process), which can be interacted with by providing inputs in form of Go instructions. To me the only stable input format is a string containing Go language as per the Go specification. Then instructions can come at once, or incrementally but I think this is correctly addressed so far, at least at input level (ast here is just an internal representation derived from the source, which remains the sole original intent).

The problem, and the discussions we have is more about the outputs.

First, an interpreter has a state, which persists between Evals. It consists of existing all packages, types, constants, variables and functions defined in the interpreter and accessible from the global scope. A call to eval makes the state evolves, but the output of Eval can not always reflect all the state changes, as many intermediate changes and side effects can occur, even for seemingly simple statements.

So we don't have to necessarily change the API of Eval, but we need to provide a mean to discover and inspect the interpreter state between Evals. By differing the state before and after an eval, you will have a much more reliable image of what has changed, rather than specialising the output of individual statements, as you propose (imports can be triggered in cascade, add symbols, funcs. etc).

So, to me, an output of Eval can never be guaranteed to be complete and accurate, due to cascading effects, side effects, etc. It's better to provide access to the interpreter state, and analyse delta between evals, or, if you know what you are looking for, i.e. you have your own AST on your side, for a var declaration, you just access this particular object, using something like: var v reflect.Value = interp.Global("main.myVar").

So the first step on our side would be to quickly provide those inspection tools.

[firelizzard18]

So we don't have to necessarily change the API of Eval, but we need to provide a mean to discover and inspect the interpreter state between Evals. By differing the state before and after an eval, you will have a much more reliable image of what has changed, rather than specialising the output of individual statements, as you propose (imports can be triggered in cascade, add symbols, funcs. etc).

Diffing the state before and after an Eval does not strike me as scalable. If the state tree reaches a non-trivial depth and/or width, storing the before state and comparing the two states will quickly become very expensive. I am not interested in investing in the engineering required to efficiently track and compare large state trees.

So, to me, an output of Eval can never be guaranteed to be complete and accurate

I don't need complete or accurate. Best-guess is fine for my purposes.

Without either the ability to interrogate the input AST or more information on the output-side, I think my most realistic option is to parse the input myself (with go/...), and hand sanitized inputs to Eval. Unless I'm missing something significant, state tree inspection, tracking, and comparison would be prohibitively expensive in terms of performance and engineering, for my purposes.

[mvertes]

I think I have a solution to improve eval and allow to customise results close to what you originally proposed, while preserving performances in batch mode and giving enough flexibility. I need to do some modifications in runCfg first, I started a PR for that which should land in a couple of days.

[mvertes]

@firelizzard18, could you resubmit your PR with the following changes:

• no external dependency added
• the added block in interp/interp.go:530 to be in a separate function, like func getFileResult(n *node) FileResult
• the insertion of a guard if !inc { return res, err } close to the end of eval in interp.go, to disable result processing if eval is not called in incremental mode
• the call of res := getFileResult(root) to be inserted after the above guard. It should avoid penalty of result processing for batch execution.

Otherwise data structures and result.go seems fine to me,

Thanks

[firelizzard18]

@mvertes I removed the external dependency and moved the logic to func getFileResult(n *node) *FileResult. I also updated the walk callback such that its performance impact should be much less or maybe even negligible. I return false for everything except fileStmt, importDecl, and typeDecl.

• the insertion of a guard if !inc { return res, err } close to the end of eval in interp.go, to disable result processing if eval is not called in incremental mode
• the call of res := getFileResult(root) to be inserted after the above guard. It should avoid penalty of result processing for batch execution.

The motivation of this feature is to improve the Go notebooks user experience. In that context, I am evaluating blocks of code with Interpreter.Eval, so checking if !inc would defeat the purpose of this PR.

[firelizzard18]

@mvertes This is ready for review

Alternatively, if #1225 is merged, I could implement an equivalent (for my purposes) interface on interp.Program. Though it still seems like EvalPath should return something meaningful.

[firelizzard18]

@mvertes Since `Interpreter.CompileAST has been added I no longer have a need for this, so I'm going to close the PR. If this is still something you would like merged, I can reopen and update it.

[mvertes]

Ok, thanks. If something from it is needed in the future, let's just create a new PR.