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

[WIP] allow parsing of JSON at compile time #10637

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@Vindaar
Copy link
Contributor

Vindaar commented Feb 11, 2019

This PR prototypes support for parsing JSON at compile time as a .compileTime. variable.
Note: it explicitly does not support storing a parsed JsonNode in a const and using it at runtime, since JsonNode is a ref type.

It introduces staticParseJson and staticParseFile (please provide better names :P) for this, so that

var jData {.compileTime.} = staticParseJson("{"name" : "Araq"}")
static:
  doAssert jData["name"].getStr == "Araq"

is possible.

Regarding the implementation, I just cleaned up my hacky code somewhat I mentioned on IRC. If we consider merging this, I'm happy improve it.
I wrote it some time ago to try to check some JSON at compile time.

Related discussion on IRC yesterday: https://irclogs.nim-lang.org/10-02-2019.html#14:30:29

Vindaar added some commits Feb 11, 2019

prototype to support JSON parsing at compile time
NOTE: this explicitly only allows to parse JSON into a compile time
variable! The `JsonNode` cannot be stored and used at runtime!
@yglukhov

This comment has been minimized.

Copy link
Member

yglukhov commented Feb 11, 2019

Why introduce a new proc instead of making parseJson work in compile time?

EDIT: Same question for lexer.

@mratsim

This comment has been minimized.

Copy link
Collaborator

mratsim commented Feb 11, 2019

Nim supports static overloading:

proc myProc(n: int): int =
  10 * n

proc myProc(n: static int): int =
  100 * n


const x = myProc(5)
let y = myProc(5)

echo x # 500
echo y # 500

echo myProc(x) # 50000
echo myProc(y) # 5000
@yglukhov

This comment has been minimized.

Copy link
Member

yglukhov commented Feb 11, 2019

And nim supports when nimvm:

proc myProc() =
  when nimvm:
    echo "I'm running in CT"
  else:
    echo "I'm running in RT"
@Vindaar

This comment has been minimized.

Copy link
Contributor Author

Vindaar commented Feb 11, 2019

And nim supports when nimvm:

Originally the code was even more hacky and I just replaced stuff. But while cleaning it up earlier today I thought there had to be something like this. I just didn't know what it was called. So, thanks a lot! Will incorporate it!

edit:
@mratsim: I know, but I didn't want to be restricted to a e.g. static string (in case input is supposed to be the result of some other CT call). What @yglukhov mentioned is exactly what I had in mind but didn't know existed / what it was called.

@Vindaar

This comment has been minimized.

Copy link
Contributor Author

Vindaar commented Feb 18, 2019

Sorry, I was busy the last week. Just got back to this and while trying to make use of nimvm I quickly became aware of its limitations. As also stated in the manual:
https://nim-lang.github.io/Nim/manual.html#statements-and-expressions-when-nimvm-statement

  • the expression must be when nimvm, so I can't do
    when defined(js) or nimvm:
    in the code.
  • no elif allowed, so the js part cannot be its own branch either.
  • cannot define symbols in the block, which are used outside, which means I can't define BaseLexer and JsonParser once for the VM and once for runtime, both with the same name, nor do something like
    type 
      BaseLexer = object
        when nimvm:
          input: string 
        else:
          input: Stream
    either.

Overloading the parseJson proc is fine at least (there when nimvm can be used) and also both lexbase.close and parsejson.open can be merged.

I thought about adding a complete StringStream for the VM, but that already fails because of the necessary up conversion from e.g.

proc ssCtRead(s: Stream, buffer: pointer, bufLen: int): int =
  var s = StringStream(s) #  <---- fails in vmgen

but that would be necessary, since the Stream type needs such a signature for the impl procs.

Possible solutions

  • drop this
  • live with RT and CT distinction of the types
  • add additional fields to the RT types to accommodate the CT requirements

Anything I'm missing and preferences?

edit: just pushed the last changes combining the static and normal parseJson procs and renaming BaseLexerRT back to BaseLexer to at least (hopefully) pass the tests.

@GULPF

This comment has been minimized.

Copy link
Member

GULPF commented Feb 18, 2019

I don't think it's a good idea to have static T overloads with completely different semantics than the base proc. If I write parseFile("/some/absolute/path") I definitely don't expect it to read and parse the file at compile time (this would be a breaking change as well).

@Vindaar

This comment has been minimized.

Copy link
Contributor Author

Vindaar commented Feb 18, 2019

I don't think it's a good idea to have static T overloads with completely different semantics than the base proc. If I write parseFile("/some/absolute/path") I definitely don't expect it to read and parse the file at compile time (this would be a breaking change as well).

Oops, that was simply a mistake on my part. parseFile was only supposed to read the file at CT if called in a static context.
edit: and I should fix that properly...

@Vindaar Vindaar force-pushed the Vindaar:parseJsonCT branch from 76c2c1d to 53d3f3e Feb 18, 2019

@krux02

This comment has been minimized.

Copy link
Contributor

krux02 commented Feb 18, 2019

Generally I like it when you manage to get the json parser to work at compile time. What I don't like is that you introduce a lot of code duplication and many changes to the signature of procedures, just to make the field input a string instead of a Stream.

Wouldn't it be better to just support the Stream type at compile time? I don't know what Araq thinks about that though since I can remember there was some resistence to support File, stdout, etc at compile time (dependencey if Stream).

@Vindaar

This comment has been minimized.

Copy link
Contributor Author

Vindaar commented Feb 18, 2019

Ok, apparently I don't understand what when nimvm really does, because the current code for parseFile with a static string always evaluates the VM branch. Maybe only in addition though? Not sure.

Generally I like it when you manage to get the json parser to work at compile time. What I don't like is that you introduce a lot of code duplication and many changes to the signature of procedures, just to make the fild input a string instead of a Stream.

Yes, totally agree.

Wouldn't it be better to just support the Stream type at compile time? I don't know what Araq thinks about that though since I can remember there was some resistence to support File, stdout, etc at compile time (dependencey if Stream).

That would certainly be more elegant. But since it's not that simple, it's beyond me at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment