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

[+] does not behave like the other [binop]s on single Iterables #5205

Closed
markjreed opened this issue Feb 12, 2023 · 6 comments
Closed

[+] does not behave like the other [binop]s on single Iterables #5205

markjreed opened this issue Feb 12, 2023 · 6 comments

Comments

@markjreed
Copy link

The Problem

Given a single scalarized Iterable, [+] reduces its elements.

Expected Behavior

[+] should behave like the other reduced arithmetic operators.

Actual Behavior

[+] appears to decontainerize a single Iterable, reducing across its elements. This seems to be because it uses Array.sum when available, instead of just doing an actual reduce with +.

Steps to Reproduce

say [+] $(1,2,3)
# result: 6
# expected result: 3

Note that replacing + with any of the other three arithmetic ops here does produce 3.

Environment

  • Operating system: macOS 13.0
  • Compiler version (perl6 -v or raku -v):
    Welcome to Rakudo™ v2022.12.
    Implementing the Raku® Programming Language v6.d.
    Built on MoarVM version 2022.12.
@lizmat lizmat closed this as completed in 5c66515 Feb 13, 2023
@lizmat
Copy link
Contributor

lizmat commented Feb 13, 2023

Thanks for the report!

@thundergnat
Copy link
Contributor

thundergnat commented Feb 16, 2023

Actually, due to single argument, I would expect this to produce 6. A stronger case could be made that say [+] $(1,2,3), (note trailing comma) should return 3.

Arguably [-] $(1,2,3), [*] $(1,2,3) and [/] $(1,2,3) are misbehaving and acting like [-] $(1,2,3),, [*] $(1,2,3), and [/] $(1,2,3),

lizmat added a commit that referenced this issue Feb 16, 2023
And also do *NOT* special case [+] anymore.  With new-disp, that
optimization was probably questionable anyway.  Should this show
any performance regressions, we can look at that later again, or
wait until RakuAST's grammar has landed.

This should address #5209
and keep #5205 happy as well.
@lizmat
Copy link
Contributor

lizmat commented Feb 16, 2023

Yeah, but they have been exposing this behaviour for a long time already. So fixing would probably need to be done at a language level.

@markjreed
Copy link
Author

@thundergnat I agree that it would at least be more useful for the other operators to behave like [+] in this regard. It should be consistent either way, though.

@lizmat So for those of us keeping score at home, you tried to fix this, it broke other stuff, so the fix was reverted. Right?

@lizmat
Copy link
Contributor

lizmat commented Feb 16, 2023

Yes, the fix was reverted, in the sense that [+] no longer uses sum internally, so it now works just like the other meta infix ops, and so should have the same semantics. So if you're using [+], the fix is still there.

@thundergnat
Copy link
Contributor

Well, I am mildly of the opinion that the meta reduce binops should follow the single argument rule, for consistency if nothing else, but also think that it is a relatively minor issue in general and that core devs time may be better spent on more critical things.

And, like lizmat said, they have been that way for some time; changing them should at least be discussed in problem-solving, with maybe some input from jnthn or even (wistfully) Timtoady.

lizmat added a commit that referenced this issue Feb 22, 2023
And also do *NOT* special case [+] anymore.  With new-disp, that
optimization was probably questionable anyway.  Should this show
any performance regressions, we can look at that later again, or
wait until RakuAST's grammar has landed.

This should address #5209
and keep #5205 happy as well.
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

No branches or pull requests

3 participants