Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

JSON parse failure #42

Closed
campoy opened this issue Nov 5, 2018 · 14 comments
Closed

JSON parse failure #42

campoy opened this issue Nov 5, 2018 · 14 comments
Labels

Comments

@campoy
Copy link

campoy commented Nov 5, 2018

I tried to parse a simple JSON object, expecting it to work as it does on the Chrome console,
but got a parsing error.

{
  "message": "hello, world"
}

Gives the error syntax error: message is not defined.

I would have expected JSON to be supported by this driver.
Is that an incorrect assumption on my side?

@dennwc dennwc added the bug label Nov 5, 2018
@dennwc
Copy link
Member

dennwc commented Nov 6, 2018

Hmm, interesting. It works if you add a variable:

a = {
  "message": "hello, world"
}

The reason for this is that the first { is interpreted as a code block with the following statement inside:

"message": "hello, world"

I'm not sure what : will mean in this context for JS, but in general this is definitely an invalid JS code.

I propose to add a driver for JSON if we want this to work properly.

@dennwc
Copy link
Member

dennwc commented Nov 8, 2018

A fun coincidence: message in an error text is referring to internal protocol message file (see #17), not the "message" value from the JSON.

Anyway, it gives a parsing error, so we might need a JSON driver.

@creachadair
Copy link
Contributor

I think that original example should already be a valid Program » Statement » ExpressionStatement » Expression » … » PrimaryExpression » ObjectLiteral, even without an assignment. (The "…" in this case elides the simple path of the precedence hierarchy).

@bzz
Copy link
Contributor

bzz commented Nov 29, 2018

Parsing of kv.json with latest docker run --rm -d -p 9432:9432 bblfsh/javascript-driver:v2.5.0

{
    "key": "value"
}

using

package main

import "gopkg.in/bblfsh/client-go.v3"

func main() {
	client, err := bblfsh.NewClient("localhost:9432")
	if err != nil {
		panic(err)
	}

	res, _, err := client.NewParseRequest().ReadFile("kv.json").UAST()
	if err != nil {
		panic(err)
	}
}

results in Unexpected token, expected ";" (2:9)

JSON driver 🤔 hm, as JSON by design should be a valid subset of JS this is a fallacy 😢, it could be nice use case for language_aliases but seems like it's not the case in general.

Will try to dig deeper and see if we can make current native parser overcome this.

@bzz bzz removed the bug label Nov 29, 2018
@bzz
Copy link
Contributor

bzz commented Nov 29, 2018

Found that https://babeljs.io/repl gives pretty good intuition on what JS driver supports now:

repl: Unexpected token, expected ; (1:4)
> 1 | {"k":"v"}

As #46 (comment) notes a particular case of difference of JSON and JavaScript that seems to be possible to mitigate with https://babeljs.io/docs/en/babel-plugin-proposal-json-strings

That's great, but does not seems to solve the case at hand.

Most probably best we can do is to document that JSON is not supported by JavaScript driver and close this one.

WDYT?

@dennwc
Copy link
Member

dennwc commented Nov 29, 2018

There is a second option though. We can change the driver a bit to try parsing the code as JS first, and if it fails, try to parse it as JSON.

@bzz
Copy link
Contributor

bzz commented Nov 30, 2018

Yes, we can do that + #46 to advertise this capability of the driver.

Honestly, I think it's worth doing that as idea of having a separate JSON driver scares me.

I belive we are a the point of discussion where it would be nice to have an input from @campoy, @mcuadros, @creachadair .

@creachadair
Copy link
Contributor

I generally support the idea of allowing drivers to handle more than one language. It does, however, raise some API design questions, like what to do if more than one driver claims credit for a file, and what to do if a file actually contains multiple languages (e.g., JS embedded in HTML, or HTML embedded in PHP).

@dennwc
Copy link
Member

dennwc commented Dec 3, 2018

Indeed, the problem of language strings embedded in the source file of another language is real. The same happens for the code that embeds static files. Or even simpler: SQL queries.

But this seems to be out of the list of responsibilities of Babelfish server.

I think we can solve it in a more generic way by adding some "language hint" for string literals in the UAST that each driver emits and the higher-level system (or the user) may pass this literal to Babelfish again, based on that hint.

This should isolate each language in its own driver, even for cases like PHP+HTML. Although the language detection will need to be very precise.

@dennwc
Copy link
Member

dennwc commented Dec 3, 2018

@bzz Actually I think we will end up with a separate driver for JSON anyway. The reason is simple: positional information.

Most languages will just parse JSON and emit the data. But only few will know where each token or literal is located.

@creachadair
Copy link
Contributor

Indeed, the problem of language strings embedded in the source file of another language is real. The same happens for the code that embeds static files. Or even simpler: SQL queries.

But this seems to be out of the list of responsibilities of Babelfish server.

That may be true today, but I don't think we should hold to that assumption. Graceful handling of nested languages is a huge feature for people trying to do code manipulation, particularly in front-end work where embedding like this is not only common but unavoidable.

That said: I agree it is not a thing we need to cope with right now. I do think we should put it on our roadmap, though. A good theory of nested quotation could potentially be an original contribution.

@dennwc
Copy link
Member

dennwc commented Dec 4, 2018

@creachadair Sorry, I meant that we have Babelfish parsing one or multiple files in a specific language and some other system above it (still written by us) that analyses this information, handles project versioning, etc. It may be the same system, but I haven't meant to exclude it from our responsibility, only from the driver's.

@creachadair
Copy link
Contributor

Oh I see! Sorry, I was confused.

@bzz
Copy link
Contributor

bzz commented Jul 8, 2019

@bzz Actually I think we will end up with a separate driver for JSON anyway. The reason is simple: positional information.

After learning more about bblfsh, I agree with Denys and am going to close this issue - JSON is not supported by JS driver and should have a separate driver eventually.

@bzz bzz closed this as completed Jul 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants