Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.4, 6.0
    • Component/s: modules/expressions
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I would like to upgrade ANTLR from 3.5 to 4.5. This version adds several features that will improve the existing grammars. The main improvement would be the allowance of left-hand recursion in grammar rules which will reduce the number of rules significantly for expressions.

      This change will require some code refactoring to the existing expressions work.

      1. LUCENE-6417.patch
        243 kB
        Uwe Schindler
      2. LUCENE-6417.patch
        237 kB
        Jack Conradson
      3. LUCENE-6417-cleanups.patch
        16 kB
        Uwe Schindler
      4. LUCENE-6471.patch
        240 kB
        Uwe Schindler
      5. LUCENE-6471.patch
        227 kB
        Jack Conradson
      6. LUCENE-6471.patch
        227 kB
        Jack Conradson

        Activity

        Hide
        Jack Conradson added a comment - - edited

        This patch contains the code necessary to upgrade to ANTLR 4.5.

        The following changes are included:

        • Updated the build.xml and ivy.xml to use ANTLR 4.5 instead of the prior version.
        • Modified the grammar to use only two parser rules including a left-hand recursive rule for the majority of the language. Simplified the lexer rules.
        • Used the provided visitor pattern classes ANTLR 4.5 provides to move through the ANTLR parse tree. The JavascriptCompiler nows inherits from a JavascriptBaseVisitor class. While this does add some extra methods, it's also a well-known design pattern that can be easily modified by future contributors. This allows for easy use of new rule context objects, so that some tokens may be ignored that aren't necessary. Note that ANTLR 4.5 no longer has ASTs because it does not allow for tree rewriting. The provided parse tree required some changes to the compiler since the root nodes now contain 3 children with something like '2' ' - ' '3' rather than tokens such as ' - ' with 2 children.
        • Moved all error handling out of the grammar file making the grammar language agnostic. Error handling is now part of separate java classes.
        Show
        Jack Conradson added a comment - - edited This patch contains the code necessary to upgrade to ANTLR 4.5. The following changes are included: Updated the build.xml and ivy.xml to use ANTLR 4.5 instead of the prior version. Modified the grammar to use only two parser rules including a left-hand recursive rule for the majority of the language. Simplified the lexer rules. Used the provided visitor pattern classes ANTLR 4.5 provides to move through the ANTLR parse tree. The JavascriptCompiler nows inherits from a JavascriptBaseVisitor class. While this does add some extra methods, it's also a well-known design pattern that can be easily modified by future contributors. This allows for easy use of new rule context objects, so that some tokens may be ignored that aren't necessary. Note that ANTLR 4.5 no longer has ASTs because it does not allow for tree rewriting. The provided parse tree required some changes to the compiler since the root nodes now contain 3 children with something like '2' ' - ' '3' rather than tokens such as ' - ' with 2 children. Moved all error handling out of the grammar file making the grammar language agnostic. Error handling is now part of separate java classes.
        Hide
        Michael McCandless added a comment -

        Thanks Jack Conradson, it looks like a lot changed between ANTLR 3.5 and 4.5!

        I confirmed tests pass with your patch, then I ran ant run-antlr in the expressions module and tests still pass. I don't understand all the changes but I trust you do! Is this ready to be committed? Maybe Robert Muir wants to review this...

        I'll fix the jar licenses/sha1 issues, but ant precommit is angry about this:

        -ecj-javadoc-lint-src:
            [mkdir] Created dir: /tmp/ecj949391657
         [ecj-lint] Compiling 21 source files to /tmp/ecj949391657
         [ecj-lint] ----------
         [ecj-lint] 1. ERROR in /l/antlr/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptParserErrorStrategy.java (at line 72)
         [ecj-lint] 	* @throws RecognitionException
         [ecj-lint] 	          ^^^^^^^^^^^^^^^^^^^^
         [ecj-lint] Javadoc: Description expected after this reference
         [ecj-lint] ----------
         [ecj-lint] 1 problem (1 error)
        

        What description should I add...?

        Show
        Michael McCandless added a comment - Thanks Jack Conradson , it looks like a lot changed between ANTLR 3.5 and 4.5! I confirmed tests pass with your patch, then I ran ant run-antlr in the expressions module and tests still pass. I don't understand all the changes but I trust you do! Is this ready to be committed? Maybe Robert Muir wants to review this... I'll fix the jar licenses/sha1 issues, but ant precommit is angry about this: -ecj-javadoc-lint-src: [mkdir] Created dir: /tmp/ecj949391657 [ecj-lint] Compiling 21 source files to /tmp/ecj949391657 [ecj-lint] ---------- [ecj-lint] 1. ERROR in /l/antlr/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptParserErrorStrategy.java (at line 72) [ecj-lint] * @throws RecognitionException [ecj-lint] ^^^^^^^^^^^^^^^^^^^^ [ecj-lint] Javadoc: Description expected after this reference [ecj-lint] ---------- [ecj-lint] 1 problem (1 error) What description should I add...?
        Hide
        Uwe Schindler added a comment - - edited

        Hi Jack, I will tomorrow look into the ASM parts. I rewrote that after the code donation from A9 with Robert. I just want to make sure the Java Bytecode is created in most efficient way Did you change the code in general or is this just moving the byte code generator fragments around in the code?

        Show
        Uwe Schindler added a comment - - edited Hi Jack, I will tomorrow look into the ASM parts. I rewrote that after the code donation from A9 with Robert. I just want to make sure the Java Bytecode is created in most efficient way Did you change the code in general or is this just moving the byte code generator fragments around in the code?
        Hide
        Uwe Schindler added a comment -

        Ah, I would replace the old-style Java 1.0 java.util.Stack by java.util.ArrayDeque (which is not synchronized). This implements Deque, which basically a generalization of array-based stack:

        Deque<Type> typeStack = new ArrayDeque<>();
        
        Show
        Uwe Schindler added a comment - Ah, I would replace the old-style Java 1.0 java.util.Stack by java.util.ArrayDeque (which is not synchronized). This implements Deque, which basically a generalization of array-based stack: Deque<Type> typeStack = new ArrayDeque<>();
        Hide
        Jack Conradson added a comment - - edited

        Sorry, Mike and Uwe, for the delayed response. I was actually on vacation the previous week. I'll fix the pre-commit and make the suggested changes. The ASM did not change at all (other than by mistake ), I was just forced to re-organize the ASM generation code into smaller chunks for the Antlr 4.5 visitor. Thanks for reviewing the patch!

        Show
        Jack Conradson added a comment - - edited Sorry, Mike and Uwe, for the delayed response. I was actually on vacation the previous week. I'll fix the pre-commit and make the suggested changes. The ASM did not change at all (other than by mistake ), I was just forced to re-organize the ASM generation code into smaller chunks for the Antlr 4.5 visitor. Thanks for reviewing the patch!
        Hide
        Jack Conradson added a comment - - edited

        Attached an updated patch. I think precommit passes now other than the license issue. Also converted the Stack to an ArrayDeque.

        Show
        Jack Conradson added a comment - - edited Attached an updated patch. I think precommit passes now other than the license issue. Also converted the Stack to an ArrayDeque.
        Hide
        Uwe Schindler added a comment -

        Thanks Jack! I am fine with the patch! Only the Deque<Type> declaration should be final (no new patch needed).

        About the assembly: I was expecting this, I just wanted to make sure that the refactoring caused no changes. Maybe dump the class files to disk for old and new and compare afterwards - but if tests work as expected, we should be quite safe - maybe we need some random stuff here (with lots of recursions).

        Robert and I dumped the class files when we refactored this 2 years ago.

        Show
        Uwe Schindler added a comment - Thanks Jack! I am fine with the patch! Only the Deque<Type> declaration should be final (no new patch needed). About the assembly: I was expecting this, I just wanted to make sure that the refactoring caused no changes. Maybe dump the class files to disk for old and new and compare afterwards - but if tests work as expected, we should be quite safe - maybe we need some random stuff here (with lots of recursions). Robert and I dumped the class files when we refactored this 2 years ago.
        Hide
        Uwe Schindler added a comment -

        I will take care of this issue! Thanks for reporting.

        I would also suggest to update ASM to 5.0.x in the future, especially because we can then create Java 8 class files in trunk (which is not really needed as we use no default interface methods, but might be good idea).

        Show
        Uwe Schindler added a comment - I will take care of this issue! Thanks for reporting. I would also suggest to update ASM to 5.0.x in the future, especially because we can then create Java 8 class files in trunk (which is not really needed as we use no default interface methods, but might be good idea).
        Hide
        Uwe Schindler added a comment -

        I will do precommit tests and then commit this patch (including the final Deque later today).

        Show
        Uwe Schindler added a comment - I will do precommit tests and then commit this patch (including the final Deque later today).
        Hide
        Jack Conradson added a comment -

        Thanks for picking this up, Uwe. Please let me know if you need anything else from me.

        Show
        Jack Conradson added a comment - Thanks for picking this up, Uwe. Please let me know if you need anything else from me.
        Hide
        Uwe Schindler added a comment -

        I fixed the build now to have all required license files (will attach patch).

        We still have one problem: The ANTLR code has now many subclasses which miss javadocs. Our Javadocs-Linter now breaks on precommit with:

        -documentation-lint:
             [echo] checking for broken html...
            [jtidy] Checking for broken html (such as invalid tags)...
           [delete] Deleting directory C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\lucene\build\jtidy_tmp
             [echo] Checking for broken links...
             [exec]
             [exec] Crawl/parse...
             [exec]
             [exec] Verify...
             [exec]
             [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptVisitor.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/expressions.js.JavascriptParser.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html
             [exec]
             [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/package-summary.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/expressions.js.JavascriptParser.html
             [exec]
             [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/package-use.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/expressions.js.JavascriptParser.html
             [exec]
             [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptCompiler.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html
             [exec]   BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html
             [exec]
             [exec] Broken javadocs links were found!
        

        Indeed the problem is: The main class is package protected, but the inner classes generated by ANTLR are public, which violates visibility. We should also make them package protected. I am not sure how to do that without regular expressions in "ant regenerate".

        Show
        Uwe Schindler added a comment - I fixed the build now to have all required license files (will attach patch). We still have one problem: The ANTLR code has now many subclasses which miss javadocs. Our Javadocs-Linter now breaks on precommit with: -documentation-lint: [echo] checking for broken html... [jtidy] Checking for broken html (such as invalid tags)... [delete] Deleting directory C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\lucene\build\jtidy_tmp [echo] Checking for broken links... [exec] [exec] Crawl/parse... [exec] [exec] Verify... [exec] [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptVisitor.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/expressions.js.JavascriptParser.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html [exec] [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/package-summary.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/expressions.js.JavascriptParser.html [exec] [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/package-use.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/expressions.js.JavascriptParser.html [exec] [exec] file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptCompiler.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.CompileContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.PrecedenceContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.NumericContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ExternalContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.UnaryContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.MuldivContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.AddsubContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwshiftContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolcompContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BooleqneContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BwxorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BworContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolandContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.BoolorContext.html [exec] BROKEN LINK: file:///build/docs/expressions/org/apache/lucene/expressions/js/JavascriptParser.ConditionalContext.html [exec] [exec] Broken javadocs links were found! Indeed the problem is: The main class is package protected, but the inner classes generated by ANTLR are public, which violates visibility. We should also make them package protected. I am not sure how to do that without regular expressions in "ant regenerate".
        Hide
        Uwe Schindler added a comment -

        Updated patch with all required license file changes, sha1 sums and changes to Solr's Ivy to use correct antlr version.

        This still has the visibility problem causing javadocs to fail. I will check out whats possible. Jack Conradson, if you know how to make the inner classes package protected, please tell me, as I have a svn checkout ready to commit, I just need to fix the visibility problem in Javadocs.

        Show
        Uwe Schindler added a comment - Updated patch with all required license file changes, sha1 sums and changes to Solr's Ivy to use correct antlr version. This still has the visibility problem causing javadocs to fail. I will check out whats possible. Jack Conradson , if you know how to make the inner classes package protected, please tell me, as I have a svn checkout ready to commit, I just need to fix the visibility problem in Javadocs.
        Hide
        Uwe Schindler added a comment - - edited

        I was trying to fix this, but the problem is: those inner classes are now part of public APIs (the visitors). So we must either make all visitor interfaces and the implementations hidden, which required impl methods pkg-protected, too (which is impossible to java: interface implementations have to be public). Alternatively we should remove the package private stuff completely and make everything public. But this f*cks up the API, because we wanted to have the whole ANT-stuff package private and therefore hidden from javadocs as implementation detail. Using the visitor interfaces, this is now impossible.

        Sorry, I cannot commit this in this stage... We have to take care of this first. Just generate the Javadocs then look at JavascriptCompiler's Javadoc and you will see the problem.

        Show
        Uwe Schindler added a comment - - edited I was trying to fix this, but the problem is: those inner classes are now part of public APIs (the visitors). So we must either make all visitor interfaces and the implementations hidden, which required impl methods pkg-protected, too (which is impossible to java: interface implementations have to be public). Alternatively we should remove the package private stuff completely and make everything public. But this f*cks up the API, because we wanted to have the whole ANT-stuff package private and therefore hidden from javadocs as implementation detail. Using the visitor interfaces, this is now impossible. Sorry, I cannot commit this in this stage... We have to take care of this first. Just generate the Javadocs then look at JavascriptCompiler's Javadoc and you will see the problem.
        Hide
        Jack Conradson added a comment -

        Hey Uwe, I understand the problem. I need to give this some thought as I don't have a good solution off the top of my head.

        Show
        Jack Conradson added a comment - Hey Uwe, I understand the problem. I need to give this some thought as I don't have a good solution off the top of my head.
        Hide
        Jack Conradson added a comment - - edited

        Uwe, would it be possible to move the implemented visitor methods from the JavascriptCompiler into a private static inner class inside of the JavascriptCompiler to hide them properly and then make all the other public classes package protected through regexs like we already do for some of the classes in the build.xml file? Would this satisfy the requirements of keeping all the ANTLR methods hidden or is there more that I'm missing?

        The other option that I can think of is to go back to parsing the raw tree again, but I prefer to avoid this if possible due to changes in the tree structures between ANTLR 3 and 4.

        Please let me know what you think.

        Show
        Jack Conradson added a comment - - edited Uwe, would it be possible to move the implemented visitor methods from the JavascriptCompiler into a private static inner class inside of the JavascriptCompiler to hide them properly and then make all the other public classes package protected through regexs like we already do for some of the classes in the build.xml file? Would this satisfy the requirements of keeping all the ANTLR methods hidden or is there more that I'm missing? The other option that I can think of is to go back to parsing the raw tree again, but I prefer to avoid this if possible due to changes in the tree structures between ANTLR 3 and 4. Please let me know what you think.
        Hide
        Robert Muir added a comment -

        nobody wants it, but a heavy regex solution can work

        Show
        Robert Muir added a comment - nobody wants it, but a heavy regex solution can work
        Hide
        Jack Conradson added a comment -

        Robert Muir then maybe the best solution is to drop the visitor and parse the tree as before? As a possible third option, I checked to see if it was possible to easily modify the string template files used to generate the code from the grammars, but there's no easy way to point to a custom one. We might be able to still do this, but it would either be through some fragile classes that I don't anticipate were meant to be overridden or by injecting a custom template file into a copied version of the antler 4 jar – definitely not a pretty solution.

        Show
        Jack Conradson added a comment - Robert Muir then maybe the best solution is to drop the visitor and parse the tree as before? As a possible third option, I checked to see if it was possible to easily modify the string template files used to generate the code from the grammars, but there's no easy way to point to a custom one. We might be able to still do this, but it would either be through some fragile classes that I don't anticipate were meant to be overridden or by injecting a custom template file into a copied version of the antler 4 jar – definitely not a pretty solution.
        Hide
        Uwe Schindler added a comment - - edited

        Hi,
        there is actually a solution without any additional regexes. The main problem are not the inner autogenerated *Context classes. They can stay public, because the outer class is pkg private. The problem are more interfaces and their implementations using those private classes in public signatures.

        The problem here is the following: JavascriptCompiter is public API but it implements an interface using pkg private classes in method signatures. This causes issues, because all methods implementing an interface must be public Because JavascriptCompiter is public those methods get visible to everybody, but their signatures use pkg-protected (sub-)classes. This breaks visibility and Javadocs linting fails (404 not found errors).

        The solution is the following:

        • make JavascriptErrorHandlingLexer, JavascriptParserErrorStrategy pkg private (its impl detail)
        • make JavascriptVisitor interface pkg-private (this one is the main issue)
        • the problem is now that JavaScriptCompiler implements JavascriptVisitor (through extending JavascriptBaseVisitor), and therefore all implemented methods get public (which is bad). The workaround is the following: Make the actual visitor implementation not part of public API, instead create the visitor (that extends JavascriptBaseVisitor) inside JavascriptCompiler as anonymous, private instance and call the visit method on it, but not extend the BaseVisitor and the "bad" interface directly in the top-level class. Because it will get an anonymous inner class it has access to all methods/fields of JavascriptCompiler, but the implemented interface and abstract class keeps private. You can move most methods handling the visiting process into the anonymous inner class. On top-level you just have the compiler (as before).

        I have not much time today to implement this, but this might work. Jack Conradson: If you work on this, can you use my patch as base? I already solved the license issues, so precommit passes, except the visibility problems.

        Finally, the VariableContext should be pkg-private, too - it was made public in some earlier commit, but it does not need to be public at all (I think?).

        Show
        Uwe Schindler added a comment - - edited Hi, there is actually a solution without any additional regexes. The main problem are not the inner autogenerated *Context classes. They can stay public, because the outer class is pkg private. The problem are more interfaces and their implementations using those private classes in public signatures. The problem here is the following: JavascriptCompiter is public API but it implements an interface using pkg private classes in method signatures. This causes issues, because all methods implementing an interface must be public Because JavascriptCompiter is public those methods get visible to everybody, but their signatures use pkg-protected (sub-)classes. This breaks visibility and Javadocs linting fails (404 not found errors). The solution is the following: make JavascriptErrorHandlingLexer, JavascriptParserErrorStrategy pkg private (its impl detail) make JavascriptVisitor interface pkg-private (this one is the main issue) the problem is now that JavaScriptCompiler implements JavascriptVisitor (through extending JavascriptBaseVisitor), and therefore all implemented methods get public (which is bad). The workaround is the following: Make the actual visitor implementation not part of public API, instead create the visitor (that extends JavascriptBaseVisitor) inside JavascriptCompiler as anonymous, private instance and call the visit method on it, but not extend the BaseVisitor and the "bad" interface directly in the top-level class. Because it will get an anonymous inner class it has access to all methods/fields of JavascriptCompiler, but the implemented interface and abstract class keeps private. You can move most methods handling the visiting process into the anonymous inner class. On top-level you just have the compiler (as before). I have not much time today to implement this, but this might work. Jack Conradson : If you work on this, can you use my patch as base? I already solved the license issues, so precommit passes, except the visibility problems. Finally, the VariableContext should be pkg-private, too - it was made public in some earlier commit, but it does not need to be public at all (I think?).
        Hide
        Jack Conradson added a comment - - edited

        Uwe, this sounds like a reasonable solution. I'll try to get a new patch up later today. Thanks for all the feedback so far. I'm fairly confident the VariableContext is meant to be used outside of the package for help parsing variables into their individual pieces –

        x.y.z --> x and y and z as individual pieces
        map['string'].t --> map and string and t

        So it doesn't make sense to have it be package-private.

        Show
        Jack Conradson added a comment - - edited Uwe, this sounds like a reasonable solution. I'll try to get a new patch up later today. Thanks for all the feedback so far. I'm fairly confident the VariableContext is meant to be used outside of the package for help parsing variables into their individual pieces – x.y.z --> x and y and z as individual pieces map ['string'] .t --> map and string and t So it doesn't make sense to have it be package-private.
        Hide
        Uwe Schindler added a comment -

        I'm fairly confident the VariableContext is meant to be used outside of the package for help parsing variables into their individual pieces

        OK. I just have not seen any method signature taking the context, so I had the feeling its internal only. But thats unrelated to this issue.

        Show
        Uwe Schindler added a comment - I'm fairly confident the VariableContext is meant to be used outside of the package for help parsing variables into their individual pieces OK. I just have not seen any method signature taking the context, so I had the feeling its internal only. But thats unrelated to this issue.
        Hide
        Jack Conradson added a comment - - edited

        Attached a new patch with the requested changes. All classes should be package-private now except for VariableContext and the JavascriptCompiler. The ANTLR visitor is now encapsulated in an anonymous inner class to hide the implementation details of the compiler.

        Show
        Jack Conradson added a comment - - edited Attached a new patch with the requested changes. All classes should be package-private now except for VariableContext and the JavascriptCompiler. The ANTLR visitor is now encapsulated in an anonymous inner class to hide the implementation details of the compiler.
        Hide
        Uwe Schindler added a comment -

        Thanks Jack,
        I will look into this later. I am out of office at the moment! But from my quick inspection, this looks really fine!
        Uwe

        Show
        Uwe Schindler added a comment - Thanks Jack, I will look into this later. I am out of office at the moment! But from my quick inspection, this looks really fine! Uwe
        Hide
        Uwe Schindler added a comment -

        Hi,
        patch looks good. I did some minor changes at the visibility of the JavascriptCompiler fields to prevent acess$X methods (Eclipse warning about synthetic access) and imported everything successfully into subversion. I also changed the visibility of the error listeners/handlers to pkg private, the javadocs now look identical to before.

        "ant precommit" passes and all maven dependencies are fine.

        "ant regenerate" recreates the files without overriding my visibility changes.

        I will run all tests tomorrow and commit, it's too late now. Thanks Jack!

        Show
        Uwe Schindler added a comment - Hi, patch looks good. I did some minor changes at the visibility of the JavascriptCompiler fields to prevent acess$X methods (Eclipse warning about synthetic access) and imported everything successfully into subversion. I also changed the visibility of the error listeners/handlers to pkg private, the javadocs now look identical to before. "ant precommit" passes and all maven dependencies are fine. "ant regenerate" recreates the files without overriding my visibility changes. I will run all tests tomorrow and commit, it's too late now. Thanks Jack!
        Hide
        Jack Conradson added a comment -

        Thanks again, Uwe, for all the feedback and taking the time to get the patch into Lucene.

        Show
        Jack Conradson added a comment - Thanks again, Uwe, for all the feedback and taking the time to get the patch into Lucene.
        Hide
        ASF subversion and git services added a comment -

        Commit 1694614 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1694614 ]

        LUCENE-6417: Upgrade ANTLR used in expressions module to version 4.5

        Show
        ASF subversion and git services added a comment - Commit 1694614 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1694614 ] LUCENE-6417 : Upgrade ANTLR used in expressions module to version 4.5
        Hide
        ASF subversion and git services added a comment -

        Commit 1694615 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1694615 ]

        Merged revision(s) 1694614 from lucene/dev/trunk:
        LUCENE-6417: Upgrade ANTLR used in expressions module to version 4.5

        Show
        ASF subversion and git services added a comment - Commit 1694615 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694615 ] Merged revision(s) 1694614 from lucene/dev/trunk: LUCENE-6417 : Upgrade ANTLR used in expressions module to version 4.5
        Hide
        Uwe Schindler added a comment -

        Thanks Jack!

        Show
        Uwe Schindler added a comment - Thanks Jack!
        Hide
        ASF subversion and git services added a comment -

        Commit 1694650 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1694650 ]

        LUCENE-6417: Add missing licenses/checksums in 5.x (Solr had no antlr4 in 5.x - no presto parser)

        Show
        ASF subversion and git services added a comment - Commit 1694650 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694650 ] LUCENE-6417 : Add missing licenses/checksums in 5.x (Solr had no antlr4 in 5.x - no presto parser)
        Hide
        Uwe Schindler added a comment -

        I have some cleanups for the ANTLR stuff (build.xml):

        • Remove useless regular expressions
        • Don't copy-paste always the same, use a patternset for the files to edit
        • remove the {.tokens}

          files after regenerating, they are not needed

        • rename the grammar Javascript.g to Javascript.g4 (the ANTLR documentation suggests g4 for the new grammar files, so editors can detect the type correctly)
        Show
        Uwe Schindler added a comment - I have some cleanups for the ANTLR stuff (build.xml): Remove useless regular expressions Don't copy-paste always the same, use a patternset for the files to edit remove the {.tokens} files after regenerating, they are not needed rename the grammar Javascript.g to Javascript.g4 (the ANTLR documentation suggests g4 for the new grammar files, so editors can detect the type correctly)
        Hide
        Uwe Schindler added a comment -

        Reopening for some fixes.

        Show
        Uwe Schindler added a comment - Reopening for some fixes.
        Hide
        ASF subversion and git services added a comment -

        Commit 1694865 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1694865 ]

        LUCENE-6417: Cleanup ANTLR code generator

        Show
        ASF subversion and git services added a comment - Commit 1694865 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1694865 ] LUCENE-6417 : Cleanup ANTLR code generator
        Hide
        ASF subversion and git services added a comment -

        Commit 1694866 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1694866 ]

        Merged revision(s) 1694865 from lucene/dev/trunk:
        LUCENE-6417: Cleanup ANTLR code generator

        Show
        ASF subversion and git services added a comment - Commit 1694866 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694866 ] Merged revision(s) 1694865 from lucene/dev/trunk: LUCENE-6417 : Cleanup ANTLR code generator
        Hide
        ASF subversion and git services added a comment -

        Commit 1694876 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1694876 ]

        LUCENE-6417: Make JavascriptCompiler completely stateless (thanks to visitor pattern)

        Show
        ASF subversion and git services added a comment - Commit 1694876 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1694876 ] LUCENE-6417 : Make JavascriptCompiler completely stateless (thanks to visitor pattern)
        Hide
        ASF subversion and git services added a comment -

        Commit 1694879 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1694879 ]

        LUCENE-6417: make params final (for Java 7)

        Show
        ASF subversion and git services added a comment - Commit 1694879 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1694879 ] LUCENE-6417 : make params final (for Java 7)
        Hide
        ASF subversion and git services added a comment -

        Commit 1694880 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1694880 ]

        Merged revision(s) 1694876,1694879 from lucene/dev/trunk:
        LUCENE-6417: Make JavascriptCompiler completely stateless (thanks to visitor pattern)

        Show
        ASF subversion and git services added a comment - Commit 1694880 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694880 ] Merged revision(s) 1694876,1694879 from lucene/dev/trunk: LUCENE-6417 : Make JavascriptCompiler completely stateless (thanks to visitor pattern)
        Hide
        Uwe Schindler added a comment - - edited

        I just committed a change to make JavaScriptCompiler completely stateless, this was non-ideal before, but thanks to separate visitor this is now much more elegant.

        Show
        Uwe Schindler added a comment - - edited I just committed a change to make JavaScriptCompiler completely stateless, this was non-ideal before, but thanks to separate visitor this is now much more elegant.
        Hide
        ASF subversion and git services added a comment -

        Commit 1694897 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1694897 ]

        LUCENE-6417: remove useless import

        Show
        ASF subversion and git services added a comment - Commit 1694897 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1694897 ] LUCENE-6417 : remove useless import
        Hide
        ASF subversion and git services added a comment -

        Commit 1694898 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1694898 ]

        Merged revision(s) 1694897 from lucene/dev/trunk:
        LUCENE-6417: remove useless import

        Show
        ASF subversion and git services added a comment - Commit 1694898 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1694898 ] Merged revision(s) 1694897 from lucene/dev/trunk: LUCENE-6417 : remove useless import

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Jack Conradson
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development