Port driver to transformation DSL (BIP-5) #19

dennwc · 2018-03-22T17:18:53Z

Signed-off-by: Denys Smirnov denys@sourced.tech

* port annotations * update driver files * serialize positions for all AST nodes * preserve nil fields in native AST Signed-off-by: Denys Smirnov <denys@sourced.tech>

Signed-off-by: Denys Smirnov <denys@sourced.tech>

juanjux · 2018-04-23T08:37:46Z

driver/normalizer/annotation.go

+	{Mappings(Annotations...)},
+	{
+		Mappings(
+			AnnotateIfNoRoles("FieldList", role.Incomplete),


The Incomplete role must not be applied automatically. It's used by the driver programmer to mark roles that doesn't express the full semantic information for any reason (usually because of lack of more specific roles).

PTAL: https://github.com/bblfsh/documentation/blob/master/proposals/bip-002.md

juanjux · 2018-04-23T08:38:35Z

driver/normalizer/annotation.go

+	{
+		Mappings(
+			AnnotateIfNoRoles("FieldList", role.Incomplete),
+			Unannotated(),


The Unannotated role on the other side should be applied automatically. Having do add a Transformer in the driver breaks a little BIP-2 since it should be automatically done despite whatever the driver programmer does, but I guess we can work with this (opening an issue if the current SDK get's merged) until we find a way to apply it automatically.

(same for the other drivers).

dennwc · 2018-04-23T09:21:59Z

Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

driver/normalizer/annotation.go, line 20 at r1 (raw file):

Previously, juanjux (Juanjo Alvarez Martinez) wrote…

The Incomplete role must not be applied automatically. It's used by the driver programmer to mark roles that doesn't express the full semantic information for any reason (usually because of lack of more specific roles).

PTAL: https://github.com/bblfsh/documentation/blob/master/proposals/bip-002.md

This is exactly what it's used here for. FieldList appears either as a list of arguments for a function (and will not receive this role, since it already has it), or a list of struct fields, that is already annotated in other ways, I believe. It might make sense to mark them as List, Incomplete, but overall I think it works as it should.

driver/normalizer/annotation.go, line 21 at r1 (raw file):

Previously, juanjux (Juanjo Alvarez Martinez) wrote…

The Unannotated role on the other side should be applied automatically. Having do add a Transformer in the driver breaks a little BIP-2 since it should be automatically done despite whatever the driver programmer does, but I guess we can work with this (opening an issue if the current SDK get's merged) until we find a way to apply it automatically.

(same for the other drivers).

I agree that this should be added automatically, but for now I'm waiting for you to verify that it works well for Python and PHP. The reason why it might not work, is because it adds this role to every node without roles, and may touch some nodes that was not meant to be annotated at all (legacy IsNode method was filtering it before). In that case the call to Unannotated might accept a function that will tell what to annotate and what to ignore.

On the other hand I think better sollution would be to return Unannotated role for Node.Roles() when the field does not exists. It will allow to remove this additional transformation (and improve prerformance by not scanning the tree again), and will automatically work for all nodes.

Comments from Reviewable

juanjux · 2018-04-23T10:40:23Z

Review status: 0 of 141 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

driver/normalizer/annotation.go, line 20 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

This is exactly what it's used here for. FieldList appears either as a list of arguments for a function (and will not receive this role, since it already has it), or a list of struct fields, that is already annotated in other ways, I believe. It might make sense to mark them as List, Incomplete, but overall I think it works as it should.

The List role is for arrays, list, etc, data structures, not for list of items in the UAST or arguments.

driver/normalizer/annotation.go, line 21 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I agree that this should be added automatically, but for now I'm waiting for you to verify that it works well for Python and PHP. The reason why it might not work, is because it adds this role to every node without roles, and may touch some nodes that was not meant to be annotated at all (legacy IsNode method was filtering it before). In that case the call to Unannotated might accept a function that will tell what to annotate and what to ignore.

On the other hand I think better sollution would be to return Unannotated role for Node.Roles() when the field does not exists. It will allow to remove this additional transformation (and improve prerformance by not scanning the tree again), and will automatically work for all nodes.

Seem to work pretty well! I found a crashing bug while fixing a node that was unannotated because it didn't match the structure of some of it's use in the native AST but I don't think is related (I'll tell you tomorrow since I don't want to interfere much with your OSD).

Comments from Reviewable

dennwc · 2018-04-23T10:42:35Z

Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

driver/normalizer/annotation.go, line 20 at r1 (raw file):

Previously, juanjux (Juanjo Alvarez Martinez) wrote…

The List role is for arrays, list, etc, data structures, not for list of items in the UAST or arguments.

This is why I omitted it, yes. Do you consider this resolved?

driver/normalizer/annotation.go, line 21 at r1 (raw file):

Previously, juanjux (Juanjo Alvarez Martinez) wrote…

Seem to work pretty well! I found a crashing bug while fixing a node that was unannotated because it didn't match the structure of some of it's use in the native AST but I don't think is related (I'll tell you tomorrow since I don't want to interfere much with your OSD).

OK. And what do you think about moving this logic to uast.Node.Roles()?

Comments from Reviewable

juanjux · 2018-04-23T11:18:29Z

Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

driver/normalizer/annotation.go, line 21 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

OK. And what do you think about moving this logic to uast.Node.Roles()?

It's probably the perfect place AFAIK.

Comments from Reviewable

juanjux · 2018-04-23T14:26:34Z

Review status: 0 of 141 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

driver/normalizer/annotation.go, line 20 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

This is why I omitted it, yes. Do you consider this resolved?

No, since it still auto applies the incomplete role in this specific case (FieldList with no roles) :) (or maybe I'm not understanding what it does.)

Comments from Reviewable

dennwc · 2018-04-24T08:42:43Z

Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

driver/normalizer/annotation.go, line 20 at r1 (raw file):

Previously, juanjux (Juanjo Alvarez Martinez) wrote…

No, since it still auto applies the incomplete role in this specific case (FieldList with no roles) :) (or maybe I'm not understanding what it does.)

Again, as I said, this is intentional - FieldList is just a proxy node. When it's used in arguments, it receives a specific role, and if not, it will get Incomplete because we actually don't care about this particular node type.

driver/normalizer/annotation.go, line 21 at r1 (raw file):

Previously, juanjux (Juanjo Alvarez Martinez) wrote…

It's probably the perfect place AFAIK.

OK, will change it in SDK

Comments from Reviewable

Signed-off-by: Denys Smirnov <denys@sourced.tech>

dennwc · 2018-04-24T13:36:53Z

Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

driver/normalizer/annotation.go, line 21 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

OK, will change it in SDK

Done.

Comments from Reviewable

Signed-off-by: Denys Smirnov <denys@sourced.tech>

dennwc added the enhancement label Mar 22, 2018

dennwc self-assigned this Mar 22, 2018

dennwc mentioned this pull request Apr 12, 2018

Update drivers to use new DSL (BIP-5) bblfsh/sdk#243

Closed

7 tasks

Denys Smirnov added 4 commits April 20, 2018 17:33

port driver annotations to a new DSL

a5df2a7

* port annotations * update driver files * serialize positions for all AST nodes * preserve nil fields in native AST Signed-off-by: Denys Smirnov <denys@sourced.tech>

update integration tests with new positional information and sort roles

6c03098

Signed-off-by: Denys Smirnov <denys@sourced.tech>

drop internal protocol and compile driver as a single binary

2135daa

Signed-off-by: Denys Smirnov <denys@sourced.tech>

move helpers to sdk and use some new helpers from it

c240afc

Signed-off-by: Denys Smirnov <denys@sourced.tech>

dennwc force-pushed the bip-5 branch from fc76686 to c240afc Compare April 20, 2018 15:34

juanjux suggested changes Apr 23, 2018

View reviewed changes

add missing annotations

4203582

Signed-off-by: Denys Smirnov <denys@sourced.tech>

dennwc force-pushed the bip-5 branch from 801a219 to 4203582 Compare April 24, 2018 13:28

juanjux approved these changes May 3, 2018

View reviewed changes

Denys Smirnov added 2 commits May 3, 2018 14:07

use a single AnnotateType helper for field roles

458362c

Signed-off-by: Denys Smirnov <denys@sourced.tech>

update to sdk v2

cca067a

Signed-off-by: Denys Smirnov <denys@sourced.tech>

dennwc force-pushed the bip-5 branch from 90bcff3 to cca067a Compare May 3, 2018 11:08

dennwc merged commit 6b40ce1 into bblfsh:master May 3, 2018

dennwc deleted the bip-5 branch May 3, 2018 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port driver to transformation DSL (BIP-5) #19

Port driver to transformation DSL (BIP-5) #19

dennwc commented Mar 22, 2018 •

edited

Loading

juanjux Apr 23, 2018

juanjux Apr 23, 2018 •

edited

Loading

dennwc commented Apr 23, 2018

juanjux commented Apr 23, 2018

dennwc commented Apr 23, 2018

juanjux commented Apr 23, 2018

juanjux commented Apr 23, 2018

dennwc commented Apr 24, 2018

dennwc commented Apr 24, 2018

Port driver to transformation DSL (BIP-5) #19

Port driver to transformation DSL (BIP-5) #19

Conversation

dennwc commented Mar 22, 2018 • edited Loading

juanjux Apr 23, 2018

Choose a reason for hiding this comment

juanjux Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

dennwc commented Apr 23, 2018

juanjux commented Apr 23, 2018

dennwc commented Apr 23, 2018

juanjux commented Apr 23, 2018

juanjux commented Apr 23, 2018

dennwc commented Apr 24, 2018

dennwc commented Apr 24, 2018

dennwc commented Mar 22, 2018 •

edited

Loading

juanjux Apr 23, 2018 •

edited

Loading