Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Negative Step in listSlice (GOOL) #3809

Closed
B-rando1 opened this issue Jun 18, 2024 · 7 comments · Fixed by #3826
Closed

Negative Step in listSlice (GOOL) #3809

B-rando1 opened this issue Jun 18, 2024 · 7 comments · Fixed by #3826
Assignees

Comments

@B-rando1
Copy link
Collaborator

I noticed that in the listSlice function that GOOL uses, a negative step value results in different behaviour between different targets. For example I'll be looking at the following pseudocode:

l = [1, 2, 3, 4]
s = l.slice(start=2, end=0, step=-1)
print(s)

In Python this translates to the nicest (in my opinion) functionality: it prints [3, 2] - that is, it created a slice from index 2 to index 1, reversing the list because of the negative step.

In other languages, this prints [], either because those languages don't support negative steps or because we aren't taking advantage of them.

It also gets even worse when we leave out the start or end values. GOOL allows us to leave either of those out, which has the effect of automatically starting at the start or ending at the end. In Python when these are combined with a negative step, it simply reverses the list. In other languages though, it throws out-of-bounds errors.

It would probably be wise to create some consistency between targets. Our two options are to either assume that the step will never be negative (and ideally enforce this constraint), or bring the other languages up to speed with Python. The second option is probably better, but there are some tricky edge cases we would need to think about.

For example, what does the pseudocode

s = l.slice(, b, c)

translate to? Depending on whether the runtime value of c is positive or negative, we have to start either at 0 or at b, and have to keep going either until the index is greater than b or less than 0. It's not impossible to get this right, but it would result in some messy code.

What are your thoughts? At this point I'm wondering if it might be better to assume that the step will always be positive, and add a listReverse function later if we find we need a way of reversing a list.

@JacquesCarette
Copy link
Owner

The semantics of negative list slice is supposed to be Python's. If it doesn't work "directly" in other languages, then proper code should be generated instead.

We definitely want to allow to state, at a higher level, what we want, and have the low-level generator 'do the right thing'. GOOL is not supposed to represent code but rather code-like intent.

@B-rando1
Copy link
Collaborator Author

I did some work on a fix for this yesterday, focusing on Swift. Swift mostly worked how it was supposed to, except that if beg or end wasn't given, they would default to the start or end of the list respectively, instead of checking the sign of step. I added some logic to check step and assign a value to each of beg or end at runtime if it isn't given. This fixes the issue, and has the same behaviour as Python for all 10 of the test cases I created.

The one thing I don't like is that some of these checks don't look smart. For example, code generated for one test:

var endIdx0: Int
if -1 > 0 {
    endIdx0 = myOtherList.count
}
else {
    endIdx0 = -1
}
mySlicedList7 = [Int](stride(from: 3, to: endIdx0, by: -1)).map({(i: Int) -> Double in myOtherList[i]})

It seems like there should be a way within GOOL to tell that -1 is a litInt, and do the logic of finding endIdx within GOOL rather than generating code to do it at runtime. I don't currently know of a way to get the value held by an SValue, though. Would either of you know, @JacquesCarette or @bmaclach? Thanks!

@JacquesCarette
Copy link
Owner

I forget the exact details, but we definitely changed some of the data-structures to have some 'static' information be available. SValue, if I remember well, is too opaque, and you won't be able to dig it out (as it could be an expression).

You are right that this logic should be possible to do within GOOL. You should look at other places where logic is done in GOOL and see where the information it uses is 'tucked away', and you'll need to mimick that.

@bmaclach
Copy link
Collaborator

I think I can fill in some details here. This issue sounds like it's in the same vein as an issue I worked on where we wanted to ensure we generated only the necessary parentheses when rendering expressions. In that case, GOOL had to know when a value was an expression, and if so, it needed to know the precedence of the operation in the expression. I ended up storing that information in the valPrec field of ValData. So I think looking at and understanding how GOOL uses valPrec will demonstrate how a problem of this type can be solved.

@smiths
Copy link
Collaborator

smiths commented Jun 23, 2024

Thank you @bmaclach.

@balacij
Copy link
Collaborator

balacij commented Jun 27, 2024

Since question is now an actionable ticket, can you please update the OP with tasks that need to be done to close it, @B-rando1 ?

@B-rando1
Copy link
Collaborator Author

What we need to do in order to close this ticket:

  • Approve and merge the PR for Swift (Fixed List Slicing in Swift #3820)
  • Abstract common functionality (i.e. makeSetterVal) into CommonPseudoOO.hs
  • Update the listSlice in Macros.hs to match. As all three other problematic targets use this function, we should be done after this. The main changes will be the same, and will benefit from the previous point. The new change is that we have to change the bounds check in the for-loop.
    • If step has a literal value, we can generate a one-sided check (i.e. i < endVal or i > endVal); otherwise we can generate a two-sided check (endVal< i < endVal or something like that).

This is going well so far, and has been a fun challenge! I think once the Swift PR is merged, the other changes should be quick.

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 a pull request may close this issue.

5 participants