Skip to content

Conversation

@rafaelha
Copy link
Contributor

@rafaelha rafaelha commented Oct 31, 2025

In three places across the kirin code base a slice object would be converted into start,stop,step triples using slice.indices, and then converted back into a slice.

There is a subtle issue with this illustrated by the following snippet:

nums = [0, 1, 2, 3, 4]
sl = slice(None, None, -1)
start, stop, step = sl.indices(len(nums))

print(nums[sl]) # [4, 3, 2, 1, 0]
print(nums[start:stop:step]) # []

Specifically, the following is an example of a incorrect rewrite that happened as a consequence

    @basic_no_opt
    def func():
        ylist = (0, 1, 2, 3, 4)
        return ylist[::-1]

    before = func()
    constprop = const.Propagate(func.dialects)
    frame, _ = constprop.run(func)
    Fixpoint(Walk(WrapConst(frame))).rewrite(func.code)
    inline_getitem = InlineGetItem()
    Fixpoint(Walk(Chain([inline_getitem, DeadCodeElimination()]))).rewrite(func.code)

    after = func()

    assert before == after
    # Failes because (4, 3, 2, 1, 0) != (,)

This PR removes all usage of slice.indices and introduces some new unit tests.

This is coming from unexpected behavior of `Slice.indices`. The solution is to simply avoid using `.indices`. A MRE is

nums = [0, 1, 2, 3, 4]
sl = slice(None, None, -1)
start, stop, step = sl.indices(len(nums))

print(nums[sl]) # [4, 3, 2, 1, 0]
print(nums[start:stop:step]) # []
@rafaelha rafaelha requested a review from weinbe58 October 31, 2025 18:44
@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
11463 10208 89% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/kirin/dialects/ilist/rewrite/inline_getitem.py 95% 🟢
src/kirin/dialects/py/indexing.py 90% 🟢
src/kirin/rewrite/getitem.py 96% 🟢
TOTAL 94% 🟢

updated for commit: f61f805 by action🐍

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

PR Preview Action v1.6.2
Preview removed because the pull request was closed.
2025-11-03 14:22 UTC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is almost the same as getitem.py (list vs ilist)

Can code duplication be avoided?

@rafaelha rafaelha changed the title Fix slicing reverse edge case Fix reverse slicing edge case Oct 31, 2025
Copy link
Member

@weinbe58 weinbe58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@weinbe58 weinbe58 merged commit f904618 into main Nov 3, 2025
13 checks passed
@weinbe58 weinbe58 deleted the rafaelha/fix-slicing-reverse-edge-case branch November 3, 2025 14:22
@rafaelha rafaelha self-assigned this Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants