Merge pull request RustPython#766 from jgirardet/seq

Fix RustPython#746: invalid slice with start or stop =-1 when step<0

Author: coolreader18

.gitignore

@@ -9,3 +9,4 @@ __pycache__
 .vscode
 wasm-pack.log
 .idea/
+tests/snippets/resources

tests/snippets/builtin_slice.py

@@ -2,24 +2,24 @@
 
 a = []
 assert a[:] == []
-assert a[:2**100] == []
-assert a[-2**100:] == []
-assert a[::2**100] == []
+assert a[: 2 ** 100] == []
+assert a[-2 ** 100 :] == []
+assert a[:: 2 ** 100] == []
 assert a[10:20] == []
 assert a[-20:-10] == []
 
 b = [1, 2]
 
 assert b[:] == [1, 2]
-assert b[:2**100] == [1, 2]
-assert b[-2**100:] == [1, 2]
-assert b[2**100:] == []
-assert b[::2**100] == [1]
+assert b[: 2 ** 100] == [1, 2]
+assert b[-2 ** 100 :] == [1, 2]
+assert b[2 ** 100 :] == []
+assert b[:: 2 ** 100] == [1]
 assert b[-10:1] == [1]
 assert b[0:0] == []
 assert b[1:0] == []
 
-assert_raises(ValueError, lambda: b[::0], 'zero step slice')
+assert_raises(ValueError, lambda: b[::0], "zero step slice")
 
 assert b[::-1] == [2, 1]
 assert b[1::-1] == [2, 1]
@@ -33,7 +33,7 @@
 assert c[9:6:-3] == [9]
 assert c[9::-3] == [9, 6, 3, 0]
 assert c[9::-4] == [9, 5, 1]
-assert c[8::-2**100] == [8]
+assert c[8 :: -2 ** 100] == [8]
 
 assert c[7:7:-2] == []
 assert c[7:8:-2] == []
@@ -43,6 +43,7 @@
 assert d[3::-1] == "4321"
 assert d[4::-3] == "52"
 
+assert [1, 2, 3, 5, 6][-1:-5:-1] == [6, 5, 3, 2]  # #746
 
 slice_a = slice(5)
 assert slice_a.start is None
@@ -71,3 +72,41 @@ def __setitem__(self, key, value):
 ss = SubScript()
 _ = ss[:]
 ss[:1] = 1
+
+
+def test_all_slices():
+    """
+    test all possible slices except big number
+    """
+
+    mod = __import__('cpython_generated_slices')
+
+    ll = mod.LL
+    start = mod.START
+    end = mod.END
+    step = mod.STEP
+    slices_res = mod.SLICES_RES
+
+    count = 0
+    failures = []
+    for s in start:
+        for e in end:
+            for t in step:
+                lhs = ll[s:e:t]
+                try:
+                    assert lhs == slices_res[count]
+                except AssertionError:
+                    failures.append(
+                        "start: {} ,stop: {}, step {}. Expected: {}, found: {}".format(
+                            s, e, t, lhs, slices_res[count]
+                        )
+                    )
+                count += 1
+
+    if failures:
+        for f in failures:
+            print(f)
+        print(len(failures), "slices failed")
+
+
+test_all_slices()

tests/test_snippets.py

@@ -1,4 +1,3 @@
-
 # This is a python unittest class automatically populating with all tests
 # in the tests folder.
 
@@ -11,6 +10,8 @@
 import subprocess
 import contextlib
 import enum
+from pathlib import Path
+import shutil
 
 import compile_code
 
@@ -19,13 +20,11 @@ class _TestType(enum.Enum):
     functional = 1
 
 
