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

At least 2 defects in javalib Collectors#joiner method #3409

Closed
LeeTibbert opened this issue Jul 23, 2023 · 2 comments · Fixed by #3421
Closed

At least 2 defects in javalib Collectors#joiner method #3409

LeeTibbert opened this issue Jul 23, 2023 · 2 comments · Fixed by #3421

Comments

@LeeTibbert
Copy link
Contributor

LeeTibbert commented Jul 23, 2023

Whilst studying the ripples of PR #3396, I discovered two defects in the javalib Colectors#joining method(s).

  • The first is user visible, albeit hard to provoke: the combiner.apply(arg1, arg2) function fail the deliminator
    with which the collector was created between arg1 & arg2.
    (For the detail oriented, the collector can not be created with a null deliminator, but it can be created with
    an empty string as the deliminator. It is hard to distinguish the cases of the empty string not being added
    and it having been properly added. Other deliminators are clearly visible to the determined.)

    Imagine the case of Collectors.joining("|"). A currently private test gives the expected results on JVM but
    fails on Scala Native:

Test org.scalanative.testsuite.javalib.util.stream.CollectorsTest.collectorsJoining_MergeSN failed: 
org.junit.ComparisonFailure: unexpected combined expected:<Left[|]Right> but was:<Left[]Right>
  • The second defect is visible to people wanting to use the Collector.joining#merge method. Normally, this
    would be either nobody or library writers.

    Reverse engineering (but not looking at the JDK code) shows that JVM
    uses a StringJoiner for the accumulator A method in [T, A, R]. Scala Native uses a StringBuilder.
    To be fair (& defensive), StringJoiner was not available when that code was written.

    Normally the A methods is abstract and "do not care". In the admittedly abstruse Test mentioned above, the
    difference means that the same Test can not be compiled & run on both JVM and Scala Native.

    Those writing implementations of parallel stream methods, say a parallel collect() are likely to use the
    combiner method and encounter difficulties.

@LeeTibbert
Copy link
Contributor Author

Normally the A methods is abstract and "do not care". In the admittedly abstruse Test mentioned above, the
difference means that the same Test can not be compiled & run on both JVM and Scala Native.

Later: I re-wrote the test so that it can execute without knowing the accumulator implementation type.
The introduces the restriction that both arguments are allocated by the exact same Supplier.
(which gets the unseen internal type A the same). This means that the prefix, suffix, & deliminator
for both arguments must be the same. OK in a test, but perhaps problematical in the World.

          In my trial Collectors.joining() code, I changed the implementation type to `StringJoiner` to match
          the JVM.  Now that the type A has been reverse engineered and `StringJoiner` (will soon) exist(s),
          no real sense to SN differing.

@LeeTibbert LeeTibbert self-assigned this Jul 27, 2023
@LeeTibbert
Copy link
Contributor Author

A fix for this issue is ready a day or two after StringJoiner becomes available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant