Cleanup 2020 08 10

[dtonhofer]

After many many days of work, here is an update jpl.pl with a reviewed grammar that parses/generates Java classnames.

Highlights:

• It actually is able to recognize Java classnames that contain $ at interesting places (see below).
• Java classnames that contain illegal characters or that are Java keywords are rejected.
• Java classnames that contain arbitrary unicode characters (e.g. Japanese characters) are accepted.
  • This is done by checking the numeric character code against a set of "runs of allowed values".
  • Therein lies a problem: This code only works on Prologs that use Unicode as internal representation. An atom is transformed to a list of character codes using atom_codes/2, and then each code is numerically checked against the larg-ish list of acceptable unicode code values. If the Prolog uses anything else than Unicode code values in the output of atom_codes/2, this will fail. (By the way, IMAO the "standard" should mandate Unicode internally; there is no upside to leaving this vague)
  • The Java code to generate the set of runs is here: CharRangePrinter.java. It prints the runs for the character codes (Unicode code values) accepted as "first character in a Java identifier" and those accepted as "non-first character in a Java identifier". The result has been copy-pasted into the Prolog source file.

Minor highlights:

• A lot of comments added after many many hours of reverse-engineering and tracing.
• Naming in DCG rules has been cleaned up; hopefully clearer!
• The call-in to the DCGs (i.e. the predicates that call the DCGs) has been cleaned.
• An extremely useful SVG diagram of the call-in to and the calls between DCG predicates is here
• The latest version of the exception-throwing code (i.e. throwme/2) has been thrown into the mix. No real changes, I just noticed that I can get rid of 1 clause.
• Meta-predicates deployed:
  • To get determinacy, the code contained implication instructions: nondet_predicate -> true. I have replaced some with calls to once(Goal) which makes intention clear. There are several places where this can be done still.
  • If-then-else code using imbricated -> is mightily confusing. A couple of places are using a simple if_then_else(IfGoal,ThenGoal,ElseGoal) metapredicate for clarity.

Test code:

• A lot of unit test code has been added to exercise the DCGs.
• Although the old DCGs have been removed from jpl.pl, they have been added to test_jpl.pl, where they are used in "parallel run" tests to verify whether the new and old code behaves the same.
• The gist: All the unit tests should pass!

Gripes:

• These source files are too big. It's so confusing and unwieldy and I really would recommend exploding the source into somaller task-specific modules. Still, I have retained the single jpl.pl. I guess my editor-fu is not yet good enough.

Problems:

• It may be slow. This may be arbitrarily fixed by caching, which is already done in fact. So it may not be slow after all. Why: The biggest slowdown effect would be from the code that recognizes Java identifiers. But the set of those is restricted in a program (they are package name parts and packageless class names), so caching those will work wonders. On the other hand, qualified class names are already cached. Some performance test will have to be done.

• The original code naively but hopelessly tries to separate an unqualified classname into parts by splitting at the '$', assuming for example that whatever comes after a '$' is some kind of inner class. As far as I can distinguish, that assumption is wrong - the unqualified classname is just a string, which may or may not contain '$'. An "inner class" is a syntactic construct of Java the language. That the Java compiler generates a classname with '$' inside for an inner class is pure happenstance. A Clojure compiler may do something entirely different and the JVM doesn't care about '$' in classnames at all. In fact, the original JPL classname parsing code failed when the classname started with a '$', which is entirely legal - that's why I decided to review this. I have added a heuristic to split the classname at '$', but in the end, splitting is a meaningless exercise. It should be abandoned, leading to less code!

• The structure of the terms describing a Java class is lopsided. Although I have retained it, it should be fixed. We have: class(PackagePartsList,ClassPartsList), array(SomeType), method(ArgList,RetVal) but for primitives, we do not have primitive(int) but just int for example. Code becomes assymetric and ambiguous. Eventually, that should be fixed. Note also that the ClassPartsList would reduce to a single ClassPart once the splitting at $ has been removed.

[JanWielemaker]

Lots of work! As said before, JPL is not my cup of tea. I'll leave the content to @ssardina and @anionic. I did quickly go through this and have some remarks.

• Lots of tests are good! Same for cleanup and better comments.
• Please do not export predicates for testing only. Simply use jpl:Goal in the tests to open the black box for testing purposes. Useless exported predicates pollute the predicate space and only confuses users (they appear in the docs as well!)
• Please use "string" for terminals in DCGs. The SWI-Prolog DCG compiler handles these correctly and this retains compatibility.
• I see a user-definition of if_then_else/3 which does the same as (if -> then; else). This is slower (because it results in needless meta calling), as if_then_else/3 is not defined as a meta-predicate it harms source code analysis and it IMO not a good idea not to use established practice in the community you work in.
• jpl_java_id_start_char/1 is indeed rather slow as is. I don't know enough about JPL to see whether this is an issue, i.e., whether or not this can be time critical. If the stuff is cached at the right point it may be ok.

[dtonhofer]

Please do not export predicates for testing only. Simply use jpl:Goal in the tests to open the black box for testing purposes. Useless exported predicates pollute the predicate space and only confuses users (they appear in the docs as well!)

Absolutely. That's a good feature. So you can access and predicate from a Module by qualifying the predicate call?

Fixed.

Please use "string" for terminals in DCGs. The SWI-Prolog DCG compiler handles these correctly and this retains compatibility.

Okay. I was really unsure about what to use here. The original had indeed Strings. So the SWI-Prolog DCG compiler will just transform these into lists of codepoints?

Fixed.

I see a user-definition of if_then_else/3 which does the same as (if -> then; else). This is slower (because it results in needless meta calling), as if_then_else/3 is not defined as a meta-predicate it harms source code analysis and it IMO not a good idea not to use established practice in the community you work in.

