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

Deprecate string-building using + with a non-String type on the left (aka any2stringadd) #6315

Merged
merged 2 commits into from
Jun 4, 2018

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Feb 8, 2018

Ref scala/bug#194

  1. This deprecates scala.Predef.any2stringadd.
  2. Another thing it does is to remove the conversion altogether under -Xsource:2.14 flag.

Background

Scala up till 2.12 has had two kinds of String concatenation operator + (also known as PLUS or ADD).

a. When the receiver is a non-String, it uses implicit Predef.any2addstring to inject +.
b. When the receiver is a String, the compiler injects a special method String_+.

In other words, true + "what" and "what" + true use different mechanisms.
This change addresses the first variant that introduces + operator into Any.
The rationale discussed in scala/bug#194 is that this implicit injection removes too much type safety, leading to confusing results.

Here some examples listed:

scala> var v = Vector[Int]()
v: scala.collection.immutable.Vector[Int] = Vector()

scala> v += 3
<console>:13: error: value += is not a member of scala.collection.immutable.Vector[Int]
  Expression does not convert to assignment because:
    type mismatch;
     found   : Int(3)
     required: String
    expansion: v = v.$plus(3)
       v += 3
         ^

scala> val xs = List(1, 2, 3)
xs: List[Int] = List(1, 2, 3)

scala> xs foreach { println _ + 1 }
<console>:13: error: type mismatch;
 found   : Int(1)
 required: String
       xs foreach { println _ + 1 }
                                ^

Note that in both examples the person who wrote the code were not trying to concatenate String.

Implementation note

Initially I've attempted to implement what @adriaanm proposed in 2014 (scala/bug#194 (comment)) and remove all deprecated implicits under -Xfuture flag. A simple implementation

val fd = settings.future && sym.isDeprecated

did not work, because the implicit caching is unware of the annotations?

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Feb 8, 2018
@hrhino
Copy link
Contributor

hrhino commented Feb 8, 2018

At last!

In what situation does settings.future && sym.isDeprecated not work?

I do think that the "deprecation removes implicit eligibility under -Xfuture" feature should be written down somewhere other than a spare comment in Contexts. The scaladoc would make sense to me.

Also, is -Xfuture sticking around? I guessed that it might be getting folded in with -Xsource:2.(n+1), but maybe that's just -Xexperimental.

