Add a builtin trace function. #487

ahakanbaba · 2018-04-10T19:48:01Z

Prints the first argument to std err with line number
Returns the second argument which is expected to be an object.

ISSUE: #130

Prints the first argument to std err with line number Returns the second argument which is expected to be an object.

ahakanbaba · 2018-04-10T19:51:05Z

@sparkprime
Added a trace function that (I think) is similar to the haskell's trace function
From here:

"When called, trace outputs the string in its first argument, before returning the second argument as its result.'"

I have not added documentation or error tests yet.

As it stands the trace function expects an obj {} as its second argument. I am not sure how useful that is.

Maybe we should implement a trace function that can return whatever its second argument is ?

ahakanbaba · 2018-04-10T19:54:46Z

By the way, are the appveyor tests operational ?
I am getting this error that I do not think I have caused

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.Platform.targets(57,5): error MSB8020: The build tools for v141 (Platform Toolset = 'v141') cannot be found. To build using the v141 build tools, please install v141 build tools. Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\projects\jsonnet-dklrc\vs2017\core.vcxproj]

sbarzowski · 2018-04-10T20:44:09Z

Thank you very much for contributing!

Don't worry about appveyor tests. There were some problems going on for a while.

sbarzowski · 2018-04-10T21:01:14Z

As it stands the trace function expects an obj {} as its second argument. I am not sure how useful that is.

Maybe we should implement a trace function that can return whatever its second argument is ?

Yes, I think it should allow any value to be passed there. It's a bit confusing otherwise. Especially since it's intended for debugging, so the user may not know about the type of the value passed to it.

ahakanbaba · 2018-04-11T00:52:19Z

core/desugarer.cpp

@@ -63,6 +63,7 @@ BuiltinDecl jsonnet_builtin_decl(unsigned long builtin)
        case 24: return {U"primitiveEquals", {U"a", U"b"}};
        case 25: return {U"native", {U"name"}};
        case 26: return {U"md5", {U"str"}};
+        case 27: return {U"trace", {U"str", U"obj"}};


Once the trace call can accept any type as the second parameter, what to call this here ? Right now it is obj.
Does it actually matter except what is printed in an error message ?

It doesn't really matter. rest might work.

Another option here is to return true, in which case this can be used as in if std.trace("foo") then ... or just assert std.trace("foo"); ...

I do not understand what you suggest.
First do you want to use "rest" like
case 27: return {U"trace", {U"str", U"rest"}};
?
Secondly, how would the return true look like ?

These are just giving the parameter names on the Jsonnet side, which matter because of being able to do std.md5(str="foo"), and otherwise aren't specified anywhere. There is no equivalent for the return value, I just mentioned it because I was thinking of ways to avoid the 2nd param, which might get a bit annoying because of the need to put a ) some distance away, when you insert the std.trace. However my suggestions are also a bit obtuse, since this really has nothing to do with assert or conditionals :)

case 27: return {U"trace", {U"str", U"rest"}}; would be fine

👍 for case 27: return {U"trace", {U"str", U"rest"}};. Functions that provide alternative semantics can be later implemented as regular jsonnet functions (non-builtin).

ahakanbaba · 2018-04-11T00:52:54Z

core/state.h

-        STRING = 0x13
+        STRING = 0x13,
+
+        ANY = 0xFF00


I am not sure whether it is acceptable to add this ENUM. I am open to suggestions though.

It is not necessary.

ahakanbaba · 2018-04-11T00:58:00Z

As @sparkprime mentioned in #130 the trace reveals implementation details.
The trace execution is also not necessarily in the order of declaration in the code.

For example

// local innerrFunc = function(x,y)
local func() = {
    t: std.trace("1",{a:"b"}),
    k: std.trace("2",[{},{a:"b"}]),
    l: std.trace("3",15),
    m: std.trace("4","text"),
    n: std.trace("5",true),
    x: std.trace("6",[{},{a:"b"}]),
    r: std.trace("8",true),
    z: std.trace("9",[{},{a:"b"}]),
    s: std.trace("10",true),
    o: std.trace("11",null),
};

{
   otherKey: func(),
   key: std.foldl(std.trace("Hello",function(acc,i) acc + i), [1,2,3],0),
}

Prints

$ ~/src/google/jsonnet/jsonnet  test.jsonnet
TRACE: test.jsonnet:17 Hello
TRACE: test.jsonnet:4 2
TRACE: test.jsonnet:5 3
TRACE: test.jsonnet:6 4
TRACE: test.jsonnet:7 5
TRACE: test.jsonnet:12 11
TRACE: test.jsonnet:9 8
TRACE: test.jsonnet:11 10
TRACE: test.jsonnet:3 1
TRACE: test.jsonnet:8 6
TRACE: test.jsonnet:10 9
{
   "key": 6,
   "otherKey": {
      "k": [
         { },
         {
            "a": "b"
         }
      ],
      "l": 15,
      "m": "text",
      "n": true,
      "o": null,
      "r": true,
      "s": true,
      "t": {
         "a": "b"
      },
      "x": [
         { },
         {
            "a": "b"
         }
      ],
      "z": [
         { },
         {
            "a": "b"
         }
      ]
   }

}