Yes... modified, but ... how do I define it as a meta-predicate? I tried to do that in a module but the predicates passed could only call back into the module itself. (One could actually also do a rewrite of the if_the_else 🤔

I really detest the ->/3 notation, it's such abd syntax for no gain except the privilege of giving students and newcomers a hard time. When imbricated it becomes exceedingly hard to read (this can be nicely seen in the jpl code itself btw). The compiler should really flag imbricated ->/3.

Update: I should probably review the generated documentation a bit, too.

[JanWielemaker]

Thanks!

how do I define it as a meta-predicate?

See meta_predicate/1.

I really detest the ->/3 notation

That is up to you. Fact is that the Prolog syntax works like this and everybody in the Prolog community is used to this. Every language has its own quirks. I never heart anyone complaining about (if->then;else) except that most people expect (if->then) to be the same as (if->then;true), which is not the case.

Surely, jpl.pl uses more (if->then;else) than I would like. In part that could be improved, in part that is the nature of a dynamically typed Prolog interface to an imperative language.

[dtonhofer]

More changes:

• Better naming for the DGC calls and the calls calling those.
• jpl_type_to_classname/2 is actually only used to feed legible data to exception terms. Removed all calls from the main ("business") code, passing the JPL Type instead of of the locally constructed "classname" (or rather, "entityname") to the exception-constructing predicates. The exception-constructing predicates can call jpl_type_to_classname/2 if they have to themselves. Much cleaner.
• This also gets rid of jpl_type_to_nicename/2 completely.
• Added some comments to the code block dealing with caching in the Prolog database.
• Collected the :- dynamic directives to directly under the caching code block.
• Updated some comments.
• Replaced two "X -> true" calls to gain determinism by equivalent calls to once/1.
• Added tests for jpl_classname_to_type/2.

[dtonhofer]

Just a moment ... I forget to list all the plunit test blocks at the beginning of test_jpl.pl and one test fails.

[dtonhofer]

More ancillary documentation:

• https://github.com/dtonhofer/prolog_notes/blob/master/code_jpl/java_classname_dcg_for_jpl/pics/jpl_and_java_type_descriptors.svg - An orientation on the JPL types and the Java entitynames/typedescriptors

• https://github.com/dtonhofer/prolog_notes/blob/master/code_jpl/java_classname_dcg_for_jpl/pics/jpl_calling_java_and_the_converse.svg - An orientation on Java calling Prolog or Prolog calling Java

The complexity in this setup is rather amazing.

[dtonhofer]

Refreshing the parsing part one last time. Upload soon (as soon as all the test cases work again).

Completely updated diagram for the parsing/constructing of the Java Strings: https://github.com/dtonhofer/prolog_notes/blob/master/code_jpl/java_classname_dcg_for_jpl/pics/jpl_and_java_type_descriptors.svg

[dtonhofer]

Woo it compiles and all the tests pass.

[dtonhofer]

Public APIs should not use assertion/1 to validate argument types. must_be/2 is intended for this. assertion/1 is intended to trap stuff that should not happen unless there is a bug in the library.

Roger that. Actually I add them as (self) documentation, I get nervous for "anything goes" calls. Will transform.

I know you do not like if->then;else, but please do stick to the SWI-Prolog layout conventions. Different conventions make the code harder to read.

Ok.

The once/1 looks suspicious to me too.

Yes. Actually it replaces the prior "phrase -> true" calls which were mean to ensure determinism (as per the comments: "make sure it is deterministic"), there are several of those. They should not be needed. If we remove them and there actually is nondeterminism, the test cases should warn.

[dtonhofer]

Yet another minor "schoon schip maken".

1. Bad choice of naming: I used "entity_name" throughout instead of "entityname" as is done with with "classname". Fixed.
2. assertion/1 calls have not changed yet
3. Added more comments to the jpl_assert/1 and rearranged code sequence for hopefully easier reading.
4. I rearranged the implication in jpl_type_to_class/2 and a few others (are these you meant, Jan?)
5. Some calls to once(phrase(foo,X)) transformed into phrase(foo,X). If there is any nondeterminacy left (which should not be the case - there is always one solution and nondeterminacy shuold be gotten rid of while parsing of course) I must have forgotten to cut. Then I will have to fix.
6. I did something infelicitous: using atomic_list_concat/2 as a simple replacement of a "here document" in a rarely called exception generator because the message line was annoyingly long. Program structure bleeds into on-screen formatting. Hmmm...

exc_desc(jpl_new_primitive,params_is_bad(Params),
         jpl_new/3,
         domain_error(constructor_args,Params),Msg) :-
   atomic_list_concat([
         'when constructing a new instance of a primitive type, 2nd arg must either be an ',
         'empty list (indicating that the default value of that type is required) or a ',
         'list containing exactly one representation of a suitable value'],Msg).

[dtonhofer]

All right, what are we going to do with this?

[anionic]

I'm happy with it - presumably it now accommodates LibreOffice's use of $ in classnames as well as everything which used to work ;-)

[JanWielemaker]

Ok. Merged as is (well, after rebase and fix white space errors). Te tests still pass.

6\. I did something infelicitous: using `atomic_list_concat/2` as a simple replacement of a ["here document"](https://en.wikipedia.org/wiki/Here_document) in a rarely called exception generator because the message line was annoyingly long. Program structure bleeds into on-screen formatting. Hmmm...

The (SWI-)Prolog way is

   ..., 'this is a long string \c
         that I continue on the next line \c
         etc.'

That is just as readable. It is not ISO compliant (ISO dropped this Quintus feature), but ISO has no way to deal with this elegantly. atomic_list_concat/3 isn't ISO either ...