@@ -830,6 +830,13 @@ trait Contexts { self: Analyzer =>
private def isQualifyingImplicit(name: Name, sym: Symbol, pre: Type, imported: Boolean) =
sym.isImplicit &&
isAccessible(sym, pre) &&
!({
// [eed3si9n] ideally I'd like to do this: val fd = settings.future && sym.isDeprecated
// but implicit caching currently does not report sym.isDeprecated correctly.
Copy link
Member Author

Choose a reason for hiding this comment

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

@hrhino In this position, I originally wrote val fd = settings.future && sym.isDeprecated, but sym here didn't return isDeprecated at least with the tests that I was using. I suspect https://github.com/scala/scala/blob/v2.13.0-M3/src/reflect/scala/reflect/internal/Symbols.scala#L3426-L3434 caching without the annotation.

@eed3si9n
Copy link
Member Author

eed3si9n commented Feb 8, 2018

@hrhino

Also, is -Xfuture sticking around? I guessed that it might be getting folded in with -Xsource:2.(n+1), but maybe that's just -Xexperimental.

You're right - scala/scala-dev#430.

I'll change to -Xsource:2.14.

@dwijnand
Copy link
Member

dwijnand commented Feb 8, 2018

Removing String#+(Any) was closed because it lacked a SIP. Does this one not?

#5235 (comment)

@eed3si9n
Copy link
Member Author

eed3si9n commented Feb 8, 2018

@dwijnand Does an implementation come first or the SIP? I thought it got closed because we ran out of time.

@sjrd
Copy link
Member

sjrd commented Feb 8, 2018

String.+(Any) doesn't cause confusion, and remains the most cross-platformly efficient way to convert to string + concatenate. It is also quite necessary as a primitive in Scala.js for various reasons. In particular, for x a primitive type (e.g., Int), x.toString() is ultimately implemented in terms of "" + x, not the other way around. I would be strongly opposed to its removal.

If someone wants to remove it and prepares an implementation, make sure to also prepare an implementation in Scala.js, so that you feel the pain of its absence ;)

@eed3si9n
Copy link
Member Author

@dwijnand @sjrd scala/bug#194 is titled "any2stringadd implicit should be removed from Predef." This PR proposes the following actions:

  • Scala 2.13: Deprecate Predef.any2stringadd. Remove when used with -Xsource:2.14
  • Scala 2.14: Remove Predef.any2stringadd

Whether the deprecation/removal is good idea or not I think should continue in scala/bug#194 to avoid duplication. (I'd be happy to discuss implementation on this PR here of course)

As it stands, this PR doesn't say anything about the String#+(Any) operator; and no one has told me (yet) that the deprecation/removal of Predef.any2stringadd must coincide with String#+(Any).

@Ichoran
Copy link
Contributor

Ichoran commented Feb 10, 2018

I find any2stringadd a dreadfully type-unsafe nuisance and never have trouble with String#+(Any), so this seems like exactly the right way to go to me.

@dwijnand
Copy link
Member

I guess I'm the lone idiot who's been accidentally appending Option[String]s (and the like) to strings, not realising it was an option of string.

@eed3si9n
Copy link
Member Author

@adriaanm adriaanm changed the title Deprecate scala.Predef.any2stringadd Deprecate scala.Predef.any2stringadd [ci: last-only] Feb 14, 2018
@Jasper-M
Copy link
Contributor

Jasper-M commented Feb 14, 2018

I personally find String#+(Any) to also be a type-unsafe nuisance. And also a pain in the ass when developing a DSL. At least you can turn off any2stringadd by shadowing its name. There's no easy way to make String#+(Any) go away.

@SethTisue
Copy link
Member

I suggest we keep this PR focused on any2stringadd, and take up String#+(Any) separately.

@SethTisue
Copy link
Member

SethTisue commented Feb 21, 2018

@eed3si9n would you mind squashing this into a single commit...?

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 21, 2018
@eed3si9n
Copy link
Member Author

@SethTisue Done.

@SethTisue SethTisue changed the title Deprecate scala.Predef.any2stringadd [ci: last-only] Cast scala.Predef.any2stringadd into Mount Doom [ci: last-only] Feb 23, 2018
@SethTisue SethTisue changed the title Cast scala.Predef.any2stringadd into Mount Doom [ci: last-only] Deprecate scala.Predef.any2stringadd [ci: last-only] Feb 23, 2018
@eed3si9n eed3si9n force-pushed the wip/deprecate-any2stringadd branch from 48076cb to 0290579 Compare March 4, 2018 21:12
@eed3si9n
Copy link
Member Author

eed3si9n commented Mar 4, 2018

Rebased to catch up with #6093

@SethTisue
Copy link
Member

[test] !!  786 - run/t10551.scala                          [output differs]
[test] % diff /home/jenkins/workspace/scala-2.13.x-validate-main@2/test/files/run/t10551-run.log /home/jenkins/workspace/scala-2.13.x-validate-main@2/test/files/run/t10551.check
[test] @@ -1,4 +1,3 @@
[test] -warning: there was one deprecation warning (since 2.13.0); re-run with -deprecation for details

@eed3si9n eed3si9n force-pushed the wip/deprecate-any2stringadd branch from 0290579 to ead89e0 Compare March 4, 2018 22:19
@eed3si9n
Copy link
Member Author

eed3si9n commented Mar 4, 2018

Updated the test with toString.

@eed3si9n
Copy link
Member Author

eed3si9n commented Mar 4, 2018

@SethTisue What's next? Is there a process of picking a reviewer?

@SethTisue
Copy link
Member

Is there a process of picking a reviewer?

blindfold yourself, then throw a dart at a printout of https://github.com/scala/scala/blob/2.13.x/README.md#get-in-touch ?

I'd suggest @lrytz for a first pass, and then @adriaanm should probably sign off on anything as fundamental as this.

@SethTisue SethTisue changed the title Deprecate scala.Predef.any2stringadd [ci: last-only] Deprecate scala.Predef.any2stringadd Mar 6, 2018
@eed3si9n eed3si9n requested a review from lrytz March 6, 2018 02:57
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.

💟

!({
// [eed3si9n] ideally I'd like to do this: val fd = settings.isScala214 && sym.isDeprecated
// but implicit caching currently does not report sym.isDeprecated correctly.
val fd = settings.isScala214 && (name ne null) && definitions.isPredefMemberNamed(sym, TermName("any2stringadd"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need some finessing, as I assume this is performance critical. I suggest caching the any2stringadd symbol and comparing directly against it, potentially along with the other caches just above. I'd also split this part of the condition into a separate method isDeprecatedImplicit. I'm going to defer my review to @retronym, since he's more familiar with the performance trade offs here.

Copy link
Member

Choose a reason for hiding this comment

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

ping @retronym

Copy link
Member

@retronym retronym May 29, 2018

Choose a reason for hiding this comment

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

Yes, it should be sym == currentRun.runDefintions.Predef_ any2stringadd, after adding lazy val Predef_ any2stringadd = .... to RunDefintions.

Copy link
Member Author

Choose a reason for hiding this comment

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

The .... part where it's left to the reader as an exercise turned out to be oddly tricky (for me). I could not get sym to equal Predef_any2stringadd when I defined it as getMemberMethod(PredefModule, nme.any2stringadd).

It turns out that any2stringadd is an implicit class, and the implicit class creates both a companion object AND a factory method also named def any2stringadd[A](a: A)! This is not normally allowed, right?

What seems to be working is:

PredefModule.info.decl(nme.any2stringadd.toTermName).suchThat(_.isMethod)

@adriaanm adriaanm requested a review from retronym April 30, 2018 13:22
@szeiger szeiger modified the milestones: 2.13.0-M4, 2.13.0-M5 May 2, 2018
@eed3si9n eed3si9n force-pushed the wip/deprecate-any2stringadd branch from 7811481 to e83c6dc Compare June 2, 2018 09:06
@eed3si9n
Copy link
Member Author

eed3si9n commented Jun 2, 2018

Rebased against 2.13.x.

@eed3si9n eed3si9n force-pushed the wip/deprecate-any2stringadd branch from e83c6dc to c647d6b Compare June 2, 2018 09:48
Ref scala/bug#194

1. This deprecates scala.Predef.any2stringadd.
2. Another thing it does is to remove the conversion altogether under `-Xsource:2.14` flag.

Scala up till 2.12 has had two kinds of String concatenation operator `+` (also known as PLUS or ADD).

a. When the receiver is a non-String, it uses implicit `Predef.any2addstring` to inject `+`.
b. When the receiver is a String, the compiler injects a special method `String_+`.

In other words, `true + "what"` and `"what" + true` use different mechanisms.
This change addresses the first variant that introduces `+` operator into `Any`.
The rationale discussed in scala/bug#194 is that this implicit injection removes too much type safety, leading to confusing results.

Here some examples listed:

```
scala> var v = Vector[Int]()
v: scala.collection.immutable.Vector[Int] = Vector()

scala> v += 3
<console>:13: error: value += is not a member of scala.collection.immutable.Vector[Int]
  Expression does not convert to assignment because:
    type mismatch;
     found   : Int(3)
     required: String
    expansion: v = v.$plus(3)
       v += 3
         ^

scala> val xs = List(1, 2, 3)
xs: List[Int] = List(1, 2, 3)

scala> xs foreach { println _ + 1 }
<console>:13: error: type mismatch;
 found   : Int(1)
 required: String
       xs foreach { println _ + 1 }
                                ^
```

Note that in both examples the person who wrote the code were not trying to concatenate String.

Initially I've attempted to implement what Adriaan proposed in 2014 (scala/bug#194 (comment)) and remove all deprecated implicits under `-Xfuture` flag. A simple implementation

```scala
val fd = settings.future && sym.isDeprecated
```

did not work, because the implicit caching is unware of the annotations?
@eed3si9n eed3si9n force-pushed the wip/deprecate-any2stringadd branch from c647d6b to b94a581 Compare June 2, 2018 20:44
`any2stringadd` is an implicit class, and the implicit class creates a
factory method `def any2stringadd[A](a: A)` and since it's a SIP-15
value class there's a companion named `any2stringadd` too!
@lrytz lrytz force-pushed the wip/deprecate-any2stringadd branch from b94a581 to a9d5d38 Compare June 4, 2018 14:57
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.

Pushed a tiny change to the way the method is looked up (a9d5d38#diff-2b1bf15b53df94495f7155a69f822adeR1507). LGTM, will merge when it goes green!

@lrytz lrytz merged commit 2212d38 into scala:2.13.x Jun 4, 2018
@eed3si9n eed3si9n deleted the wip/deprecate-any2stringadd branch June 5, 2018 03:34
@SethTisue
Copy link
Member

@eed3si9n apparently some other code path is involved here:

scala> 3 + "foo"
res1: String = 3foo

note the absence of deprecation warning.

@sjrd
Copy link
Member

sjrd commented Jun 5, 2018

def +(x: String): String

All primitive types explicitly declare a def +(s: String): String, so prim + s doesn't use the implicit any2StringAdd. You would have to deprecate those explicit defs as well.

smarter added a commit to smarter/scala that referenced this pull request Jun 8, 2018
This is consistent with the deprecation of any2stringadd in scala#6315.
@smarter
Copy link
Member

smarter commented Jun 8, 2018

All primitive types explicitly declare a def +(s: String): String, so prim + s doesn't use the implicit any2StringAdd. You would have to deprecate those explicit defs as well.

I was randomly looking at Int.scala and rediscovered this, so I made a PR for that exact purpose: #6755

smarter added a commit to smarter/scala that referenced this pull request Jun 8, 2018
This is consistent with the deprecation of any2stringadd in scala#6315.
smarter added a commit to smarter/scala that referenced this pull request Jun 8, 2018
This is consistent with the deprecation of any2stringadd in scala#6315.
// [eed3si9n] ideally I'd like to do this: val fd = settings.isScala214 && sym.isDeprecated
// but implicit caching currently does not report sym.isDeprecated correctly.
val fd = settings.isScala214 && (sym == currentRun.runDefinitions.Predef_any2stringaddMethod)
if (settings.XlogImplicits && fd) echo(sym.pos, sym + " is not a valid implicit value because:\n" + "-Xsource:2.14 removes scala.Predef.any2stringadd")
Copy link
Member

Choose a reason for hiding this comment

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

This logging is pretty noisy. When is intended to be shown?

$ qscalac -Xlog-implicits -Xsource:2.14 -d /tmp src/scalap/**/*.scala
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
src/scalap/scala/tools/scalap/Classfile.scala:122: materializing requested scala.reflect.type.ClassTag[String] using scala.reflect.`package`.materializeClassTag[String]()
      for ((x, i) <- entries.zipWithIndex ; if x != null) yield
                  ^
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
src/scalap/scala/tools/scalap/scalax/rules/scalasig/ClassFileParser.scala:233: materializing requested scala.reflect.type.ClassTag[Option[Any]] using scala.reflect.`package`.materializeClassTag[Option[Any]]()
  private val values = Array.fill[Option[Any]](size)(None)
                                                    ^
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
src/scalap/scala/tools/scalap/Decode.scala:57: refArrayOps is not a valid implicit value for bytes.type => ?{def take: ?} because:
inferred type arguments [Byte] do not conform to method refArrayOps's type parameter bounds [T <: AnyRef]
      bytes take length
      ^
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
src/scalap/scala/tools/scalap/Main.scala:109: materializing requested scala.reflect.type.ClassTag[String] using scala.reflect.`package`.materializeClassTag[String]()
        classname.split('.').map(NameTransformer.encode _).mkString(".")
                                ^
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
src/scalap/scala/tools/scalap/scalax/rules/Memoisable.scala:50: error: value + is not a member of AnyRef
      if(DefaultMemoisable.debug) println(key + " -> " + other)
                                              ^
src/scalap/scala/tools/scalap/scalax/rules/Memoisable.scala:56: error: value + is not a member of AnyRef
    if(DefaultMemoisable.debug) println(key + " -> " + t + " (" + out + ")")
                                            ^
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
src/scalap/scala/tools/scalap/scalax/rules/scalasig/ScalaSig.scala:33: wrapRefArray is not a valid implicit value for Array[Byte] => scala.collection.IterableOnce[?] because:
inferred type arguments [Byte] do not conform to method wrapRefArray's type parameter bounds [T <: AnyRef]
      case ConstValueIndex(index) => bytesForIndex(index)
                                                  ^
src/scalap/scala/tools/scalap/scalax/rules/scalasig/ScalaSig.scala:34: materializing requested scala.reflect.type.ClassTag[Byte] using scala.reflect.`package`.materializeClassTag[Byte]()
    }.toArray
      ^
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
src/scalap/scala/tools/scalap/scalax/rules/scalasig/ScalaSig.scala:45: refArrayOps is not a valid implicit value for bytes.type => ?{def take: ?} because:
inferred type arguments [Byte] do not conform to method refArrayOps's type parameter bounds [T <: AnyRef]
        ScalaSigAttributeParsers.parse(ByteCode(bytes.take(length)))
                                                ^
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
method any2stringadd is not a valid implicit value because:
-Xsource:2.14 removes scala.Predef.any2stringadd
two errors found

Copy link
Member Author

Choose a reason for hiding this comment

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

I intended it to be called only when any2stringadd was invoked, but it seems like that's not the case.

eed3si9n added a commit to eed3si9n/scala that referenced this pull request Jun 12, 2018
eed3si9n added a commit to eed3si9n/scala that referenced this pull request Jun 12, 2018
eed3si9n added a commit to eed3si9n/scala that referenced this pull request May 16, 2019
Fixes scala/bug#11527
Ref scala#6315

This is so we get the deprecation on the API documentation.
@SethTisue SethTisue changed the title Deprecate scala.Predef.any2stringadd Deprecate string-building using + with a non-String type on the left (any2stringadd) Jun 6, 2019
@SethTisue SethTisue changed the title Deprecate string-building using + with a non-String type on the left (any2stringadd) Deprecate string-building using + with a non-String type on the left (aka any2stringadd) Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.