The trace messages are interleaved.

ahakanbaba · 2018-04-11T01:01:00Z

If all this looks reasonable I will also add documentation and then this can be merged.
Please take a look

sparkprime · 2018-04-11T01:18:07Z

core/vm.cpp

@@ -1290,6 +1293,33 @@ class Interpreter {
        return nullptr;
    }

+    const AST *builtinTrace(const LocationRange &loc, const std::vector<Value> &args)
+    {
+        validateBuiltinArgs(loc, "trace", args, {Value::STRING, Value::ANY});


Just don't call this and you don't need to define ANY.

Inline the bits of it that you do need, though. There must be some examples of this.

builtinLength is an example

sparkprime · 2018-04-11T01:18:57Z

core/vm.cpp

+        std::cerr << "TRACE: " << loc.file << ":" << loc.begin.line << " " <<  str
+            << std::endl;
+
+        auto t =  args[1].t;


you should be able to do scratch = args[1];

ahakanbaba · 2018-04-11T17:01:41Z

After we resolve the last questions about how to write case 27: return {U"trace", {U"str", U"obj"}}; I will write some documentation and this should be ready to merge.

ahakanbaba · 2018-04-11T18:55:34Z

As far as I can tell, this is ready to merge FYI @sparkprime @sbarzowski

ahakanbaba · 2018-04-11T23:31:25Z

I have just seen the go port
https://github.com/google/go-jsonnet
Who is expected to duplicate the new functionality there ?

sbarzowski · 2018-04-12T06:51:51Z

doc/docs/stdlib.html

+
+<h4 id="trace">std.trace(str, rest)</h4>
+
+<p> <code>trace</code> outputs the string to std::cerr in its first argument, before


s/std:cerr/stderr/, std::cerr is a C++ concept.

I would phrase it this way:
<code>trace</code> outputs the given string <code>str</code> to stderr and returns <code>rest</code> as the result.

sbarzowski · 2018-04-12T06:53:09Z

doc/docs/stdlib.html

+    else
+        std.trace('cond is false returning ' + std.toString(in2), in2);
+}</code></p>
+


I would extend the example with a call to conditionalReturn and then we could include example output as well.

sbarzowski · 2018-04-12T06:57:58Z

doc/docs/stdlib.html

+<h3 id="Debugging">Debugging</h3>
+
+<h4 id="trace">std.trace(str, rest)</h4>
+


Maybe we can add a note about versions of jsonnet in which it is available. In the past, users were confused when they tried to call new functions in older versions of jsonnet.

<em>Available since version UNRELEASED.</em> would work I think.

I was hoping you could cut a release after this merges. Can I call it 0.10.1 ?

@sparkprime decides about these things.

It would probably be 0.11 since new features were added. We would also need to add documentation for #481 before. I'm all for frequent releases, but the current procedure for new releases is a bit tedious AFAIK.

sbarzowski · 2018-04-12T07:33:17Z

I have just seen the go port
https://github.com/google/go-jsonnet
Who is expected to duplicate the new functionality there ?

It should be quite easy. Just a few lines need to be added to this file: https://github.com/google/go-jsonnet/blob/master/builtins.go. The only non-obvious part is extracting the location - and for that e.trace.loc should work. If you could do it, that would be awesome and if not, I'll handle it, no worries.

ahakanbaba · 2018-04-12T15:22:38Z

About the go port, Is there a document describing the reasoning behind it ? I would be very interested to know what drove you to re-implement jsonnet in go.
Does it run any faster ? If the performance is better we would be interested in giving it a try.

Also I recommend to redirect new features to go-jsonnet and stop accepting PRs to C++ except bugfixes from now on. Double committing can be really strenuous.

sbarzowski · 2018-04-12T16:09:17Z

About the go port, Is there a document describing the reasoning behind it ?

A few reasons:

The community around Kubernetes and Cloud generally (who are the main users of jsonnet currently) likes Go. I hope it will be more approachable this way and more people will contribute.
It's easier to avoid security issues with Go.
We can use Golang GC instead of implementing our own.
It was an opportunity to clean some things up and exhaustively test the implementation.
I would say the Go version is quite a bit simpler and easier to experiment with.
There are plans to build more tooling on top of it - and with Go it can be a bit easier. For now there is only an extremely primitive linter (a stub really) but it's a start.

@sparkprime could probably come up with more things.

Does it run any faster ? If the performance is better we would be interested in giving it a try.

