Bip 5 #21

juanjux · 2018-05-16T16:36:38Z

Signed-off-by: Juanjo Alvarez juanjo@sourced.tech

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

dennwc · 2018-05-16T16:57:03Z

Reviewed 12 of 215 files at r1.
Review status: 12 of 215 files reviewed at latest revision, all discussions resolved, some commit checks broke.

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

	{
		ResponseMetadata{
			TopLevelIsRootNode: true,

You may omit the whole ResponseMetadata helper if this parameter is true.

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

	}

	if gostr, ok := s.Native().(string); ok {

same: string(s) instead of s.Native().(string)
and please check other places as well

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

	}

	if _, ok := operatorRoles[s]; !ok {

This is exactly what Lookup does, but I haven't added it to the guide yet :P

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

	// Parent of the last element of the qualified identifier (annotates
	// it and the child which is a normal identifier)
	MapAST("send", Obj{

Looks like a good place to use AnnotateType (it supports nesting now!).

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

			"selector": Var("childselector"),
			uast.KeyRoles: Roles(role.Identifier),
			"__notcall": Int(1),

Bool(true)?

fixtures/var_global.rb.uast, line 8 at r1 (raw file):

.  Children: {
.  .  0: module {
.  .  .  Roles: Identifier,Module,Statement

Identifier?

fixtures/while.rb.uast, line 5 at r1 (raw file):

Errors: 
UAST: 
 {

Looks like we now have an empty node as a root

fixtures/while.rb.uast, line 30 at r1 (raw file):

.  .  .  .  .  Children: {
.  .  .  .  .  .  0: true {
.  .  .  .  .  .  .  Roles: Boolean,Condition,Expression,Literal

missing While role, compared to an old AST

fixtures/while.rb.uast, line 47 at r1 (raw file):

.  .  .  .  .  .  }
.  .  .  .  .  .  1: lvasgn {
.  .  .  .  .  .  .  Roles: Assignment,Binary,Body,Expression,Identifier,Left

same for While role

native/lib/ruby_driver/node_converter.rb, line 78 at r1 (raw file):

      # the inner nodes of the above
      when "Complex", "Rational", "Symbol"
        return {@@typekey => node_type(node), "token" => node.to_s}

@token?

native/lib/ruby_driver/node_converter.rb, line 232 at r1 (raw file):

      subelem = node.loc.send(key)
      if subelem != nil
        hash["pos_line_start"] = subelem.begin.line

Maybe we can use new names for position fields here as well?

Comments from Reviewable

juanjux · 2018-05-17T08:25:56Z

Review status: 12 of 215 files reviewed at latest revision, 11 unresolved discussions, some commit checks broke.

fixtures/while.rb.uast, line 30 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

missing While role, compared to an old AST

Yes, I changed it to be in line with other annotations that doesn't replicate the parent role in the children (it's a tree anyway).

fixtures/while.rb.uast, line 47 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

same for While role

Ditto.

Comments from Reviewable

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

juanjux · 2018-05-17T08:32:56Z

Review status: 11 of 215 files reviewed at latest revision, 11 unresolved discussions, some commit checks broke.

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

Previously, dennwc (Denys Smirnov) wrote…

Bool(true)?

Done.

Comments from Reviewable

juanjux · 2018-05-17T08:34:00Z

Review status: 11 of 215 files reviewed at latest revision, 11 unresolved discussions, some commit checks broke.

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

Previously, juanjux (Juanjo Alvarez Martinez) wrote…

Done.

Will probably review it anyway, my spider sense tells me there must be a better way (the "send" annotation is pretty complicated, but only the one for functions without parameters is failing, I'll need to check with you because probably finding the solution is a good test for the new DSL).

Comments from Reviewable

juanjux · 2018-05-17T08:35:29Z

Review status: 11 of 215 files reviewed at latest revision, 11 unresolved discussions, some commit checks broke.

fixtures/var_global.rb.uast, line 8 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Identifier?

Done. I'm not sure why reviewable still marks this as pending...

Comments from Reviewable

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

…nt work Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

juanjux · 2018-05-18T15:06:18Z

Review status: 9 of 226 files reviewed at latest revision, 7 unresolved discussions.

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

Previously, dennwc (Denys Smirnov) wrote…

You may omit the whole ResponseMetadata helper if this parameter is true.

Done.

fixtures/while.rb.uast, line 5 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Looks like we now have an empty node as a root

Done.

native/lib/ruby_driver/node_converter.rb, line 78 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

@token?

Done.

native/lib/ruby_driver/node_converter.rb, line 232 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Maybe we can use new names for position fields here as well?

Those are actually the names defined in legacy.go but, as talked on Slack, if I remove the ObjectToNoder mapping them they don't work.

Comments from Reviewable

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

juanjux · 2018-05-29T15:45:56Z

Review status: 5 of 226 files reviewed at latest revision, 7 unresolved discussions.

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

Previously, dennwc (Denys Smirnov) wrote…

This is exactly what Lookup does, but I haven't added it to the guide yet :P

Fixed! (and a lot of cruft removed).

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

Previously, dennwc (Denys Smirnov) wrote…

Looks like a good place to use AnnotateType (it supports nesting now!).

Changed all that part with the last commit.

Comments from Reviewable

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

dennwc · 2018-05-17T16:43:22Z

native/lib/ruby_driver/node_converter.rb

-
-      #when "mlhs"
-        #return node.children.map{ |x| convert(x) }.compact
+        return {@@typekey => node_type(node), "token" => node.to_s}


dennwc · 2018-05-17T16:45:19Z

native/lib/ruby_driver/node_converter.rb

-        hash["end_line"] = subelem.end.line
-        hash["start_col"] = subelem.begin.column
-        hash["end_col"] = subelem.end.column
+        hash["pos_line_start"] = subelem.begin.line


We can use new names for position fields here as well.

Aren't these the new names already?

(PS: at least those are the ones in sdk/uast/transformer/legacy.go).

These are variable names, it uses uast.KeyStart in a separate node with a type ast:Position.

So something like: hash["type": "ast:Position"] = {"start": bla, "end": bla, "offset": bla}?

Just saw the right format in the Java driver. Will change it.

Done, works, 👍

dennwc · 2018-06-06T10:27:14Z

native/exe/native

@@ -1,4 +1,4 @@
-#!/usr/bin/ruby -wU
+#!/usr/bin/ruby -wUI/opt/driver/src/build/dependencies


This won't work locally, right?

Mmmm, I don't remember why I did that, but both make test and rebuilding the docker image work without it, I'll remove it and see if CI works here.

dennwc · 2018-06-06T10:33:53Z

native/lib/ruby_driver/node_converter.rb

-          "end_line" => comment.loc.last_line,
-          "start_col" => comment.loc.column,
-          "end_col" => comment.loc.last_column
+          "pos_line_start" => comment.loc.first_line,


Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

dennwc · 2018-06-06T11:51:54Z

LGTM! Thanks!

Juanjo Alvarez added 4 commits May 8, 2018 18:05

Some annotations

e59ac8c

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

All annotations and tokens except send done

bf8a29d

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

send role semi annotated

5346b69

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Fix annotation for simple identifiers that are not functions

d120500

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

juanjux added the enhancement label May 16, 2018

juanjux self-assigned this May 16, 2018

juanjux requested a review from dennwc May 16, 2018 16:36

dennwc mentioned this pull request May 16, 2018

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

Closed

7 tasks

Some fixes from review (still not all)

fc45382

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Juanjo Alvarez added 5 commits May 17, 2018 12:19

Add missing files

68e10e8

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Update the native binary path in fixtures_test.go, make test still do…

de1d2a6

…nt work Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Install json gem

103314b

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Fix for nodes missing native column and setting it to 0

56e1a12

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Fixed function + operator annotations

1233fb0

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Juanjo Alvarez added 4 commits May 18, 2018 17:06

Some fixes from review

7c5bd57

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Some fixes from review

d457585

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

finish the send node annotations

0fa5f88

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Cleanup (now) unneeded cruft

312391e

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Add the send_array variant of sent for arrays

009fd4a

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

dennwc reviewed Jun 6, 2018

View reviewed changes

Juanjo Alvarez added 2 commits June 6, 2018 13:10

Remove unneeded(?) include from the native script

6c5cd43

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Use the default position keys

1cd8ab9

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

dennwc approved these changes Jun 6, 2018

View reviewed changes

juanjux merged commit d16e7f1 into bblfsh:master Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bip 5 #21

Bip 5 #21

juanjux commented May 16, 2018 •

edited

Loading

dennwc commented May 16, 2018

juanjux commented May 17, 2018

juanjux commented May 17, 2018

juanjux commented May 17, 2018

juanjux commented May 17, 2018

juanjux commented May 18, 2018

juanjux commented May 29, 2018

dennwc May 17, 2018

dennwc May 17, 2018

juanjux Jun 6, 2018 •

edited

Loading

dennwc Jun 6, 2018

juanjux Jun 6, 2018 •

edited

Loading

juanjux Jun 6, 2018

juanjux Jun 6, 2018

dennwc Jun 6, 2018

juanjux Jun 6, 2018

dennwc Jun 6, 2018

dennwc commented Jun 6, 2018

		@@ -1,4 +1,4 @@
		#!/usr/bin/ruby -wU
		#!/usr/bin/ruby -wUI/opt/driver/src/build/dependencies

Bip 5 #21

Bip 5 #21

Conversation

juanjux commented May 16, 2018 • edited Loading

dennwc commented May 16, 2018

juanjux commented May 17, 2018

juanjux commented May 17, 2018

juanjux commented May 17, 2018

juanjux commented May 17, 2018

juanjux commented May 18, 2018

juanjux commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanjux Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanjux Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennwc commented Jun 6, 2018

juanjux commented May 16, 2018 •

edited

Loading

juanjux Jun 6, 2018 •

edited

Loading

juanjux Jun 6, 2018 •

edited

Loading