Should string/bytes multiplication be commutative?

[e-kayrakli]

Today we can do:

writeln("foo"*3);
writeln(b"foo"*3);

but we cannot do

writeln(3*"foo");
writeln(3*b"foo");  // I was curious whether this would parse, it does (but doesn't resolve)

because we don't have the overloads of these operator with symmetric arguments.

I think we should support int*string and int*bytes operations, as well.

[e-kayrakli]

I have added "good first issue" on this because of ease of implementation. Before we implement this, I want to make sure that we have consensus that we want this (and that I am not missing anything as to why we don't have this today).

[bradcray]

I never even realize/remember we support this form of multiplication until someone mentions it once every blue moon. Is this "multiply string by integer" a feature that's found in other scripting-like languages or did we make it up? If others support it, do they support the commutative form as well? If so, I agree we should as well.

If there isn't a precedent for it, I'd be inclined to deprecate it (or move it into a helper module that has to be explicitly used/included), where my main concern about it is that it's the type of feature where if the example is not as literal as the one above (say, it's a * b, and you think you're doing something times intint multiplication, but one of your int expressions is actually a string (whoops!), you're likely to get pretty surprised. Of course, you're going to get surprised eventually anyway, but if Chapel is an outlier language by supporting stringint multiplication, then better to be surprised by getting a "no such overload" compiler error?

[e-kayrakli]

Python does support it, and commutatively:

> python3
Python 3.9.1 (default, Dec 28 2020, 11:22:14)
[Clang 11.0.0 (clang-1100.0.33.17)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> "s"*3
'sss'
>>> 3*"s"
'sss'

> python
Python 2.7.16 (default, Jan 27 2020, 04:46:15)
[GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.37.14)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> "s"*3
'sss'
>>> 3*"s"
'sss'

[bradcray]

Thanks, that's good enough for me!

[shubhamkmr04]

Hey @e-kayrakli , can I work on this issue?

[e-kayrakli]

@shubhamkmr04 -- Thanks for the interest! After checking with others offline, as well, I think we can proceed with this.

• I expect this work to be simply adding two new operators similar to:

  • string multiplication: https://github.com/chapel-lang/chapel/blob/master/modules/internal/String.chpl#L2145
  • bytes multiplication: https://github.com/chapel-lang/chapel/blob/master/modules/internal/Bytes.chpl#L1101

  Where both new functions will have reversed order of arguments. And they'll end up calling the same doMultiply internal helper.

• For now, I think we can add pragma "no doc" on the new operators; I think existing documentation should be sufficient. Optionally, we can add to that documentation that the operation is commutative

• Unfortunately, our test coverage for string/bytes multiplication looks limited. I see some multiplications here and there, but not a dedicated test. So, it'd be nice for this PR to add a simple string and bytes multiplication test. The good side of that is that I don't see a lot of edge cases in this operation. Maybe an obvious one is ""*5 and b""*5, and their reversed counterparts.

• A caveat for this work: we have Add deprecation warning for operators declared without the keyword #17503 pending and to be merged soon. See that, it'll change existing procs to be operator instead (a new keyword in the language). So, while preparing your PR, please make sure you use operator and not proc, and refrain from touching other operators to avoid merge conflicts with that PR.

Let me know if you have any questions and if/when you have PR on this.

[shubhamkmr04]

Hey @e-kayrakli Thanks for the explanation, These are the changes I am have done so far:

• I have added the proc *(n:integral, s: string) and proc *(n:integral, s:bytes) in the files String.chpl and Bytes.chpl respectively with pragma "no doc" , which have reversed order of arguments as compared to them before, so we are now able to do this:

writeln(3*"foo");
writeln(3*b"foo");

• I added it in the documentation that the operation is commutative.
  
  String.chpl now looking like
  

I got some doubts too:

1. Should we explain the operation on bytes using blocks? the same way we explained it in the strings (See the screenshot I shared above from String.chpl)
2. Right now it is passing the tests ""*3 , 3*"" , b""*3 and 3*b"" . I am getting empty strings in the result as we expected. I am not very familiar with the testing system. Can you please tell me how should I add tests to my PR?
3. You said that we are converting procs to operator in the upcoming merges. This means I should use operator and not proc in my two newly added functions and I should not change proc to operator in the two previously used functions for strings/bytes*int operations, right?

proc *(s: string, n: integral) {
return doMultiply(s, n);
}

pragma "no doc"
operator *(n:integral, s: string) {
return doMultiply(s, n);
}

[e-kayrakli]

All good news! Feel free to open a draft PR even if everything is not done. We can discuss these matters under a PR.

I would probably take a simpler approach in documenting this. (I also think that the documentation for string/bytes operators can use some more editing, but that's not what you should do in this PR.) For now, maybe just add "The operation is commutative.", right before the existing "For example:", and then add the reversed version in the existing code-block, and update the output, obviously.

Should we explain the operation on bytes using blocks? the same way we explained it in the strings (See the screenshot I shared above from String.chpl)

That's a good idea. Frankly, I can do with or without the examples, but I wish string/bytes versions are consistent. And while we already have an example for the string version, let's add the same thing to the bytes version.

Right now it is passing the tests ""3 , 3"" , b""3 and 3b"" . I am getting empty strings in the result as we expected. I am not very familiar with the testing system. Can you please tell me how should I add tests to my PR?

Ah, I have missed to add some pointers about it. First make sure that the test infrastructure is built: make test-venv. Then, for this particular case, the easiest solution is: (1) add a test/types/string/multiplication.chpl which has those and some other cases for string multiplication, (2) run it, make sure the output is as expected, (3) put that output in test/types/string/multiplication.good, (4) run start_test test/types/string/multiplication.chpl, and make sure that it is reported as success. Repeat the same thing for bytes and in directory test/types/bytes. See https://github.com/chapel-lang/chapel/blob/master/doc/rst/developer/bestPractices/TestSystem.rst for more about writing tests.

A bonus here is to write a single Chapel file that tests with both string and bytes types. This would involve casting all string/bytes literals to a config type dataType and using a multiplication.compopts file. See test/types/string/slices/slices.chpl and the .compopts file with the same name.

You said that we are converting procs to operator in the upcoming merges. This means I should use operator and not proc in my two newly added functions and I should not change proc to operator in the two previously used functions for strings/bytes*int operations, right?

That's exactly right. Leave all the existing procs as-is, but make yours use operator instead. In other words, your code will look inconsistent with the rest of the implementation. But that'll be adjusted via #17503. See this line, for example: https://github.com/chapel-lang/chapel/pull/17503/files#diff-676bd7034a7d6b4a6f4df70f72936ac03b5ab5d189e69bb9d2bac8c6bcfb38d4R1101