Skip to content

Make StringOps a pure value class #6565

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.

Sign up for GitHub

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

Jump to bottom
Merged
lrytz merged 1 commit into from
May 14, 2018
Merged

Make StringOps a pure value class #6565

lrytz merged 1 commit into from
May 14, 2018

Conversation

@szeiger
Copy link
Contributor

@szeiger szeiger commented Apr 26, 2018

This is the same treatment we gave ArrayOps previously. Some of the
more complex and nonsensical (for Strings) operations are provided for
compatibility. They delegate to WrappedString. Most operations have
efficient implementations in StringOps.

Fixes scala/bug#10845.
Fixes scala/bug#10844.

I've changed the signature of contains to disallow widening (and thus the need for expensive equality tests):

  def contains(elem: Char): Boolean = s.indexOf(elem) >= 0

Other collection-values methods with widening (from Char to Any) are probably not very useful for Strings. We should consider deprecating them.

The following methods are new overloads. They were missing for completeness:

  @`inline` def concat(suffix: String): String = s + suffix
  def prependedAll(prefix: String): String = prefix + s
  @`inline` def ++: (prefix: String): String = prependedAll(prefix)
  @`inline` def appendedAll(suffix: String): String = s + suffix

@julienrf
Copy link
Contributor

julienrf commented Apr 26, 2018

How can we be sure that we don’t forget operations? (like it was the case with scanLeft on ArrayOps, or like we have in scala/bug#10852)

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Apr 26, 2018
@lrytz
Copy link
Member

lrytz commented Apr 27, 2018

Hmm... the change is not binary compatible and causes the partest binary to fail

[test] Caused by: sbt.ForkMain$ForkError: java.lang.NoSuchMethodError: scala.collection.StringOps.drop(I)Ljava/lang/Object;
[test] 	at scala.tools.partest.nest.Runner.retainOn$1(Runner.scala:332)

@lrytz
Copy link
Member

lrytz commented Apr 27, 2018

#6566 would fix that issue

@szeiger
Copy link
Contributor Author

szeiger commented Apr 27, 2018

I went through the scaladoc generated from the previous implementation to make sure that I don't overlook any operations. AFAIR @lrytz linked to a tool to automate this in some ticket but I can't find it anymore.

We still need to fix ArrayOps, I noticed several more methods that are missing for which there is no ticket yet.

@lrytz
Copy link
Member

lrytz commented Apr 27, 2018

In https://github.com/scala/collection-strawman/issues/419 I proposed some crude code that uses reflection to show members

lrytz
lrytz reviewed May 10, 2018
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which are the "other collection-values methods with widening" that are "not very useful"? It would probably be good to deprecate them.

src/library/scala/collection/StringOps.scala Outdated
* @return the first char of this string.
* @throws NoSuchElementException if the string is empty.
*/
def head: Char = if(s.isEmpty) throw new NoSuchElementException else s.charAt(0)
Copy link
Member

@lrytz lrytz May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also include a message "head of empty string"

@szeiger
Copy link
Contributor Author

szeiger commented May 11, 2018

All widening operations (excluding @inline aliases):

Standard collection operations that are overloaded in StringOps and therefore need a copy of the original method. If we want to keep an implicit conversion from String to Seq[Char] we have to keep them. And I reversed my stance on this conversion: I don't think we should remove it. A String is already conceptually a sequence of chars, treating it as a Seq[Char] is not a problem. The problem is that "sequence of chars" is not very meaningful in Unicode.

def concat[B >: Char](suffix: Iterable[B]): immutable.IndexedSeq[B]
def padTo[B >: Char](len: Int, elem: B): immutable.IndexedSeq[B]
def prepended[B >: Char](elem: B): immutable.IndexedSeq[B]
def prependedAll[B >: Char](prefix: Iterable[B]): immutable.IndexedSeq[B]
def appended[B >: Char](elem: B): immutable.IndexedSeq[B]
def patch[B >: Char](from: Int, other: IterableOnce[B], replaced: Int): immutable.IndexedSeq[B]

Useful because it allows widening to Object[] (with a cast):

def toArray[B >: Char : ClassTag]: Array[B]

Widening doesn't make much sense here. We could remove the widening.

def fold[A1 >: Char](z: A1)(op: (A1, A1) => A1): A1

These methods could be removed completely. You'll still be able to call the version in WrappedString (so the return type will be WrappedString instead of String) but IMHO they are useless for Strings.

def diff(that: Seq[_ >: Char]): String
def intersect(that: Seq[_ >: Char]): String
def sorted[B >: Char](implicit ord: Ordering[B]): String

@szeiger szeiger force-pushed the wip/stringops-overhaul branch from b9d20a1 to e759e8e Compare May 11, 2018 14:19
@szeiger
Copy link
Contributor Author

szeiger commented May 11, 2018

Rebased and updated.

@adriaanm

This comment has been minimized.

Sign in to view
@adriaanm

This comment has been minimized.

Sign in to view
@lrytz
Copy link
Member

lrytz commented May 14, 2018

The failure here is explained in c5733d2. We need to rebase after merging #6611

@lrytz lrytz force-pushed the wip/stringops-overhaul branch from e759e8e to 5e60987 Compare May 14, 2018 12:01
@lrytz
Copy link
Member

lrytz commented May 14, 2018

Rebased on top of #6611 (and fixed some exception messages)

@lrytz lrytz force-pushed the wip/stringops-overhaul branch 6 times, most recently from da99b03 to 6d36d86 Compare May 14, 2018 13:20
@szeiger @lrytz
Make StringOps a pure value class
7c68757 
This is the same treatment we gave ArrayOps previously. Some of the
more complex and nonsensical (for Strings) operations are provided for
compatibility. They delegate to WrappedString. Most operations have
efficient implementations in StringOps.
@lrytz lrytz force-pushed the wip/stringops-overhaul branch from 6d36d86 to 7c68757 Compare May 14, 2018 14:14
@lrytz lrytz mentioned this pull request May 14, 2018
Closed
@lrytz lrytz merged commit 16e9bc1 into scala:2.13.x May 14, 2018
@szeiger szeiger mentioned this pull request May 30, 2018
Merged
@lrytz lrytz mentioned this pull request Aug 20, 2018
Merged
@julienrf julienrf mentioned this pull request Oct 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrytz lrytz lrytz left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.13.0-M4

Development

Successfully merging this pull request may close these issues.

Move StringOps out of the collections hierarchy String.patch does not anymore return a String when it could

5 participants

@szeiger @julienrf @lrytz @adriaanm @scala-jenkins