Consider optional keys in ConstantArrayType::slice

[herndlm]

This was hard. I think the tests show that it works correct, but tbh., it's a challenge for me to get the more complicated optional examples right in my head :D

There're a couple of more interesting "normal" constant array array_slice tests existing already in LegacyNodeScopeResolverTest.

[herndlm]

still not sure, about some tests, I'll check this again tomorrow. The behaviour is not correct yet

[ondrejmirtes]

Oh yes, it's hard :)

[herndlm]

I think I made it work. Also added all edge cases I could think of. It's unfortunately far from as elegant as I was hoping, mostly because of negative offsets and lengths/limits.
Don't want to see this anymore for a bit xD, but maybe I'll come back later and think through a couple of the test cases again to ensure I didn't miss anything obvious.

[herndlm]

Unfortunately I did find one more thing. I'll adapt it in the evening.

[herndlm]

I have a fix locally again, but I was wondering @ondrejmirtes: I saw that many optional key cases seem to be handled via ConstantArrayType::getAllArrays which creates a power set. Then often, the arrays are modified in a loop, to be then unified again. My question: Should this always be preferred? E.g. I saw that array_pop uses this as well, combined with ConstantArrayType::removeLast, but I could simply adapt removeLast to handle the optional keys which safes us all the hassle of creating and unifying many arrays. Would that make sense from a performance point of view? If so, can I simply adapt removeLast (in yet another PR of course) or is BC a problem here?

[ondrejmirtes]

I've stopped using ConstantArrayType::getAllArrays on purpose, because although it makes operations you're trying to fix easier, it's really expensive performance wise. That's why I decided it's better to use the one ConstantArrayType with optional keys included, and fix all the algorithms where ConstantArrayType::getAllArrays was previously used.

[herndlm]

nice, ok then, I'll need to do something else first then ;) and thx for the immediate answer btw! :)

[herndlm]

OK, I think I'm happy with it now. I did this in 2 commits, the first one extracts generic remove(First|Last)Elements from the recently adapted removeFirst and removeLast that works with an arbitrary amount of elements. The second commit does the slice magic. And complexity-wise I think it's OK now.

I changed the return type of removeFirst from Type to self though, which causes the BC check to fail. Is this really a problem? I can change that back for sure, makes using it a bit more annoying, but that's not a big deal. I saw that removeLast also returns self, so it made sense to me also consistency-wise.

[ondrejmirtes]

Thank you very much for figuring this out :) I agree it's not really a BC break.