Node.load() implementation

[bzz]

This is a next step from the #83 - loading Go-decoded tree to the JVM memory.

It consists of

• Node.load() implementation + tests
• better error handling: JVM runtime exceptions instead of std::runtime_error
• restore single CI profile as all existing tests should be passing now
• small tests refactoring to extract BblfshClientBaseTest

---

This change is 

[bzz]

@creachadair All feedback addressed in 1e7858f, ready for another round!

[dennwc]

Reviewed 10 of 15 files at r1, 1 of 2 files at r2, 2 of 5 files at r3.
Reviewable status: 12 of 15 files reviewed, 18 unresolved discussions (waiting on @bzz, @creachadair, and @dennwc)

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 136 at r3 (raw file):

    if (!obj) {
      return NODE_NULL;
    } else if (env->IsInstanceOf(obj, env->FindClass(CLS_JSTR))) {

Wondering if it makes sense to cache the result of env->FindClass, or make it static.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 243 at r3 (raw file):

    return size;
  }
  std::string *KeyAt(size_t i) {

Am I missing something or i is unused? Same below.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 308 at r3 (raw file):

  // In the second case it creates a new reference.
  Node *lookupOrCreate(jobject obj) {
    if (!obj) return nullptr;

Should it also test for JNull? Or does it store those in the map?

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 323 at r3 (raw file):

    Node *node = new Node(this, kind, obj);
    obj2node[obj] = node;
    return node;

Should probably return nullptr for kind == Null

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 342 at r3 (raw file):

  // Returns a new reference.
  jobject toJ(Node *node) {
    if (node == nullptr) return nullptr;

!node as in other places?

---

src/test/scala/org/bblfsh/client/v2/BblfshClientBaseTest.scala, line 18 at r1 (raw file):

  before {
    val fileContent = Source.fromFile(fileName).getLines.mkString("\n")

Does it split and re-join the file?

[bzz]

Reviewable status: 12 of 15 files reviewed, 18 unresolved discussions (waiting on @bzz and @creachadair)

---

src/main/native/jni_utils.h, line 9 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Some of this is pre-existing, but consider documenting these names here in the header (this is what will be included by anywhere it's used, so this is presumably what the reader is going to look at).

A the least, please consider leaving a block comment nearby these definitions, to describe what they're used for. Some of the names are kind of obvious, but for example it isn't obvious that these are Babelfish types rather than Java ones, for example.

Done.

---

src/main/native/jni_utils.cc, line 24 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Each of these should be (for example) const char CLS_NODE[] = "…". (Or alternatively const char* const CLS_NODE = "…", but the first is a better signal of intent and less typing too)

Done.

---

src/main/native/jni_utils.cc, line 34 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

const char *CLS_JBOOL = "org/bblfsh/client/v2/JBool";

(I think?)

Done.

---

src/main/native/jni_utils.cc, line 50 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

This function is doing enough interesting work that I think it would benefit from a comment. In particular, it affects control flow in the host program so that probably deserves a call-out IMO.

Done in declaration.

---

src/main/native/jni_utils.cc, line 53 at r1 (raw file):

Previously, bzz (Alexander) wrote…

I see what you mean, but think that I would prefer to simply change this to a better error propagation at runtime.

Having a flag + reloading the native library from the JVM runtime sounds too error-prone and I do not think it will worth it. Especially, given the fact that one has to rebuild the native part with the clang debug flag anyway, in order to preserve the symbols.

Done.

---

src/main/native/jni_utils.cc, line 76 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

      std::string("failed to call <init> constructor for ").append(className));

Done.

---

src/main/native/jni_utils.cc, line 90 at r1 (raw file):

Previously, bzz (Alexander) wrote…

We do not have a name of the class at this point.

Done.

---

src/main/native/jni_utils.cc, line 115 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

  checkJvmException(std::string("failed to get method ")

(optional nit)

Done.

---

src/main/native/jni_utils.cc, line 121 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

  checkJvmException(std::string("failed to call method ")

(optional nit)

Done.

---

src/main/scala/org/bblfsh/client/v2/Context.scala, line 12 at r1 (raw file):

Previously, bzz (Alexander) wrote…

No, sorry for confusion - this is literally a mark for the next step, that I'm already working on for the next PR.

Done.

---

src/main/scala/org/bblfsh/client/v2/JNode.scala, line 16 at r1 (raw file):

Previously, bzz (Alexander) wrote…

In general, yes, that is true and should not be needed when modeled by the type hierarchy.

But in this particular case - it's just a convenience, in order to avoid having this exact logic on JNI side. Added a comment to clarify that.

Done.

---

src/main/scala/org/bblfsh/client/v2/JNode.scala, line 21 at r1 (raw file):

Previously, bzz (Alexander) wrote…

It signifies a single element rather then a container one, and you are right, it should be 0 instead (same as in other clients).

Done.

[bzz]

Reviewable status: 12 of 15 files reviewed, 18 unresolved discussions (waiting on @bzz, @creachadair, @dennwc, and @Denys)

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 136 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Wondering if it makes sense to cache the result of env->FindClass, or make it static.

Great question! TL;DR - yes it very much does, as a performance optimization.

But doing so correctly requires taking extra care with regards to threading (JNI can be running in different threads) and right now is left as TODO.

If that's alright, I would prefer addressing it later under that TODO, after we get first correct solution working.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 243 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Am I missing something or i is unused? Same below.

Very good catch! You spotted a bug - until now this code was not used or tested, as comment on line 192 mentions (sorry, have no idea how to link lines in here).

If that's alright - it is already fixed in the next PR, which addresses that TODO properly.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 308 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Should it also test for JNull? Or does it store those in the map?

You rock @Denys (oh, man! name mentions are not auto-suggested 🤦‍♂️ I will never be able to tag Michael or Oleksandr here..)

But yes, that is definitely is another bug, for the same reason fixed in the same PR.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 323 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Should probably return nullptr for kind == Null

True, but this is also a direct copy from https://github.com/bblfsh/client-python/blob/master/bblfsh/pyuast.cc#L626 so if you do not have a strong opinion, I would prefer keeping the behavior same as much as possible.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 342 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

!node as in other places?

👍 but again - it's a copy from https://github.com/bblfsh/client-python/blob/master/bblfsh/pyuast.cc#L653 and the same idiom is used in 4 other places there, so 🤷‍♂️ if that's one, I'll keep in case that is the only change needed, but will batch up with others otherwise.

---

src/test/scala/org/bblfsh/client/v2/BblfshClientBaseTest.scala, line 18 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Does it split and re-join the file?

Yes, exactly.

That's the shortest API snippet in Scala that:

• reads the file fromFile
• streams the lines
• joins them back with normalized line-ending

[bzz]

I'm a bit lost with reviewable as we've been mixing it with GH reviews here, but @dennwc - all feedback was addressed, so please take another look.

This and #93 blocks further work as all changes go to the same place, so would prefer merge it soon.

[dennwc]

Reviewed 3 of 5 files at r3.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @bzz and @creachadair)

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 136 at r3 (raw file):

Previously, bzz (Alexander) wrote…

Great question! TL;DR - yes it very much does, as a performance optimization.

But doing so correctly requires taking extra care with regards to threading (JNI can be running in different threads) and right now is left as TODO.

If that's alright, I would prefer addressing it later under that TODO, after we get first correct solution working.

Sure, adding it later makes sense 👍

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 308 at r3 (raw file):

Previously, bzz (Alexander) wrote…

You rock @Denys (oh, man! name mentions are not auto-suggested 🤦‍♂️ I will never be able to tag Michael or Oleksandr here..)

But yes, that is definitely is another bug, for the same reason fixed in the same PR.

Yeah, you tagged the wrong user 😅 (@dennwc is the right one, and the issues is here).
OK with solving it in the next PR though.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 323 at r3 (raw file):

Previously, bzz (Alexander) wrote…

True, but this is also a direct copy from https://github.com/bblfsh/client-python/blob/master/bblfsh/pyuast.cc#L626 so if you do not have a strong opinion, I would prefer keeping the behavior same as much as possible.

Sorry, missed that it's private and is not called anywhere except New functions.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 342 at r3 (raw file):

Previously, bzz (Alexander) wrote…

👍 but again - it's a copy from https://github.com/bblfsh/client-python/blob/master/bblfsh/pyuast.cc#L653 and the same idiom is used in 4 other places there, so 🤷‍♂️ if that's one, I'll keep in case that is the only change needed, but will batch up with others otherwise.

The Python client may be inconsistent in style indeed, but it's not an example to follow :D Anyway, resolving this since it won't be solved in this PR anyway.

---

src/test/scala/org/bblfsh/client/v2/BblfshClientBaseTest.scala, line 18 at r1 (raw file):

Previously, bzz (Alexander) wrote…

Yes, exactly.

That's the shortest API snippet in Scala that:

• reads the file fromFile
• streams the lines
• joins them back with normalized line-ending

Isn't there a function to just read all bytes (and let parse to handle it the same way as string)?
It's not a blocker, but it's surprising. It would have some effect on performance.

[bzz]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @creachadair and @dennwc)

---

src/test/scala/org/bblfsh/client/v2/BblfshClientBaseTest.scala, line 18 at r1 (raw file):
Well, this is a part of stdlib, and we have been using this since 1.0 in scala-client CLI, and also those are only the tests.

Isn't there a function to just read all bytes

Of course there are many other ways (new File -> java input stream -> buffered IO) but they are verbose and given the above + the fact that it's not on performance critical path or anything like that - I would prefer simplest way possible to support the notion of "correctness by inspection" inside the tests (as tests do not have tests).

[dennwc]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @creachadair)

---

src/test/scala/org/bblfsh/client/v2/BblfshClientBaseTest.scala, line 18 at r1 (raw file):

Previously, bzz (Alexander) wrote…

Well, this is a part of stdlib, and we have been using this since 1.0 in scala-client CLI, and also those are only the tests.

Isn't there a function to just read all bytes

Of course there are many other ways (new File -> java input stream -> buffered IO) but they are verbose and given the above + the fact that it's not on performance critical path or anything like that - I would prefer simplest way possible to support the notion of "correctness by inspection" inside the tests (as tests do not have tests).

Sorry, confused this with a non-test file.