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

[GSP-3] We need to have access to the AST in the wasm parser #20

Open
jsign opened this issue Oct 26, 2022 · 4 comments
Open

[GSP-3] We need to have access to the AST in the wasm parser #20

jsign opened this issue Oct 26, 2022 · 4 comments
Labels
enhancement New feature or request linear Sync issue with linear

Comments

@jsign
Copy link

jsign commented Oct 26, 2022

Allow the client to get access to the AST for a query, probably to solve use-cases for the new ACL features.

GSP-3

@jsign
Copy link
Author

jsign commented Oct 26, 2022

I also suggested @brunocalza an alternate idea to avoid dealing with potential technical problems of AST communication in the WASM parser.

We can simply allow the WASM parser to have high-level functions to do whatever thask the client is trying to achieve, for example:

  • Know if a particular query has a value somewhere it's interested in (e.g: look in this GRANT/REVOKE if it has a rule X)
  • Mutate the AST in some defined way. (e.g: if we know, in some cases, we always want to mutate the AST of a GRANT/REVOKE in some known way, it might be simpler to ask the WASM parser to do that internally and not let the client mess around walking the AST and giving back a serialized result)

Potential drawbacks:

  • The WASM parser would have some extra code, leading to a bigger size. Not sure it would be much since the parsing code is quite big so whatever new feature, in relative terms, I'm not sure would be a reasonable %.
  • The client can't do "whatever it wants" as it would be possible by walking and editing the AST in any way it wants.

The main benefits are:

  • The client doesn't need to know about AST nodes, which types are correct, walk the AST, or similar.
  • No serializing or deserializing of the AST is needed internally (WASM parser) or in the client (JS).
  • We aren't leaking AST details to the external world. Doing that (e.g: giving the AST to the client) means that if we do AST changes, we can break JS code if some existing node type changes or similar.
  • Not leaking the AST means we don't have to document how the JS client can use the AST result. Like, "what is this XXX node type?", etc).

To be clear, I'm not suggesting that we should use high-level APIs, but sounds it can avoid a lot of complexity for the clients and have some other benefits about breaking changes.

I'm not following much the need for this, so up to the interested parties. Just throwing an idea here.

cc @carsonfarmer @brunocalza @sanderpick @joewagner

@carsonfarmer
Copy link
Member

Yes I think this approach is acceptable. The benefit of allowing access to the AST from clients is that it opens up potential usage that we haven't thought of yet... Having said this, I did take a step down the proposed road here with the "type" property that gets returned with the normalize function in the WASM wrapper, so I don't think size is an issue at all here. Long story short: this seems like a reasonable course of action.

@brunocalza brunocalza moved this to 🆕 New in go-tableland backlog Oct 28, 2022
@jsign jsign moved this from 🆕 New to 📋 Prioritized in go-tableland backlog Oct 31, 2022
@sanderpick sanderpick added this to the 5.1.1 milestone Nov 2, 2022
@joewagner
Copy link
Collaborator

Looking though some old issues, and I'm wondering if we can mark this as complete?
cc: @carsonfarmer @brunocalza

@carsonfarmer
Copy link
Member

Yeh we kind of decided we don't want to provide full access to the raw AST. So I think we can close. It would be nice/useful, but not worth the effort at this stage.

@brunocalza brunocalza added the linear Sync issue with linear label Mar 23, 2023
@brunocalza brunocalza changed the title We need to have access to the AST in the wasm parser [GSP-3] We need to have access to the AST in the wasm parser Mar 23, 2023
@sanderpick sanderpick removed this from the 5.1.1 milestone Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request linear Sync issue with linear
Projects
No open projects
Development

No branches or pull requests

5 participants