google / go-jsonnet

Apache License 2.0
1.64k stars 236 forks source link

go-jsonnet reports static errors in tla-codes imports differently to c++ #391

Open ekimekim opened 4 years ago

ekimekim commented 4 years ago

Suppose I have a top-level function which takes an arg (trivial function used here for simplicity):

function(foo) foo

Now suppose I specify a top-level arg using tla-code that imports some other file: --tla-code 'foo=import "foo.jsonnet"'

However, that file contains a syntax error:

$ cat foo.jsonnet 
not valid

Here is the result of running this with c++ jsonnet:

$ jsonnet --tla-code foo='import "foo.jsonnet"' -e 'function(foo) foo'
STATIC ERROR: foo.jsonnet:1:5-10: did not expect: (IDENTIFIER, "valid")

Here is the result with go-jsonnet:

$ go-jsonnet --tla-code foo='import "foo.jsonnet"' -e 'function(foo) foo'
RUNTIME ERROR: foo.jsonnet:1:5-10 Did not expect: (IDENTIFIER, "valid")
    <top-level-arg:foo>:1:1-21  $
    <cmdline>:1:15-18   function <anonymous>
    Top-level function  

The same behaviour occurs if you replace --tla-code foo='import "foo.jsonnet"' with --tla-code-file foo=foo.jsonnet.

sparkprime commented 4 years ago

We didn't ever try to make the error messages match exactly, on the grounds that they were not intended to be machine readable and making the stack traces match exactly would have constrained the Go implementation's design considerably.

ekimekim commented 4 years ago

A word-for-word match, sure. But I was surprised to find something that was a static error in one was a runtime error in the other.

ekimekim commented 4 years ago

Though now you mention it, machine readable stack traces would be really useful. My main problem with the current output in the go version is that due to how we're importing the filename the user passes in, there's a traceback with a bunch of noise they don't care about - the error is in their file. If I were able to parse the traceback, determine it was a static import error, then only present that error message to the user it'd be very helpful.

sbarzowski commented 4 years ago

Not sure what to do about this. It is a runtime error in the sense that this file was imported during evaluation (and you technically can have code like if something then import 'foo.jsonnet' else null and in such cases you want to know where this import is coming from).

ekimekim commented 4 years ago

Why does that not happen in the c++ version? Are TLAs strictly evaluated there vs lazily here? But yes, agreed that in the general case you do want to know where the import is coming from.

Is there a way to test if a file is syntactically valid without evaluating it? Then we could check the file beforehand and give a more useful error message for that case.

sbarzowski commented 4 years ago

Semantically it's the same as in C++. The C++ implementation just drops the stack traces in such cases.

And of course files could be parsed beforehand. But we should not do it in the interpreter - the imports are specified to be lazy - it doesn't matter what is there until you use it. This is one of the things that upcoming linter will help with (it traverses the imports and checks all of them for various errors that can be caught statically).

ekimekim commented 4 years ago

But we should not do it in the interpreter

Yes, sorry. Poor choice of words on my part. I meant that the tool I'm writing (using the python bindings) could do it, since the stack trace being a problem is specific to my usage. The linter also sounds promising.

sbarzowski commented 4 years ago

FYI you can do the parsing without evaluation in go-jsonnet (SnippetToAST or ImportAST depending on whether you want to provide the source code yourself or import it). I'm afraid it's not available (yet) in Python or C library, though.

sparkprime commented 4 years ago

This doesn't really have anything to do with TLAs:

dcunnin@dcunnin:~$ cat bad.jsonnet 
{
dcunnin@dcunnin:~$ jsonnet -e 'import "bad.jsonnet"'
STATIC ERROR: bad.jsonnet:2:1: unexpected: end of file while parsing field definition
dcunnin@dcunnin:~$ go-jsonnet -e 'import "bad.jsonnet"'
RUNTIME ERROR: bad.jsonnet:2:1 Unexpected: end of file while parsing field definition
        <cmdline>:1:1-21        $
        During evaluation
sparkprime commented 4 years ago

I'd argue the Go behaviour is preferable in that case at the very least.