-logger = logging.getLogger('tests')
-ROOT_DIR = '..'
-TEST_ROOT = os.path.abspath(os.path.join(ROOT_DIR, 'tests'))
-TEST_DIRS = {
-    _TestType.functional: os.path.join(TEST_ROOT, 'snippets'),
-}
-CPYTHON_RUNNER_DIR = os.path.abspath(os.path.join(ROOT_DIR, 'py_code_object'))
+logger = logging.getLogger("tests")
+ROOT_DIR = ".."
+TEST_ROOT = os.path.abspath(os.path.join(ROOT_DIR, "tests"))
+TEST_DIRS = {_TestType.functional: os.path.join(TEST_ROOT, "snippets")}
+CPYTHON_RUNNER_DIR = os.path.abspath(os.path.join(ROOT_DIR, "py_code_object"))
 RUSTPYTHON_RUNNER_DIR = os.path.abspath(os.path.join(ROOT_DIR))
 RUSTPYTHON_LIB_DIR = os.path.abspath(os.path.join(ROOT_DIR, "Lib"))
 
@@ -38,12 +37,12 @@ def pushd(path):
 
 
 def perform_test(filename, method, test_type):
-    logger.info('Running %s via %s', filename, method)
-    if method == 'cpython':
+    logger.info("Running %s via %s", filename, method)
+    if method == "cpython":
         run_via_cpython(filename)
-    elif method == 'cpython_bytecode':
+    elif method == "cpython_bytecode":
         run_via_cpython_bytecode(filename, test_type)
-    elif method == 'rustpython':
+    elif method == "rustpython":
         run_via_rustpython(filename, test_type)
     else:
         raise NotImplementedError(method)
@@ -57,16 +56,16 @@ def run_via_cpython(filename):
 
 def run_via_cpython_bytecode(filename, test_type):
     # Step1: Create bytecode file:
-    bytecode_filename = filename + '.bytecode'
-    with open(bytecode_filename, 'w') as f:
+    bytecode_filename = filename + ".bytecode"
+    with open(bytecode_filename, "w") as f:
         compile_code.compile_to_bytecode(filename, out_file=f)
 
     # Step2: run cpython bytecode:
     env = os.environ.copy()
-    env['RUST_LOG'] = 'info,cargo=error,jobserver=error'
-    env['RUST_BACKTRACE'] = '1'
+    env["RUST_LOG"] = "info,cargo=error,jobserver=error"
+    env["RUST_BACKTRACE"] = "1"
     with pushd(CPYTHON_RUNNER_DIR):
-        subprocess.check_call(['cargo', 'run', bytecode_filename], env=env)
+        subprocess.check_call(["cargo", "run", bytecode_filename], env=env)
 
 
 def run_via_rustpython(filename, test_type):
@@ -75,26 +74,26 @@ def run_via_rustpython(filename, test_type):
     env['RUST_BACKTRACE'] = '1'
     env['PYTHONPATH'] = RUSTPYTHON_LIB_DIR
 
-    target = 'release'
-    if env.get('CODE_COVERAGE', 'false') == 'true':
-        target = 'debug'
-    binary = os.path.abspath(os.path.join(ROOT_DIR, 'target', target, 'rustpython'))
+    target = "release"
+    if env.get("CODE_COVERAGE", "false") == "true":
+        target = "debug"
+    binary = os.path.abspath(os.path.join(ROOT_DIR, "target", target, "rustpython"))
 
     subprocess.check_call([binary, filename], env=env)
 
 
 def create_test_function(cls, filename, method, test_type):
     """ Create a test function for a single snippet """
     core_test_directory, snippet_filename = os.path.split(filename)
-    test_function_name = 'test_{}_'.format(method) \
-        + os.path.splitext(snippet_filename)[0] \
-        .replace('.', '_').replace('-', '_')
+    test_function_name = "test_{}_".format(method) + os.path.splitext(snippet_filename)[
+        0
+    ].replace(".", "_").replace("-", "_")
 
     def test_function(self):
         perform_test(filename, method, test_type)
 
     if hasattr(cls, test_function_name):
