Fix bugs in slicing with a negative stride

There was a crash when the starting point of a negative slice is larger than
the list length.

In addition, there was an incorrect result when the ending point of a negative
slice is less than the list's length times -1. It would wrongly exclude the
first element.

RELNOTES: Fix slicing bug where "abc"[:-4:-1] would give wrong answer

--
MOS_MIGRATED_REVID=138878716

7 files changed, 260 insertions, 124 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index f9f2f62a88..c83ed98793 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -381,6 +381,130 @@ public final class EvalUtils {
     return -1;
   }
 
+  // The following functions for indexing and slicing match the behavior of Python.
+
+  /**
+   * Resolves a positive or negative index to an index in the range [0, length), or throws
+   * EvalException if it is out-of-range. If the index is negative, it counts backward from
+   * length.
+   */
+  public static int getSequenceIndex(int index, int length, Location loc)
+      throws EvalException {
+    int actualIndex = index;
+    if (actualIndex < 0) {
+      actualIndex += length;
+    }
+    if (actualIndex < 0 || actualIndex >= length) {
+      throw new EvalException(loc, "Index out of range (index is "
+          + index + ", but sequence has " + length + " elements)");
+    }
+    return actualIndex;
+  }
+
+  /**
+   * Performs index resolution after verifying that the given object has index type.
+   */
+  public static int getSequenceIndex(Object index, int length, Location loc)
+      throws EvalException {
+    if (!(index instanceof Integer)) {
+      throw new EvalException(loc, "Indices must be integers, not "
+          + EvalUtils.getDataTypeName(index));
+    }
+    return getSequenceIndex(((Integer) index).intValue(), length, loc);
+  }
+
+  /**
+   * Resolves a positive or negative index to an integer that can denote the left or right boundary
+   * of a slice. If reverse is false, the slice has positive stride (i.e., its elements are in their
+   * normal order) and the result is guaranteed to be in range [0, length + 1). If reverse is true,
+   * the slice has negative stride and the result is in range [-1, length). In either case, if the
+   * index is negative, it counts backward from length. Note that an input index of -1 represents
+   * the last element's position, while an output integer of -1 represents the imaginary position
+   * to the left of the first element.
+   */
+  public static int clampRangeEndpoint(int index, int length, boolean reverse) {
+    if (index < 0) {
+      index += length;
+    }
+    if (!reverse) {
+      return Math.max(Math.min(index, length), 0);
+    } else {
+      return Math.max(Math.min(index, length - 1), -1);
+    }
+  }
+
+  /**
+   * Resolves a positive or negative index to an integer that can denote the boundary for a
+   * slice with positive stride.
+   */
+  public static int clampRangeEndpoint(int index, int length) {
+    return clampRangeEndpoint(index, length, false);
+  }
+
+  /**
+   * Calculates the indices of the elements that should be included in the slice [start:end:step]
+   * of a sequence with the given length. Each of start, end, and step must be supplied, and step
+   * may not be 0.
+   */
+  public static List<Integer> getSliceIndices(int start, int end, int step, int length) {
+    if (step == 0) {
+      throw new IllegalArgumentException("Slice step cannot be zero");
+    }
+    start = clampRangeEndpoint(start, length, step < 0);
+    end = clampRangeEndpoint(end, length, step < 0);
+    ImmutableList.Builder<Integer> indices = ImmutableList.builder();
+    for (int current = start; step > 0 ? current < end : current > end; current += step) {
+      indices.add(current);
+    }
+    return indices.build();
+  }
+
+  /**
+   * Calculates the indices of the elements in a slice, after validating the arguments and replacing
+   * Runtime.NONE with default values. Throws an EvalException if a bad argument is given.
+   */
+  public static List<Integer> getSliceIndices(
+      Object startObj, Object endObj, Object stepObj, int length, Location loc)
+      throws EvalException {
+    int start;
+    int end;
+    int step;
+
+    if (stepObj == Runtime.NONE) {
+      // This case is excluded by the parser, but let's handle it for completeness.
+      step = 1;
+    } else if (stepObj instanceof Integer) {
+      step = ((Integer) stepObj).intValue();
+    } else {
+      throw new EvalException(loc,
+          String.format("Slice step must be an integer, not '%s'", stepObj));
+    }
+    if (step == 0) {
+      throw new EvalException(loc, "Slice step cannot be zero");
+    }
+
+    if (startObj == Runtime.NONE) {
+      start = (step > 0) ? 0 : length - 1;
+    } else if (startObj instanceof Integer) {
+      start = ((Integer) startObj).intValue();
+    } else {
+      throw new EvalException(loc,
+          String.format("Slice start must be an integer, not '%s'", startObj));
+    }
+    if (endObj == Runtime.NONE) {
+      // If step is negative, can't use -1 for end since that would be converted
+      // to the rightmost element's position.
+      end = (step > 0) ? length : -length - 1;
+    } else if (endObj instanceof Integer) {
+      end = ((Integer) endObj).intValue();
+    } else {
+      throw new EvalException(loc,
+          String.format("Slice end must be an integer, not '%s'", endObj));
+    }
+
+    return getSliceIndices(start, end, step, length);
+  }
+
   /** @return true if x is Java null or Skylark None */
   public static boolean isNullOrNone(Object x) {
     return x == null || x == Runtime.NONE;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
index 32114ada7e..575a7d2de1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
@@ -51,7 +51,7 @@ public final class IndexExpression extends Expression {
       return SkylarkType.convertToSkylark(result, env);
     } else if (objValue instanceof String) {
       String string = (String) objValue;
-      int index = MethodLibrary.getListIndex(keyValue, string.length(), loc);
+      int index = EvalUtils.getSequenceIndex(keyValue, string.length(), loc);
       return string.substring(index, index + 1);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
index ee00adbca3..6157e7cafe 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
 import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
 import java.util.ArrayList;
-import java.util.Comparator;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
@@ -52,16 +51,6 @@ public class MethodLibrary {
 
   private MethodLibrary() {}
 
-  // Convert string index in the same way Python does.
-  // If index is negative, starts from the end.
-  // If index is outside bounds, it is restricted to the valid range.
-  public static int clampIndex(int index, int length) {
-    if (index < 0) {
-      index += length;
-    }
-    return Math.max(Math.min(index, length), 0);
-  }
-
   // Emulate Python substring function
   // It converts out of range indices, and never fails
   private static String pythonSubstring(String str, int start, Object end, String msg)
@@ -69,12 +58,12 @@ public class MethodLibrary {
     if (start == 0 && EvalUtils.isNullOrNone(end)) {
       return str;
     }
-    start = clampIndex(start, str.length());
+    start = EvalUtils.clampRangeEndpoint(start, str.length());
     int stop;
     if (EvalUtils.isNullOrNone(end)) {
       stop = str.length();
     } else {
-      stop = clampIndex(Type.INTEGER.convert(end, msg), str.length());
+      stop = EvalUtils.clampRangeEndpoint(Type.INTEGER.convert(end, msg), str.length());
     }
     if (start >= stop) {
       return "";
@@ -82,29 +71,6 @@ public class MethodLibrary {
     return str.substring(start, stop);
   }
 
-  private static int getListIndex(int index, int listSize, Location loc)
-      throws EvalException {
-    // Get the nth element in the list
-    int actualIndex = index;
-    if (actualIndex < 0) {
-      actualIndex += listSize;
-    }
-    if (actualIndex < 0 || actualIndex >= listSize) {
-      throw new EvalException(loc, "List index out of range (index is "
-          + index + ", but list has " + listSize + " elements)");
-    }
-    return actualIndex;
-  }
-
-  public static int getListIndex(Object index, int listSize, Location loc)
-      throws EvalException {
-    if (!(index instanceof Integer)) {
-      throw new EvalException(loc, "List indices must be integers, not "
-          + EvalUtils.getDataTypeName(index));
-    }
-    return getListIndex(((Integer) index).intValue(), listSize, loc);
-  }
-
   // supported string methods
 
   @SkylarkSignature(name = "join", objectType = StringModule.class, returnType = String.class,
@@ -576,7 +542,7 @@ public class MethodLibrary {
       throws ConversionException {
     String substr = pythonSubstring(self, start, end, msg);
     int subpos = forward ? substr.indexOf(sub) : substr.lastIndexOf(sub);
-    start = clampIndex(start, self.length());
+    start = EvalUtils.clampRangeEndpoint(start, self.length());
     return subpos < 0 ? subpos : subpos + start;
   }
 
@@ -994,68 +960,6 @@ public class MethodLibrary {
     }
   };
 
-  /**
-   *  Calculates the indices of the elements that should be included in the slice [start:end:step]
-   * of a sequence with the given length.
-   */
-  public static List<Integer> getSliceIndices(
-      Object startObj, Object endObj, Object stepObj, int length, Location loc
-  ) throws EvalException {
-
-    if (!(stepObj instanceof Integer)) {
-      throw new EvalException(loc, "slice step must be an integer");
-    }
-    int step = ((Integer) stepObj).intValue();
-    if (step == 0) {
-      throw new EvalException(loc, "slice step cannot be zero");
-    }
-    int start = getSliceIndex(startObj,
-        step,
-        /*positiveStepDefault=*/ 0,
-        /*negativeStepDefault=*/ length - 1,
-        /*length=*/ length,
-        loc);
-    int end = getSliceIndex(endObj,
-        step,
-        /*positiveStepDefault=*/ length,
-        /*negativeStepDefault=*/ -1,
-        /*length=*/ length,
-        loc);
-    Comparator<Integer> comparator = getOrderingForStep(step);
-    ImmutableList.Builder<Integer> indices = ImmutableList.builder();
-    for (int current = start; comparator.compare(current, end) < 0; current += step) {
-      indices.add(current);
-    }
-    return indices.build();
-  }
-
-  /**
-   * Converts the given value into an integer index.
-   *
-   * <p>If the value is {@code None}, the return value of this methods depends on the sign of the
-   * slice step.
-   */
-  private static int getSliceIndex(Object value, int step, int positiveStepDefault,
-      int negativeStepDefault, int length, Location loc) throws EvalException {
-    if (value == Runtime.NONE) {
-      return step < 0 ? negativeStepDefault : positiveStepDefault;
-    } else {
-      try {
-        return MethodLibrary.clampIndex(Type.INTEGER.cast(value), length);
-      } catch (ClassCastException ex) {
-        throw new EvalException(loc, String.format("'%s' is not a valid int", value));
-      }
-    }
-  }
-
-  private static Ordering<Integer> getOrderingForStep(int step) {
-    Ordering<Integer> ordering = Ordering.<Integer>natural();
-    if (step < 0) {
-      ordering = ordering.reverse();
-    }
-    return ordering;
-  }
-
   @SkylarkSignature(
     name = "min",
     returnType = Object.class,
@@ -1255,7 +1159,7 @@ public class MethodLibrary {
         public Runtime.NoneType invoke(
             MutableList<Object> self, Integer index, Object item, Location loc, Environment env)
             throws EvalException {
-          self.add(clampIndex(index, self.size()), item, loc, env);
+          self.add(EvalUtils.clampRangeEndpoint(index, self.size()), item, loc, env);
           return Runtime.NONE;
         }
       };
@@ -1363,7 +1267,7 @@ public class MethodLibrary {
         public Object invoke(MutableList<?> self, Object i, Location loc, Environment env)
             throws EvalException {
           int arg = i == Runtime.NONE ? -1 : (Integer) i;
-          int index = getListIndex(arg, self.size(), loc);
+          int index = EvalUtils.getSequenceIndex(arg, self.size(), loc);
           Object result = self.get(index);
           self.remove(index, loc, env);
           return result;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
index 1c1c52b6bb..1221aa67cc 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
@@ -132,7 +132,7 @@ public abstract class SkylarkList<E> extends MutableCollection<E> implements Lis
   @Override
   public final E getIndex(Object key, Location loc) throws EvalException {
     List<E> list = getContentsUnsafe();
-    int index = MethodLibrary.getListIndex(key, list.size(), loc);
+    int index = EvalUtils.getSequenceIndex(key, list.size(), loc);
     return list.get(index);
   }
 
@@ -159,7 +159,7 @@ public abstract class SkylarkList<E> extends MutableCollection<E> implements Lis
     List<E> list = getContentsUnsafe();
     int length = list.size();
     ImmutableList.Builder<E> slice = ImmutableList.builder();
-    for (int pos : MethodLibrary.getSliceIndices(start, end, step, length, loc)) {
+    for (int pos : EvalUtils.getSliceIndices(start, end, step, length, loc)) {
       slice.add(list.get(pos));
     }
     return slice.build();
@@ -176,7 +176,7 @@ public abstract class SkylarkList<E> extends MutableCollection<E> implements Lis
   public void set(Object key, E value, Location loc, Environment env) throws EvalException {
     checkMutable(loc, env);
     List list = getContentsUnsafe();
-    int index = MethodLibrary.getListIndex(key, list.size(), loc);
+    int index = EvalUtils.getSequenceIndex(key, list.size(), loc);
     list.set(index, value);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
index 0647e2942c..f7c36e5a7a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
@@ -74,7 +74,7 @@ public final class SliceExpression extends Expression {
       return SkylarkType.convertToSkylark(slice, env);
     } else if (objValue instanceof String) {
       String string = (String) objValue;
-      List<Integer> indices = MethodLibrary.getSliceIndices(startValue, endValue, stepValue,
+      List<Integer> indices = EvalUtils.getSliceIndices(startValue, endValue, stepValue,
           string.length(), loc);
       char[] result = new char[indices.size()];
       char[] original = ((String) objValue).toCharArray();
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index b70e1f6819..ea81adb35d 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -1028,8 +1028,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
   @Test
   public void testListSlice_WrongType() throws Exception {
     new BothModesTest()
-        .testIfExactError("'a' is not a valid int", "'123'['a'::]")
-        .testIfExactError("'b' is not a valid int", "'123'[:'b':]");
+        .testIfExactError("Slice start must be an integer, not 'a'", "'123'['a'::]")
+        .testIfExactError("Slice end must be an integer, not 'b'", "'123'[:'b':]");
   }
 
   @Test
@@ -1074,7 +1074,7 @@ public class MethodLibraryTest extends EvaluationTestCase {
 
   @Test
   public void testListSliceStep_InvalidStep() throws Exception {
-    String msg = "slice step cannot be zero";
+    String msg = "Slice step cannot be zero";
     new BothModesTest()
         .testIfExactError(msg, "[1, 2, 3][::0]")
         .testIfExactError(msg, "[1, 2, 3][1::0]")
@@ -1098,7 +1098,7 @@ public class MethodLibraryTest extends EvaluationTestCase {
         .testEval("(1, 2, 3, 4, 5)[3:1:-1]", "(4, 3)")
         .testEval("(1, 2, 3, 4, 5)[::-2]", "(5, 3, 1)")
         .testEval("(1, 2, 3, 4, 5)[::-10]", "(5,)")
-        .testIfExactError("slice step cannot be zero", "(1, 2, 3)[1::0]");
+        .testIfExactError("Slice step cannot be zero", "(1, 2, 3)[1::0]");
   }
 
   @Test
@@ -1145,7 +1145,7 @@ public class MethodLibraryTest extends EvaluationTestCase {
   public void testListAccessBadIndex() throws Exception {
     new BothModesTest()
         .testIfErrorContains(
-            "List indices must be integers, not string",
+            "Indices must be integers, not string",
             "[[1], [2]]['a']");
   }
 
@@ -1177,9 +1177,9 @@ public class MethodLibraryTest extends EvaluationTestCase {
   @Test
   public void testStringIndexingOutOfRange() throws Exception {
     new BothModesTest()
-        .testIfErrorContains("List index out of range", "'abcdef'[10]")
-        .testIfErrorContains("List index out of range", "'abcdef'[-11]")
-        .testIfErrorContains("List index out of range", "'abcdef'[42]");
+        .testIfErrorContains("Index out of range", "'abcdef'[10]")
+        .testIfErrorContains("Index out of range", "'abcdef'[-11]")
+        .testIfErrorContains("Index out of range", "'abcdef'[42]");
   }
 
   @Test
@@ -1196,8 +1196,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
   @Test
   public void testStringSlice_WrongType() throws Exception {
     new BothModesTest()
-        .testIfExactError("'a' is not a valid int", "'123'['a'::]")
-        .testIfExactError("'b' is not a valid int", "'123'[:'b':]");
+        .testIfExactError("Slice start must be an integer, not 'a'", "'123'['a'::]")
+        .testIfExactError("Slice end must be an integer, not 'b'", "'123'[:'b':]");
   }
 
   @Test
@@ -1242,7 +1242,7 @@ public class MethodLibraryTest extends EvaluationTestCase {
 
   @Test
   public void testStringSliceStep_InvalidStep() throws Exception {
-    String msg = "slice step cannot be zero";
+    String msg = "Slice step cannot be zero";
     new BothModesTest()
         .testIfExactError(msg, "'123'[::0]")
         .testIfExactError(msg, "'123'[1::0]")
@@ -1484,15 +1484,15 @@ public class MethodLibraryTest extends EvaluationTestCase {
   public void testListIndexOutOfRange() throws Exception {
     new BothModesTest()
         .testIfErrorContains(
-            "List index out of range (index is 3, but list has 3 elements)", "[0, 1, 2][3]")
+            "Index out of range (index is 3, but sequence has 3 elements)", "[0, 1, 2][3]")
         .testIfErrorContains(
-            "List index out of range (index is -4, but list has 3 elements)", "[0, 1, 2][-4]")
+            "Index out of range (index is -4, but sequence has 3 elements)", "[0, 1, 2][-4]")
         .testIfErrorContains(
-            "List index out of range (index is -2, but list has 1 elements)", "[0][-2]")
+            "Index out of range (index is -2, but sequence has 1 elements)", "[0][-2]")
         .testIfErrorContains(
-            "List index out of range (index is 1, but list has 1 elements)", "[0][1]")
+            "Index out of range (index is 1, but sequence has 1 elements)", "[0][1]")
         .testIfErrorContains(
-            "List index out of range (index is 1, but list has 0 elements)", "[][1]");
+            "Index out of range (index is 1, but sequence has 0 elements)", "[][1]");
   }
 
   @Test
@@ -1611,7 +1611,7 @@ public class MethodLibraryTest extends EvaluationTestCase {
         .testLookup("ret", 3);
     new BothModesTest()
         .testIfErrorContains(
-            "List index out of range (index is 3, but list has 2 elements)", "[1, 2].pop(3)");
+            "Index out of range (index is 3, but sequence has 2 elements)", "[1, 2].pop(3)");
 
     new BothModesTest().testIfErrorContains("Type tuple has no function pop()", "(1, 2).pop()");
   }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
index 00a4d14d3e..d6fa9fdab8 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
@@ -29,11 +29,119 @@ import org.junit.runners.JUnit4;
 public class SkylarkListTest extends EvaluationTestCase {
 
   @Test
-  public void testListIndex() throws Exception {
+  public void testIndex() throws Exception {
     eval("l = [1, '2', 3]");
     assertThat(eval("l[0]")).isEqualTo(1);
     assertThat(eval("l[1]")).isEqualTo("2");
     assertThat(eval("l[2]")).isEqualTo(3);
+
+    eval("t = (1, '2', 3)");
+    assertThat(eval("t[0]")).isEqualTo(1);
+    assertThat(eval("t[1]")).isEqualTo("2");
+    assertThat(eval("t[2]")).isEqualTo(3);
+  }
+
+  @Test
+  public void testIndexOutOfBounds() throws Exception {
+    checkEvalError("Index out of range (index is 3, but sequence has 3 elements)",
+        "['a', 'b', 'c'][3]");
+    checkEvalError("Index out of range (index is 10, but sequence has 3 elements)",
+        "['a', 'b', 'c'][10]");
+    checkEvalError("Index out of range (index is 0, but sequence has 0 elements)",
+        "[][0]");
+  }
+
+  @Test
+  public void testNegativeIndices() throws Exception {
+    eval("l = ['a', 'b', 'c']");
+    assertThat(eval("l[0]")).isEqualTo("a");
+    assertThat(eval("l[-1]")).isEqualTo("c");
+    assertThat(eval("l[-2]")).isEqualTo("b");
+    assertThat(eval("l[-3]")).isEqualTo("a");
+    checkEvalError("Index out of range (index is -4, but sequence has 3 elements)",
+        "l[-4]");
+    checkEvalError("Index out of range (index is -1, but sequence has 0 elements)",
+        "[][-1]");
+  }
+
+  @SuppressWarnings("unchecked")
+  private SkylarkList<Object> listEval(String... input) throws Exception {
+    return (SkylarkList<Object>) eval(input);
+  }
+
+  @Test
+  public void testSlice() throws Exception {
+    eval("l = ['a', 'b', 'c']");
+    assertThat(listEval("l[0:3]")).containsExactly("a", "b", "c").inOrder();
+    assertThat(listEval("l[0:2]")).containsExactly("a", "b").inOrder();
+    assertThat(listEval("l[0:1]")).containsExactly("a").inOrder();
+    assertThat(listEval("l[0:0]")).isEmpty();
+    assertThat(listEval("l[1:3]")).containsExactly("b", "c").inOrder();
+    assertThat(listEval("l[2:3]")).containsExactly("c").inOrder();
+    assertThat(listEval("l[3:3]")).isEmpty();
+    assertThat(listEval("l[2:1]")).isEmpty();
+    assertThat(listEval("l[3:0]")).isEmpty();
+
+    eval("t = ('a', 'b', 'c')");
+    assertThat(listEval("t[0:3]")).containsExactly("a", "b", "c").inOrder();
+    assertThat(listEval("t[1:2]")).containsExactly("b").inOrder();
+  }
+
+  @Test
+  public void testSliceDefault() throws Exception {
+    eval("l = ['a', 'b', 'c']");
+    assertThat(listEval("l[:]")).containsExactly("a", "b", "c").inOrder();
+    assertThat(listEval("l[:2]")).containsExactly("a", "b").inOrder();
+    assertThat(listEval("l[2:]")).containsExactly("c").inOrder();
+  }
+
+  @Test
+  public void testSliceNegative() throws Exception {
+    eval("l = ['a', 'b', 'c']");
+    assertThat(listEval("l[-2:-1]")).containsExactly("b").inOrder();
+    assertThat(listEval("l[-2:]")).containsExactly("b", "c").inOrder();
+    assertThat(listEval("l[0:-1]")).containsExactly("a", "b").inOrder();
+    assertThat(listEval("l[-1:1]")).isEmpty();
+  }
+
+  @Test
+  public void testSliceBounds() throws Exception {
+    eval("l = ['a', 'b', 'c']");
+    assertThat(listEval("l[0:5]")).containsExactly("a", "b", "c").inOrder();
+    assertThat(listEval("l[-10:2]")).containsExactly("a", "b").inOrder();
+    assertThat(listEval("l[3:10]")).isEmpty();
+    assertThat(listEval("l[-10:-9]")).isEmpty();
+  }
+
+  @Test
+  public void testSliceSkip() throws Exception {
+    eval("l = ['a', 'b', 'c', 'd', 'e', 'f', 'g']");
+    assertThat(listEval("l[0:6:2]")).containsExactly("a", "c", "e").inOrder();
+    assertThat(listEval("l[0:7:2]")).containsExactly("a", "c", "e", "g").inOrder();
+    assertThat(listEval("l[0:10:2]")).containsExactly("a", "c", "e", "g").inOrder();
+    assertThat(listEval("l[-6:10:2]")).containsExactly("b", "d", "f").inOrder();
+    assertThat(listEval("l[1:5:3]")).containsExactly("b", "e").inOrder();
+    assertThat(listEval("l[-10:3:2]")).containsExactly("a", "c").inOrder();
+    assertThat(listEval("l[-10:10:1]")).containsExactly(
+        "a", "b", "c", "d", "e", "f", "g").inOrder();
+  }
+
+  @Test
+  public void testSliceNegativeSkip() throws Exception {
+    eval("l = ['a', 'b', 'c', 'd', 'e', 'f', 'g']");
+    assertThat(listEval("l[5:2:-1]")).containsExactly("f", "e", "d").inOrder();
+    assertThat(listEval("l[5:2:-2]")).containsExactly("f", "d").inOrder();
+    assertThat(listEval("l[5:3:-2]")).containsExactly("f").inOrder();
+    assertThat(listEval("l[6::-4]")).containsExactly("g", "c").inOrder();
+    assertThat(listEval("l[7::-4]")).containsExactly("g", "c").inOrder();
+    assertThat(listEval("l[-1::-4]")).containsExactly("g", "c").inOrder();
+    assertThat(listEval("l[-1:-10:-4]")).containsExactly("g", "c").inOrder();
+    assertThat(listEval("l[-1:-3:-4]")).containsExactly("g").inOrder();
+    assertThat(listEval("l[2:5:-1]")).isEmpty();
+    assertThat(listEval("l[-10:5:-1]")).isEmpty();
+    assertThat(listEval("l[1:-8:-1]")).containsExactly("b", "a").inOrder();
+
+    checkEvalError("Slice step cannot be zero", "l[2:5:0]");
   }
 
   @Test