It depends. There are things that are order of magnitude faster in go-jsonnet. Mostly because of more builtins and better desugaring of some code structures. But there are also some things that are slower.

Also I recommend to redirect new features to go-jsonnet and stop accepting PRs to C++ except bugfixes from now on. Double committing can be really strenuous.

@sparkprime announced a plan to eventually deprecate C++ version, but some things need to happen first (fully shared tests, some remaining issues in go-jsonnet). Actually double committing is not that much of an issue - tests and stdlib are shared, and implementation details can differ. Only new language features need to be implemented in both places and adding such things is a pretty big deal anyway.

sparkprime · 2018-04-12T18:32:06Z

The security concerns about people running Jsonnet on untrusted code on the server side was probably the biggest motivator for go-jsonnet. It was started by jbeda of heptio but most of the work was done by sbarzowski.

I find go-jsonnet generally about 2x slower in my usage. You could certainly experiment with it though, the commandline binary is a drop-in replacement.

It really needs someone to go over it with a profiler and tweak some of the code at a microscopic level, I suspect that would get a chunk of performance back. The next thing would be to implement optimizations at the Jsonnet language level, and only implement them in go-jsonnet.

I would indeed like to cut over to add features to go-jsonnet only, but it's not quite ready yet. In particular, the ASTs do not record comments and whitespace, which is bad for tooling.

I think we can merge this now, right sbarzowski?

sbarzowski · 2018-04-12T19:38:56Z

Yes, LGTM.

sparkprime · 2018-04-12T20:07:33Z

Thanks @ahakanbaba I think a lot of people will find this useful!

ahakanbaba · 2018-04-12T22:56:43Z

@sparkprime awesome.
Could you please generate a new release so that we can also start using it ?

ahakanbaba · 2018-04-20T23:57:46Z

@sparkprime Do you plan to generate a release anytime soon ? It would be great for and and make it easier for us to use std.trace().

abourget · 2019-05-13T16:06:47Z

Any way to get timings of what's inside the trace (time to compute the rest) ?

sbarzowski · 2019-05-13T16:17:03Z

@abourget No, but it's an interesting idea and probably quite easy to implement. (Though it may be tricky to interpret the results sometimes due to caching and laziness, at least this std.traceWithTime should probably always actually evaluate its second argument). You can create a separate issue so it doesn't get lost.

Also if you are having performance issues feel free to report performance issues (especially if the problem is also happening with go-jsonnet).

Add a builtin trace function.

7cf845b

Prints the first argument to std err with line number Returns the second argument which is expected to be an object.

googlebot added the cla: yes label Apr 10, 2018

Remove misleading comment and add colon after TRACE

f8cee45

ahakanbaba added 2 commits April 11, 2018 00:27

Make the trace call accept any type as second parameter

deadd25

Add error testing for the trace command

691c547

ahakanbaba commented Apr 11, 2018

View reviewed changes

sparkprime reviewed Apr 11, 2018

View reviewed changes

ahakanbaba added 2 commits April 11, 2018 16:51

Improve error handling of trace and add more error tests.

de04f98

Delete extra space.

b400aff

ahakanbaba added 2 commits April 11, 2018 18:50

Parameter name change and add documentation

24665dc

Remove an extra curly brackets

4577f25

sbarzowski reviewed Apr 12, 2018

View reviewed changes

Extend the example of the document .

4fa8ab6

sparkprime merged commit f0dcfc4 into google:master Apr 12, 2018


		<h4 id="trace">std.trace(str, rest)</h4>

		<p> <code>trace</code> outputs the string to std::cerr in its first argument, before

		<h3 id="Debugging">Debugging</h3>

		<h4 id="trace">std.trace(str, rest)</h4>

Add a builtin trace function. #487

Add a builtin trace function. #487

Conversation

ahakanbaba commented Apr 10, 2018

ahakanbaba commented Apr 10, 2018

ahakanbaba commented Apr 10, 2018

sbarzowski commented Apr 10, 2018 • edited Loading

sbarzowski commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahakanbaba commented Apr 11, 2018 • edited Loading

ahakanbaba commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahakanbaba commented Apr 11, 2018

ahakanbaba commented Apr 11, 2018

ahakanbaba commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbarzowski commented Apr 12, 2018 • edited Loading

ahakanbaba commented Apr 12, 2018

sbarzowski commented Apr 12, 2018

sparkprime commented Apr 12, 2018

sbarzowski commented Apr 12, 2018

sparkprime commented Apr 12, 2018

ahakanbaba commented Apr 12, 2018

ahakanbaba commented Apr 20, 2018

abourget commented May 13, 2019

sbarzowski commented May 13, 2019

sbarzowski commented Apr 10, 2018 •

edited

Loading

ahakanbaba commented Apr 11, 2018 •

edited

Loading

sbarzowski commented Apr 12, 2018 •

edited

Loading