-
Notifications
You must be signed in to change notification settings - Fork 21
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
Implement the Any2StringAdd rewrite #10
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* | ||
rule = fix.scala213.Any2StringAdd | ||
*/ | ||
package fix.scala213 | ||
|
||
abstract class Any2StringAdd { | ||
// Strings: leave as-is, both literal strings and non-literal | ||
def s = "bob" | ||
def str1 = s + s | ||
def str2 = s + "bob" | ||
def str3 = "bob" + s | ||
def str4 = "bob" + "fred" | ||
|
||
// Non-strings: add toString | ||
def nil = Nil + s | ||
|
||
// Non-string, generic type: add toString | ||
type A | ||
def x: A | ||
def generic = x + "bob" | ||
|
||
// Primitives: add toString | ||
def unit = () | ||
def bool = true | ||
def byte = 1.toByte | ||
def short = 1.toShort | ||
def char = 'a' | ||
def int = 1 | ||
def long = 1L | ||
def float = 1.0F | ||
def double = 1.0 | ||
// | ||
def unit1 = unit + s | ||
def bool1 = bool + s | ||
def byte1 = byte + s | ||
def byte2 = byte + byte | ||
def short1 = short + s | ||
def short2 = short + short | ||
def char1 = char + s | ||
def char2 = char + char | ||
def int1 = int + s | ||
def int2 = int + int | ||
def long1 = long + s | ||
def long2 = long + long | ||
def float1 = float + s | ||
def float2 = float + float | ||
def double1 = double + s | ||
def double2 = double + double | ||
|
||
// With infix operators, make sure to use parens | ||
def parens1 = Nil ++ Nil + s | ||
def parens2 = int + int + s | ||
def parens3 = {Nil ++ Nil} + s | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package fix.scala213 | ||
|
||
abstract class Any2StringAdd { | ||
// Strings: leave as-is, both literal strings and non-literal | ||
def s = "bob" | ||
def str1 = s + s | ||
def str2 = s + "bob" | ||
def str3 = "bob" + s | ||
def str4 = "bob" + "fred" | ||
|
||
// Non-strings: add toString | ||
def nil = Nil.toString + s | ||
|
||
// Non-string, generic type: add toString | ||
type A | ||
def x: A | ||
def generic = x.toString + "bob" | ||
|
||
// Primitives: add toString | ||
def unit = () | ||
def bool = true | ||
def byte = 1.toByte | ||
def short = 1.toShort | ||
def char = 'a' | ||
def int = 1 | ||
def long = 1L | ||
def float = 1.0F | ||
def double = 1.0 | ||
// | ||
def unit1 = unit.toString + s | ||
def bool1 = bool.toString + s | ||
def byte1 = "" + byte + s | ||
def byte2 = byte + byte | ||
def short1 = "" + short + s | ||
def short2 = short + short | ||
def char1 = "" + char + s | ||
def char2 = char + char | ||
def int1 = "" + int + s | ||
def int2 = int + int | ||
def long1 = "" + long + s | ||
def long2 = long + long | ||
def float1 = "" + float + s | ||
def float2 = float + float | ||
def double1 = "" + double + s | ||
def double2 = double + double | ||
|
||
// With infix operators, make sure to use parens | ||
def parens1 = (Nil ++ Nil).toString + s | ||
def parens2 = "" + (int + int) + s | ||
def parens3 = {Nil ++ Nil}.toString + s | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
fix.scala213.Any2StringAdd | ||
fix.scala213.Core | ||
fix.scala213.ScalaSeq | ||
fix.scala213.Varargs |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package fix.scala213 | ||
|
||
import scalafix.v1._ | ||
|
||
import scala.meta._ | ||
|
||
object Any2StringAdd { | ||
val any2stringaddPlusString = SymbolMatcher.exact("scala/Predef.any2stringadd#`+`().") | ||
val primitivePlusString = SymbolMatcher.exact( | ||
"scala/Byte#`+`().", | ||
"scala/Short#`+`().", | ||
"scala/Char#`+`().", | ||
"scala/Int#`+`().", | ||
"scala/Long#`+`().", | ||
"scala/Float#`+`().", | ||
"scala/Double#`+`().", | ||
) | ||
} | ||
|
||
final class Any2StringAdd extends SemanticRule("fix.scala213.Any2StringAdd") { | ||
import Any2StringAdd._ | ||
|
||
override def fix(implicit doc: SemanticDocument): Patch = { | ||
doc.tree.collect { | ||
case any2stringaddPlusString(Term.ApplyInfix(lhs, _, _, _)) => addToString(lhs) | ||
case primitivePlusString(Term.ApplyInfix(lhs, _, _, _)) => blankStringPlus(lhs) | ||
}.asPatch | ||
} | ||
|
||
private def addToString(term: Term) = term match { | ||
case _: Term.Name | _: Term.Select | _: Term.Block => Patch.addRight(term, ".toString") | ||
case _ => Patch.addLeft(term, "(") + Patch.addRight(term, ").toString") | ||
} | ||
|
||
private def blankStringPlus(term: Term) = term match { | ||
case _: Term.Name | _: Term.Select | _: Term.Block => Patch.addLeft(term, """"" + """) | ||
case _ => Patch.addLeft(term, """"" + (""") + Patch.addRight(term, ")") | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my only concern here is that it unnecessarily boxes primitives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that must be why the deprecation message for these is to use string interpolation.
Should we do
"" + x
for these? (And"" + (...)
for non-Name/Select/Block.) . Or should we try to rewrite it to use string interpolation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. IIRC, string interpolation is special-cased by the compiler in 2.13, so it's probably at least as efficient as other options, if not more so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's the right choice. But for mechanical rewrites you could get some weird rewrites, so maybe this is best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. see scala/scala#7201 scala/bug#11025