-        raise ValueError('Duplicate test case {}'.format(test_function_name))
+        raise ValueError("Duplicate test case {}".format(test_function_name))
     setattr(cls, test_function_name, test_function)
 
 
@@ -104,25 +103,61 @@ def wrapper(cls):
         for test_type, filename in get_test_files():
             create_test_function(cls, filename, method, test_type)
         return cls
+
     return wrapper
 
 
 def get_test_files():
     """ Retrieve test files """
     for test_type, test_dir in TEST_DIRS.items():
-        for filepath in sorted(glob.iglob(os.path.join(test_dir, '*.py'))):
+        for filepath in sorted(glob.iglob(os.path.join(test_dir, "*.py"))):
             filename = os.path.split(filepath)[1]
-            if filename.startswith('xfail_'):
+            if filename.startswith("xfail_"):
                 continue
 
             yield test_type, os.path.abspath(filepath)
 
 
-@populate('cpython')
+def generate_slices(path):
+    # loop used to build slices_res.py with cpython
+    ll = [0, 1, 2, 3]
+    start = list(range(-7, 7))
+    end = list(range(-7, 7))
+    step = list(range(-5, 5))
+    step.pop(step.index(0))
+    for i in [start, end, step]:
+        i.append(None)
+
+    slices_res = []
+    for s in start:
+        for e in end:
+            for t in step:
+                slices_res.append(ll[s:e:t])
+
+    path.write_text(
+        "SLICES_RES={}\nSTART= {}\nEND= {}\nSTEP= {}\nLL={}\n".format(
+            slices_res, start, end, step, ll
+        )
+    )
+
+
+@populate("cpython")
 # @populate('cpython_bytecode')
-@populate('rustpython')
+@populate("rustpython")
 class SampleTestCase(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
-        subprocess.check_call(['cargo', 'build'])
-        subprocess.check_call(['cargo', 'build', '--release'])
+        # Here add resource files
+        cls.slices_resource_path = Path(TEST_DIRS[_TestType.functional]) / "cpython_generated_slices.py"
+        if cls.slices_resource_path.exists():
+            cls.slices_resource_path.unlink()
+
+        generate_slices(cls.slices_resource_path)
+
+        # cargo stuff
+        subprocess.check_call(["cargo", "build"])
+        subprocess.check_call(["cargo", "build", "--release"])
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.slices_resource_path.unlink()

vm/src/obj/objsequence.rs

@@ -2,11 +2,11 @@ use std::cell::RefCell;
 use std::marker::Sized;
 use std::ops::{Deref, DerefMut, Range};
 
-use num_bigint::BigInt;
-use num_traits::{One, Signed, ToPrimitive, Zero};
-
 use crate::pyobject::{IdProtocol, PyObject, PyObjectRef, PyResult, TryFromObject, TypeProtocol};
+
 use crate::vm::VirtualMachine;
+use num_bigint::{BigInt, ToBigInt};
+use num_traits::{One, Signed, ToPrimitive, Zero};
 
 use super::objbool;
 use super::objint::PyInt;
@@ -86,8 +86,20 @@ pub trait PySliceableSequence {
                 } else {
                     // calculate the range for the reverse slice, first the bounds needs to be made
                     // exclusive around stop, the lower number
-                    let start = start.as_ref().map(|x| x + 1);
-                    let stop = stop.as_ref().map(|x| x + 1);
+                    let start = start.as_ref().map(|x| {
+                        if *x == (-1).to_bigint().unwrap() {
+                            self.len() + BigInt::one() //.to_bigint().unwrap()
+                        } else {
+                            x + 1
+                        }
+                    });
+                    let stop = stop.as_ref().map(|x| {
+                        if *x == (-1).to_bigint().unwrap() {
+                            self.len().to_bigint().unwrap()
+                        } else {
+                            x + 1
+                        }
+                    });
                     let range = self.get_slice_range(&stop, &start);
                     if range.start < range.end {
                         match (-step).to_i32() {