Fix hover hint for complex expressions

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/util/locations/impl/TreeMapLookup.java

@@ -34,85 +34,53 @@
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.eclipse.lsp4j.Position;
 import org.eclipse.lsp4j.Range;
+import org.eclipse.lsp4j.util.Ranges;
 import org.rascalmpl.vscode.lsp.util.locations.IRangeMap;
 
 public class TreeMapLookup<T> implements IRangeMap<T> {
 
     private final NavigableMap<Range, T> data = new TreeMap<>(TreeMapLookup::compareRanges);
 
     private static int compareRanges(Range a, Range b) {
-        Position aStart = a.getStart();
-        Position aEnd = a.getEnd();
-        Position bStart = b.getStart();
-        Position bEnd = b.getEnd();
-        if (aEnd.getLine() < bStart.getLine()) {
-            return -1;
+        if (a.equals(b)) {
+            return 0;
         }
-        if (aStart.getLine() > bEnd.getLine()) {
+        // check containment; strict since `a != b`
+        // parent is always larger than child
+        if (Ranges.containsRange(a, b)) {
             return 1;
         }
-        // some kind of containment, or just on the same line
-        if (aStart.getLine() == bStart.getLine()) {
-            // start at same line
-            if (aEnd.getLine() == bEnd.getLine()) {
-                // end at same line
-                if (aStart.getCharacter() == bStart.getCharacter()) {
-                    return Integer.compare(aEnd.getCharacter(), bEnd.getCharacter());
-                }
-                return Integer.compare(aStart.getCharacter(), bStart.getCharacter());
-            }
-            return Integer.compare(aEnd.getLine(), bEnd.getLine());
+        if (Ranges.containsRange(b, a)) {
+            return -1;
         }
-        return Integer.compare(aStart.getLine(), bStart.getLine());
-    }
 
-    private static boolean rangeContains(Range a, Range b) {
         Position aStart = a.getStart();
         Position aEnd = a.getEnd();
         Position bStart = b.getStart();
         Position bEnd = b.getEnd();
 
-        if (aStart.getLine() <= bStart.getLine()
-            && aEnd.getLine() >= bEnd.getLine()) {
-            if (aStart.getLine() == bStart.getLine()) {
-                if (aStart.getCharacter() > bStart.getCharacter()) {
-                    return false;
-                }
-            }
-            if (aEnd.getLine() == bEnd.getLine()) {
-                if (aEnd.getCharacter() < bEnd.getCharacter()) {
-                    return false;
-                }
-            }
-            return true;
+        if (aStart.getLine() != bStart.getLine()) {
+            return Integer.compare(bStart.getLine(), aStart.getLine());
         }
-        return false;
-    }
-
-    private @Nullable T contains(@Nullable Entry<Range, T> entry, Range from) {
-        if (entry != null) {
-            Range match = entry.getKey();
-            if (rangeContains(match, from)) {
-                return entry.getValue();
-            }
+        if (aEnd.getLine() != bEnd.getLine()) {
+            return Integer.compare(bEnd.getLine(), aEnd.getLine());
         }
-        return null;
+        // start characters cannot be equal since start/end lines are equal and neither range contains the other
+        return Integer.compare(bEnd.getCharacter(), aEnd.getCharacter());
     }
 
     @Override
     public @Nullable T lookup(Range from) {
         // since we allow for overlapping ranges, it might be that we have to
         // search all the way to the "bottom" of the tree to see if we are
         // contained in something larger than the closest key
-        var previousKeys = data.headMap(from, true).descendingMap();
-        for (var candidate : previousKeys.entrySet()) {
-            T result = contains(candidate, from);
-            if (result != null) {
-                return result;
-            }
-        }
-        // could be that it's at the start of the entry (so the entry it not in the head map)
-        return contains(data.ceilingEntry(from), from);
+        // if we could come up with a *valid* ordering such that `data.floorKey(from)` is always
+        // the smallest key containing `from` (or another key when none contain `from`), we could use `data.floorEntry` here instead of iterating
+        return data.tailMap(from, true).entrySet()
+            .stream()
+            .filter(e -> Ranges.containsRange(e.getKey(), from))
+            .map(Entry::getValue)
+            .findFirst().orElse(null);
     }
 
     @Override

rascal-lsp/src/test/java/engineering/swat/rascal/lsp/util/LookupTests.java

@@ -31,6 +31,8 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Random;
+import java.util.stream.Collectors;
+
 import org.eclipse.lsp4j.Position;
 import org.eclipse.lsp4j.Range;
 import org.junit.Test;
@@ -89,6 +91,7 @@ public void testSimpleLookupEnd() {
     public void testSimpleLookupInside1() {
         TreeMapLookup<String> target = buildTreeLookup2();
         assertSame("hit", target.lookup(range(1,6, 1, 7)));
+        assertSame("hit", target.lookup(range(1,5, 1, 8)));
     }
 
     @Test
@@ -137,12 +140,43 @@ public void testOverlappingRanges() {
         assertSame("big1", target.lookup(cursor(4,5)));
     }
 
+    @Test
+    public void testSingleLineTree() {
+        TreeMapLookup<String> target = new TreeMapLookup<>();
+        target.put(range(0, 1, 0, 10), "composite expression");
+        target.put(range(0, 1, 0, 4), "first half");
+        target.put(range(0, 1, 0, 2), "first quarter");
+        target.put(range(0, 2, 0, 4), "second quarter");
+        target.put(range(0, 6, 0, 10), "second half");
+        target.put(range(0, 6, 0, 8), "third quarter");
+        target.put(range(0, 8, 0, 10), "fourth quarter");
+        assertSame("composite expression", target.lookup(cursor(0, 5)));
+        assertSame("first quarter", target.lookup(cursor(0, 1)));
+        assertSame("second quarter", target.lookup(cursor(0, 4)));
+        assertSame("third quarter", target.lookup(cursor(0, 6)));
+        assertSame("fourth quarter", target.lookup(cursor(0, 10)));
+    }
+
+    @Test
+    public void testMultiLineTree() {
+        TreeMapLookup<String> target = new TreeMapLookup<>();
+        target.put(range(0, 1, 2, 10), "composite expression");
+        target.put(range(0, 1, 1, 4), "first half");
+        target.put(range(1, 6, 2, 10), "second half");
+        assertSame("composite expression", target.lookup(cursor(1, 5)));
+        assertSame("first half", target.lookup(cursor(0, 1)));
+        assertSame("first half", target.lookup(cursor(1, 4)));
+        assertSame("second half", target.lookup(cursor(1, 6)));
+        assertSame("second half", target.lookup(cursor(2, 10)));
+    }
+
     @Test
     public void randomRanges() {
         TreeMapLookup<String> target = new TreeMapLookup<>();
         Map<Range, String> ranges = new HashMap<>();
         Random r = new Random();
-        for (int i = 0; i < 1000; i++) {
+        int tries = 1000;
+        for (int i = 0; i < tries; i++) {
             int startLine = r.nextInt(10);
             int startColumn = r.nextInt(10);
             int endLine = startLine + r.nextInt(3);
@@ -156,9 +190,11 @@ public void randomRanges() {
             ranges.put(range(startLine, startColumn, endLine, endColumn), "" + i);
         }
         ranges.forEach(target::put);
-        for (var e: ranges.entrySet()) {
+        int i = 0;
+        for (var e : ranges.entrySet()) {
             var found = target.lookup(e.getKey());
-            assertSame("Entry " + e + "should be found", e.getValue(), found);
+            var foundKeys = ranges.entrySet().stream().filter(range -> range.getValue().equals(found)).collect(Collectors.toSet());
+            assertSame("Entry " + e + " should be found, but found " + foundKeys +" (attempt " + i++ + "/" + tries + ")", e.getValue(), found);
         }
     }