Fix BSTVM (#11304)

* test ieeetran with test entry * fix * Fix path to IEEEtran.bst * Streamline conversion to BstEntry * Somre more traces * Add '' * WIP: more tracing (and fix of identifier resolution) Co-authored-by: Christoph <siedlerkiller@gmail.com> * Some more traces (and cleaner code for bstSubstring) * New .withField syntax for TestEntry * Remove trace logging configuration * Fix expected string * Final cleanup * checkstyle * fix tests --------- Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>
JabRef · May 20, 2024 · c4c0a23 · c4c0a23
1 parent dfe1b0c
commit c4c0a23
Show file tree

Hide file tree

Showing 7 changed files with 144 additions and 70 deletions.
diff --git a/src/main/java/org/jabref/logic/bst/BstEntry.java b/src/main/java/org/jabref/logic/bst/BstEntry.java
@@ -9,12 +9,15 @@ public class BstEntry {
 
     public final BibEntry entry;
 
-    public final Map<String, String> localStrings = new HashMap<>();
-
+    // ENTRY: First sub list
     public final Map<String, String> fields = new HashMap<>();
 
+    // ENTRY: Second sub list
     public final Map<String, Integer> localIntegers = new HashMap<>();
 
+    // ENTRY: Third sub list
+    public final Map<String, String> localStrings = new HashMap<>();
+
     public BstEntry(BibEntry e) {
         this.entry = e;
     }

diff --git a/src/main/java/org/jabref/logic/bst/BstFunctions.java b/src/main/java/org/jabref/logic/bst/BstFunctions.java
@@ -16,6 +16,7 @@
 import org.jabref.model.entry.Author;
 import org.jabref.model.entry.AuthorList;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.antlr.v4.runtime.ParserRuleContext;
 import org.antlr.v4.runtime.tree.ParseTree;
 import org.slf4j.Logger;
@@ -310,9 +311,9 @@ private void bstAddPeriod(BstVMVisitor visitor, ParserRuleContext ctx) {
      * the book function. When given as an argument to the ITERATE
      * command, call.type$ actually produces the output for the entries.
      * For an entry with an unknown type, it executes the function
-     * default.type. Thus you should define (before the READ command)
+     * default.type. Thus, you should define (before the READ command)
      * one function for each standard entry type as well as a
-     * default.type function.
+     * <code>default.type</code> function.
      */
     public class BstCallTypeFunction implements BstFunction {
         @Override
@@ -325,7 +326,9 @@ public void execute(BstVMVisitor visitor, ParserRuleContext ctx, BstEntry bstEnt
             if (bstEntry == null) {
                 this.execute(visitor, ctx); // Throw error
             } else {
-                functions.get(bstEntry.entry.getType().getName()).execute(visitor, ctx, bstEntry);
+                String entryType = bstEntry.entry.getType().getName();
+                LOGGER.trace("Handling {}", entryType);
+                functions.get(entryType).execute(visitor, ctx, bstEntry);
             }
         }
     }
@@ -431,6 +434,7 @@ private void bstEmpty(BstVMVisitor visitor, ParserRuleContext ctx) {
         Object o1 = stack.pop();
 
         if (o1 == null) {
+            LOGGER.trace("null is empty");
             stack.push(BstVM.TRUE);
             return;
         }
@@ -439,7 +443,9 @@ private void bstEmpty(BstVMVisitor visitor, ParserRuleContext ctx) {
             throw new BstVMException("Operand does not match function empty$ (line %d)".formatted(ctx.start.getLine()));
         }
 
-        stack.push("".equals(s.trim()) ? BstVM.TRUE : BstVM.FALSE);
+        boolean result = "".equals(s.trim());
+        LOGGER.trace("empty$({}) result: {}", s, result);
+        stack.push(result ? BstVM.TRUE : BstVM.FALSE);
     }
 
     /**
@@ -679,43 +685,47 @@ private void bstStack(BstVMVisitor visitor, ParserRuleContext ctx) {
      * Pops the top three literals (they are the two integers literals
      * len and start, and a string literal, in that order). It pushes
      * the substring of the (at most) len consecutive characters
-     * starting at the startth character (assuming 1-based indexing) if
+     * starting at the start-th character (assuming 1-based indexing) if
      * start is positive, and ending at the start-th character
      * (including) from the end if start is negative (where the first
      * character from the end is the last character).
      */
-    private void bstSubstring(BstVMVisitor visitor, ParserRuleContext ctx) {
+    @VisibleForTesting
+    void bstSubstring(BstVMVisitor visitor, ParserRuleContext ctx) {
         if (stack.size() < 3) {
             throw new BstVMException("Not enough operands on stack for operation substring$ (line %d)".formatted(ctx.start.getLine()));
         }
         Object o1 = stack.pop();
         Object o2 = stack.pop();
         Object o3 = stack.pop();
 
-        if (!((o1 instanceof Integer len) && (o2 instanceof Integer start) && (o3 instanceof String s))) {
+        if (!((o1 instanceof Integer length) && (o2 instanceof Integer start) && (o3 instanceof String string))) {
             throw new BstVMException("Expecting two integers and a string for substring$ (line %d)".formatted(ctx.start.getLine()));
         }
 
-        int lenI = len;
-        int startI = start;
-
-        if (lenI > (Integer.MAX_VALUE / 2)) {
-            lenI = Integer.MAX_VALUE / 2;
+        if (length > (Integer.MAX_VALUE / 2)) {
+            length = Integer.MAX_VALUE / 2;
         }
 
-        if (startI > (Integer.MAX_VALUE / 2)) {
-            startI = Integer.MAX_VALUE / 2;
+        if (start > (Integer.MAX_VALUE / 2)) {
+            start = Integer.MAX_VALUE / 2;
         }
 
-        if (startI < (Integer.MIN_VALUE / 2)) {
-            startI = -Integer.MIN_VALUE / 2;
+        if (start < (Integer.MIN_VALUE / 2)) {
+            start = -Integer.MIN_VALUE / 2;
         }
 
-        if (startI < 0) {
-            startI += s.length() + 1;
-            startI = Math.max(1, (startI + 1) - lenI);
+        if (start < 0) {
+            int endOneBased = string.length() + start + 1;
+            start = Math.max(1, endOneBased - length + 1);
+            length = endOneBased - start + 1;
         }
-        stack.push(s.substring(startI - 1, Math.min((startI - 1) + lenI, s.length())));
+
+        int zeroBasedStart = start - 1;
+        String result = string.substring(zeroBasedStart, Math.min(zeroBasedStart + length, string.length()));
+
+        LOGGER.trace("substring$(s, start, len): ({}, {}, {})={}", string, start, length, result);
+        stack.push(result);
     }
 
     /**

diff --git a/src/main/java/org/jabref/logic/bst/BstVM.java b/src/main/java/org/jabref/logic/bst/BstVM.java
@@ -2,11 +2,11 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Deque;
 import java.util.List;
 import java.util.Objects;
+import java.util.stream.Collectors;
 
 import org.jabref.model.database.BibDatabase;
 import org.jabref.model.entry.BibEntry;
@@ -69,10 +69,8 @@ private static ParseTree charStream2CommonTree(CharStream query) {
     public String render(Collection<BibEntry> bibEntries, BibDatabase bibDatabase) {
         Objects.requireNonNull(bibEntries);
 
-        List<BstEntry> entries = new ArrayList<>(bibEntries.size());
-        for (BibEntry entry : bibEntries) {
-            entries.add(new BstEntry(entry));
-        }
+        // needs to be modifiable due to sort operations later
+        List<BstEntry> entries = bibEntries.stream().map(BstEntry::new).collect(Collectors.toList());
 
         StringBuilder resultBuffer = new StringBuilder();
 

diff --git a/src/main/java/org/jabref/logic/bst/BstVMVisitor.java b/src/main/java/org/jabref/logic/bst/BstVMVisitor.java
@@ -58,7 +58,9 @@ public Integer visitIntegersCommand(BstParser.IntegersCommandContext ctx) {
 
     @Override
     public Integer visitFunctionCommand(BstParser.FunctionCommandContext ctx) {
-        bstVMContext.functions().put(ctx.id.getText(),
+        String name = ctx.id.getText();
+        LOGGER.trace("Function: {}", name);
+        bstVMContext.functions().put(name,
                 (visitor, functionContext) -> visitor.visit(ctx.function));
         return BstVM.TRUE;
     }
@@ -115,18 +117,24 @@ public Integer visitReadCommand(BstParser.ReadCommandContext ctx) {
     @Override
     public Integer visitExecuteCommand(BstParser.ExecuteCommandContext ctx) {
         this.selectedBstEntry = null;
-        visit(ctx.bstFunction());
+        BstParser.BstFunctionContext bstFunction = ctx.bstFunction();
+        String name = bstFunction.getText();
+        LOGGER.trace("Executing function {}", name);
+        visit(bstFunction);
+        LOGGER.trace("Finished executing function {}", name);
 
         return BstVM.TRUE;
     }
 
     @Override
     public Integer visitIterateCommand(BstParser.IterateCommandContext ctx) {
+        String name = ctx.bstFunction().getText();
+        LOGGER.trace("Executing {}", name);
         for (BstEntry entry : bstVMContext.entries()) {
             this.selectedBstEntry = entry;
             visit(ctx.bstFunction());
         }
-
+        LOGGER.trace("Finished executing {}", name);
         return BstVM.TRUE;
     }
 
@@ -182,51 +190,64 @@ public Integer visitSortCommand(BstParser.SortCommandContext ctx) {
 
     @Override
     public Integer visitIdentifier(BstParser.IdentifierContext ctx) {
-        resolveIdentifier(ctx.IDENTIFIER().getText(), ctx);
+        String name = ctx.IDENTIFIER().getText();
+        LOGGER.trace("Identifier: {}", name);
+        resolveIdentifier(name, ctx);
         return BstVM.TRUE;
     }
 
     protected void resolveIdentifier(String name, ParserRuleContext ctx) {
+        LOGGER.trace("Resolving name {} at resolveIdentifier", name);
+        LOGGER.trace("Stack: {}", bstVMContext.stack());
         if (selectedBstEntry != null) {
+            LOGGER.trace("selectedBstEntry is available");
             if (selectedBstEntry.fields.containsKey(name)) {
-                bstVMContext.stack().push(selectedBstEntry.fields.get(name));
+                String value = selectedBstEntry.fields.get(name);
+                LOGGER.trace("entry field {}={}", name, value);
+                bstVMContext.stack().push(value);
                 return;
             }
             if (selectedBstEntry.localStrings.containsKey(name)) {
-                bstVMContext.stack().push(selectedBstEntry.localStrings.get(name));
+                String value = selectedBstEntry.localStrings.get(name);
+                LOGGER.trace("entry local string {}={}", name, value);
+                bstVMContext.stack().push(value);
                 return;
             }
             if (selectedBstEntry.localIntegers.containsKey(name)) {
-                bstVMContext.stack().push(selectedBstEntry.localIntegers.get(name));
+                Integer value = selectedBstEntry.localIntegers.get(name);
+                LOGGER.trace("entry local integer {}={}", name, value);
+                bstVMContext.stack().push(value);
                 return;
             }
         }
 
         if (bstVMContext.strings().containsKey(name)) {
-            bstVMContext.stack().push(bstVMContext.strings().get(name));
+            String value = bstVMContext.strings().get(name);
+            LOGGER.trace("global string {}={}", name, value);
+            bstVMContext.stack().push(value);
             return;
         }
         if (bstVMContext.integers().containsKey(name)) {
-            bstVMContext.stack().push(bstVMContext.integers().get(name));
+            Integer value = bstVMContext.integers().get(name);
+            LOGGER.trace("global integer {}={}", name, value);
+            bstVMContext.stack().push(value);
             return;
         }
         if (bstVMContext.functions().containsKey(name)) {
-            bstVMContext.functions().get(name).execute(this, ctx);
+            LOGGER.trace("function {}", name);
+            bstVMContext.functions().get(name).execute(this, ctx, selectedBstEntry);
             return;
         }
 
+        LOGGER.warn("No matching identifier found: {}", name);
         throw new BstVMException("No matching identifier found: " + name);
     }
 
     @Override
     public Integer visitBstFunction(BstParser.BstFunctionContext ctx) {
         String name = ctx.getChild(0).getText();
-        if (bstVMContext.functions().containsKey(name)) {
-            bstVMContext.functions().get(name).execute(this, ctx, selectedBstEntry);
-        } else {
-            visit(ctx.getChild(0));
-        }
-
+        LOGGER.trace("Resolving name {} at visitBstFunction", name);
+        resolveIdentifier(name, ctx);
         return BstVM.TRUE;
     }
 

diff --git a/src/main/java/org/jabref/logic/util/TestEntry.java b/src/main/java/org/jabref/logic/util/TestEntry.java
@@ -1,6 +1,6 @@
 package org.jabref.logic.util;
 
-import java.util.Arrays;
+import java.util.List;
 
 import org.jabref.model.entry.BibEntry;
 import org.jabref.model.entry.field.StandardField;
@@ -12,31 +12,32 @@ private TestEntry() {
     }
 
     public static BibEntry getTestEntry() {
-        BibEntry entry = new BibEntry(StandardEntryType.Article);
-        entry.setCitationKey("Smith2016");
-        entry.setField(StandardField.AUTHOR, "Smith, Bill and Jones, Bob and Williams, Jeff");
-        entry.setField(StandardField.EDITOR, "Taylor, Phil");
-        entry.setField(StandardField.TITLE, "Title of the test entry");
-        entry.setField(StandardField.NUMBER, "3");
-        entry.setField(StandardField.VOLUME, "34");
-        entry.setField(StandardField.ISSUE, "7");
-        entry.setField(StandardField.YEAR, "2016");
-        entry.setField(StandardField.PAGES, "45--67");
-        entry.setField(StandardField.MONTH, "July");
-        entry.setField(StandardField.FILE, ":testentry.pdf:PDF");
-        entry.setField(StandardField.JOURNAL, "BibTeX Journal");
-        entry.setField(StandardField.PUBLISHER, "JabRef Publishing");
-        entry.setField(StandardField.ADDRESS, "Trondheim");
-        entry.setField(StandardField.URL, "https://github.com/JabRef");
-        entry.setField(StandardField.DOI, "10.1001/bla.blubb");
-        entry.setField(StandardField.ABSTRACT,
-                "This entry describes a test scenario which may be useful in JabRef. By providing a test entry it is possible to see how certain things will look in this graphical BIB-file mananger.");
-        entry.setField(StandardField.COMMENT, "Lorem ipsum dolor sit amet, consectetur adipisici elit, sed eiusmod tempor incidunt ut labore et " +
-                "dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquid ex ea commodi consequat. " +
-                "Quis aute iure reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint obcaecat cupiditat non " +
-                "proident, sunt in culpa qui officia deserunt mollit anim id est laborum.");
-        entry.putKeywords(Arrays.asList("KeyWord1", "KeyWord2", "KeyWord3", "Keyword4"), ';');
-
+        BibEntry entry = new BibEntry(StandardEntryType.Article)
+                .withCitationKey("Smith2016")
+                .withField(StandardField.AUTHOR, "Smith, Bill and Jones, Bob and Williams, Jeff")
+                .withField(StandardField.EDITOR, "Taylor, Phil")
+                .withField(StandardField.TITLE, "Title of the test entry")
+                .withField(StandardField.NUMBER, "3")
+                .withField(StandardField.VOLUME, "34")
+                .withField(StandardField.ISSUE, "7")
+                .withField(StandardField.YEAR, "2016")
+                .withField(StandardField.PAGES, "45--67")
+                .withField(StandardField.MONTH, "July")
+                .withField(StandardField.FILE, ":testentry.pdf:PDF")
+                .withField(StandardField.JOURNAL, "BibTeX Journal")
+                .withField(StandardField.PUBLISHER, "JabRef Publishing")
+                .withField(StandardField.ADDRESS, "Trondheim")
+                .withField(StandardField.URL, "https://github.com/JabRef")
+                .withField(StandardField.DOI, "10.1001/bla.blubb")
+                .withField(StandardField.ABSTRACT,
+                        "This entry describes a test scenario which may be useful in JabRef. By providing a test entry it is possible to see how certain things will look in this graphical BIB-file mananger.")
+                .withField(StandardField.COMMENT, """
+                        Lorem ipsum dolor sit amet, consectetur adipisici elit, sed eiusmod tempor incidunt ut labore et
+                          dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquid ex ea commodi consequat.
+                          Quis aute iure reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint obcaecat cupiditat non
+                          proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
+                        """);
+        entry.putKeywords(List.of("KeyWord1", "KeyWord2", "KeyWord3", "Keyword4"), ';');
         return entry;
     }
 

diff --git a/src/test/java/org/jabref/logic/bst/BstFunctionsTest.java b/src/test/java/org/jabref/logic/bst/BstFunctionsTest.java
@@ -1,5 +1,6 @@
 package org.jabref.logic.bst;
 
+import java.nio.file.Path;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -16,6 +17,8 @@
 
 import org.antlr.v4.runtime.RecognitionException;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -200,6 +203,28 @@ public void substring() throws RecognitionException {
         assertEquals(0, vm.getStack().size());
     }
 
+    @ParameterizedTest
+    @CsvSource({
+            "d, abcd, -1, 1",
+            "cd, abcd, -1, 2",
+            "bcd, abcd, -1, 3",
+            "c, abcd, -2, 1",
+            "abc, abcd, -2, 100",
+            "abc, abcd, -2, 2147483647",
+            "b, abcd, -3, 1",
+            "a, abcd, -4, 1",
+            "'', abcd, -5, 1" // invalid number -5
+    })
+    void substringPlain(String expected, String full, Integer start, Integer length) {
+        BstVMContext bstVMContext = new BstVMContext(List.of(), new BibDatabase(), Path.of("404.bst"));
+        BstFunctions bstFunctions = new BstFunctions(bstVMContext, new StringBuilder());
+        bstVMContext.stack().push(full);
+        bstVMContext.stack().push(start);
+        bstVMContext.stack().push(length);
+        bstFunctions.bstSubstring(null, null);
+        assertEquals(expected, bstVMContext.stack().pop());
+    }
+
     @Test
     public void empty() throws RecognitionException {
         BstVM vm = new BstVM("""