Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nix-instantiate: parse to json ast #4731

Closed
wants to merge 20 commits into from
Closed

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Apr 22, 2021

resolve #4726
closed in favor of #5512

basically there are two possible solutions:

1. recursive: on struct Expr and its derived structs, add a method showAsJson.
the root expression is printed with expr->showAsJson(std::cout);
and showAsJson prints child expressions with childExpr->showAsJson(std::cout);.
a similar solution is used in nlohmann/json

2. use an iterative visitor function to print the AST nodes
to make this work, we would have to expose the node type in struct Expr
-> see internalType in struct Value
this could be faster than solution 1, but its harder to implement (manage the stack manually)

1a. Alipha from ##c++ on freenode suggested a more complex solution
https://wandbox.org/permlink/luDOhrLzloiSME6h
but imho we dont need that complexity (keep it simple)

src/libexpr/nixexpr.hh Outdated Show resolved Hide resolved
src/libexpr/nixexpr.hh Outdated Show resolved Hide resolved
@@ -79,16 +79,28 @@ struct Expr
{
virtual ~Expr() { };
virtual void show(std::ostream & str) const;
virtual void showAsAterm(std::ostream & str) const;
virtual void showAsJson(std::ostream & str) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be

virtual nlohmann::json to_json() const;

or something like that.

Copy link
Contributor Author

@milahu milahu Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nlohmann::json sounds like overhead .. intermediary data structure + unnecessary pretty printing + unnecessary string validation (check for invalid utf8)

Copy link
Contributor Author

@milahu milahu May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nlohmann's dump function is similar to my showAsJson, also using recursion, not iteration (which could be faster)

@@ -31,7 +31,12 @@ void processExpr(EvalState & state, const Strings & attrPaths,
bool evalOnly, OutputKind output, bool location, Expr * e)
{
if (parseOnly) {
std::cout << format("%1%\n") % *e;
if (output == okJSON) {
std::cout << format("%1%\n") % (ExprAsJson *) *e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't add this functionality to nix-instantiate since it's legacy. Better to add a new command like nix parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thanks for remind. also the CLI of nix-instantiate is not ideal (--json, --xml)

Copy link
Contributor Author

@milahu milahu May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first draft in 4c35710

@milahu
Copy link
Contributor Author

milahu commented Apr 23, 2021

status: working prototype

echo '{ foo = "bar"; }' >test.nix; ./nix-instantiate --parse --json test.nix | jq

json ast result
{
  "type": 6,
  "recursive": false,
  "attrs": [
    {
      "type": "attr",
      "inherited": false,
      "name": "foo",
      "value": {
        "type": 10,
        "value": "bar"
      }
    }
  ],
  "dynamicAttrs": []
}

type 6 = ExprAttrs, type 10 = ExprString, ... see enum class NodeTypeId in nixexpr.hh
"type": "attr", should be numeric type

issue: parser resolves relative paths to absolute paths, but should stay relative
= parser and interpreter should be better separated

@milahu
Copy link
Contributor Author

milahu commented May 3, 2021

i will remove the other formats like json-arrays-fmt, only needed for benchmarks

benchmark results

my json-arrays format needs 50% of the size
and 70% of the parse-time of the json format
80 ms vs 120 ms for a 900 KByte nix file (all-packages.nix from nixpkgs)

my json-numtypes format (with types encoded as numbers) can be parsed 10% faster
than my json format (with types encoded as strings)

generating the json and walking the tree has the same speed
in javascript, consuming array-trees is not faster than consuming object-trees
(but javascript is highly optimized for objects)

benchmark graphs

three graphs: generate, parse, walk
three formats: json, json-numtypes, json-arrays = purple, green, blue

benchmark-parse-json-vs-json-arrays 1620024276

@milahu milahu requested a review from edolstra May 4, 2021 16:38
@milahu
Copy link
Contributor Author

milahu commented May 4, 2021

the CLI of nix parse needs some more work
should we just use the same interface as nix eval?

$ nix parse --output-format json '(1 + 2)'
{"type":"ExprConcatStrings","line":1,"column":2,"strings":[{"type":"ExprInt","value":1},{"type":"ExprInt","value":2}]}

$ nix parse --output-format json --file some-file.nix
...

note: concat strings and arithmetic sum have the same node type

@milahu
Copy link
Contributor Author

milahu commented Nov 6, 2021

push

needed for nix-gui, to avoid stupid workarounds like this

@milahu
Copy link
Contributor Author

milahu commented Nov 7, 2021

closing in favor of #5512

@milahu milahu closed this Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: parse nix expression to json ast
2 participants