Observe some -Xlint recommendations by som-snytt · Pull Request #6806 · scala/scala

som-snytt · 2018-06-15T07:08:13Z

Not all unused things are removed outright.

lrytz

I think we can remove unused code in general, the only exception I spotted is the check methods in ChampHashSet/Map, these probably make sense to keep in comments.

src/reflect/scala/reflect/internal/StdNames.scala

src/compiler/scala/reflect/reify/phases/Reshape.scala

src/compiler/scala/tools/cmd/Interpolation.scala

src/library/scala/Predef.scala

src/library/scala/collection/View.scala

src/library/scala/collection/immutable/ChampHashMap.scala

som-snytt · 2018-06-15T09:08:26Z

src/compiler/scala/tools/nsc/io/Socket.scala


  def printWriter()                         = new PrintWriter(outputStream(), true)
-  def bufferedReader(implicit codec: Codec) = new BufferedReader(new InputStreamReader(inputStream()))
+  def bufferedReader(implicit codec: Codec) = new BufferedReader(new InputStreamReader(inputStream(), codec.decoder))


A potential bug!

som-snytt · 2018-06-15T09:08:46Z

build.sbt

  },
  fork in run := true,
+  //scalacOptions += "-Xlint:-nullary-override,-by-name-right-associative,-inaccessible,_",
+  //scalacOptions ++= Seq("-Xmaxerrs", "5", "-Xmaxwarns", "5"),


placeholders.

som-snytt · 2018-06-15T09:09:04Z

src/compiler/scala/reflect/reify/phases/Reshape.scala

      val mods2 = toPreTyperModifiers(mods1, ddef.symbol)
      ValDef(mods2, name1, tpt0, extractRhs(rhs0))
    }
+    */


First, do no harm.

som-snytt · 2018-06-15T09:11:47Z

src/compiler/scala/tools/nsc/symtab/SymbolTrackers.scala

-        val treeStrings = symMap(sym) map { t =>
-          "%10s: %s".format(t.shortClass, t)
-        }
+        val treeStrings = symMap(sym).map(t => s"${t.shortClass}%10s: $t")


For a moment, I forgot the s interpolator. Luckily, -Xlint told me.

Actually, -Xlint complained about :+ on L182. Still need to follow up.

som-snytt · 2018-06-15T09:14:12Z

src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala

      in.bp = runtimeAnnotStart
      val numAnnots = u2
-      var i = 0
+      val i = 0


Not obvious whether it's a cut/paste bug.

This is an oversight, i should be incremented in the loop. #6816

Reverted to var.

You said you reverted it. Maybe you inverted it.

som-snytt · 2018-06-15T09:14:56Z

src/compiler/scala/tools/nsc/transform/patmat/PatternMatching.scala

          // but for the rest of us pass in top as the expected type to avoid waste.
          val pt = if (origTp <:< definitions.AnyTpe) definitions.AnyTpe else WildcardType
-          localTyper.typed(translated, definitions.AnyTpe) setType origTp
+          localTyper.typed(translated, pt) setType origTp


-Xlint would have told you that. -- Paul

som-snytt · 2018-06-15T09:15:40Z

src/compiler/scala/tools/nsc/typechecker/PatternTypers.scala

    import infer._

-    private def unit = context.unit
+    //private def unit = context.unit


can probably zap aliases.

som-snytt · 2018-06-15T09:16:36Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

      }

-      def typedTypeSelectionQualifier(tree: Tree, pt: Type = AnyRefTpe) =
+      def typedTypeSelectionQualifier(tree: Tree, pt: Type /*= AnyRefTpe*/) =


documentary default, removing unused default.

Happy to have the default removed

Made non-documentary.

som-snytt · 2018-06-15T09:17:35Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala


      def typedSingletonTypeTree(tree: SingletonTypeTree) = {
-        val refTyped = typedTypeSelectionQualifier(tree.ref, WildcardType )
+        val refTyped = typedTypeSelectionQualifier(tree.ref, WildcardType)


collateral whitespace change!

som-snytt · 2018-06-15T09:18:27Z

src/library/scala/Predef.scala

  object <:< {
    /** If `A <: B` and `B <: A`, then `A = B` (subtyping is antisymmetric) */
-    def antisymm[A, B](implicit l: A <:< B, r: B <:< A): A =:= B = =:=.singleton.asInstanceOf[A =:= B]
+    def antisymm[A, B](implicit @deprecated("unused","shim") l: A <:< B, @deprecated("unused","shim") r: B <:< A): A =:= B = =:=.singleton.asInstanceOf[A =:= B]


Made a PR to ignore these cases.

som-snytt · 2018-06-15T09:18:55Z

src/library/scala/collection/ArrayOps.scala

      def mkRowBuilder() = ArrayBuilder.make[B](ClassTag[B](aClass.getComponentType))
      val bs = new ArrayOps(asArray(xs(0))).map((x: B) => mkRowBuilder())
-      var j = 0
+      //var j = 0


safe to del.

som-snytt · 2018-06-15T09:20:34Z

src/library/scala/collection/View.scala


  /** A view that partitions an underlying collection into two views */
-  @SerialVersionUID(3L)
+  //@SerialVersionUID(3L)


These warnings in the build should be resolved. Also, maybe the warning should sit behind a flag. -Xlint:serial? -Ywarn-serial? -Ywarn-srsly?

som-snytt · 2018-06-15T09:22:12Z

src/library/scala/concurrent/Channel.scala

 */
 class Channel[A] {
-  class LinkedList[A] {
+  private class LinkedList {


we're all friends here.

som-snytt · 2018-06-15T09:25:21Z

src/reflect/scala/reflect/api/Trees.scala

   */
-  @deprecated("use q\"new ${sym.toType}(..$args)\" instead", "2.10.1")
+  private[this] final val depstr_New = "use q\"new ${sym." + "toType}(..$args)\" instead"
+  @deprecated(depstr_New, "2.10.1")


implicitNotFound gets a pass, but actually all ordinary annotations? I think macros supplying constants would be OK.

som-snytt · 2018-06-15T09:25:37Z

src/reflect/scala/reflect/api/TypeTags.scala

  /* @group TypeTags */
  // This class only exists to silence MIMA complaining about a binary incompatibility.
  // Only the top-level class (api.PredefTypeCreator) should be used.
+  @deprecated("This class only exists to silence MIMA complaining about a binary incompatibility.", since="forever")


didn't work

som-snytt · 2018-06-15T09:26:31Z

src/reflect/scala/reflect/internal/Types.scala

        super.prefixString
    )
    // Suppressing case class copy method which risks subverting our single point of creation.
+    @deprecated("Suppressing case class copy method", since="forever")


also didn't work.

som-snytt · 2018-06-15T09:27:03Z

src/reflect/scala/reflect/internal/transform/UnCurry.scala

    // so we still see repeated parameter types (uncurry replaces them with Seq)
    val isRepeated = origSym.info.paramss.flatten.map(sym => definitions.isRepeatedParamType(sym.tpe))
-    val oldPs = newInfo.paramss.head
+    val _ = newInfo.paramss.head


wary of side effects, left it in.

som-snytt · 2018-06-15T09:27:24Z

src/reflect/scala/reflect/io/IOStats.scala

 package reflect.io

-import scala.reflect.internal.util.Statistics
+//import scala.reflect.internal.util.Statistics


actually all that file is commented out.

som-snytt · 2018-06-15T09:28:10Z

src/repl-frontend/scala/tools/nsc/interpreter/shell/ILoop.scala

    * Run with output muted both from ILoop and from the intp reporter.
    */
-  private def interpretPreamble = {
+  private def interpretPreamble() = {


left most "parens mean side effects" alone.

som-snytt · 2018-06-15T20:55:09Z

Feedback is addressed. Hopefully will be merged before it fails to merge.

som-snytt · 2018-06-15T23:28:55Z

src/reflect/scala/reflect/internal/StdNames.scala

    val REIFY_FREE_PREFIX: NameType        = "free$"
    val REIFY_FREE_THIS_SUFFIX: NameType   = "$this"
-    val REIFY_FREE_VALUE_SUFFIX: NameType  = "$value"
+    val REIFY_FREE_VALUE_SUFFIX: NameType  = s"$$value"      // looks like missing interpolator due to `value` in scopre


Unfortunately, StringContext name hides the interpolator. Ironic!

lrytz

Hopefully will be merged before it fails to merge.

😢

lrytz · 2018-06-18T09:16:24Z

src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala

      in.bp = runtimeAnnotStart
      val numAnnots = u2
-      var i = 0
+      val i = 0


This is an oversight, i should be incremented in the loop. #6816

lrytz · 2018-06-18T09:37:25Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

      }

-      def typedTypeSelectionQualifier(tree: Tree, pt: Type = AnyRefTpe) =
+      def typedTypeSelectionQualifier(tree: Tree, pt: Type /*= AnyRefTpe*/) =


Happy to have the default removed

lrytz · 2018-06-18T09:40:44Z

src/reflect/scala/reflect/internal/StdNames.scala

@@ -351,7 +358,7 @@ trait StdNames {
    val MIRROR_UNTYPED: NameType           = "$m$untyped"
    val REIFY_FREE_PREFIX: NameType        = "free$"
    val REIFY_FREE_THIS_SUFFIX: NameType   = "$this"


was there no warning here?

Sharp eyes! I'll wonder if this isPrivateLocal when I'm fully awake.

I need a couple more days of sleep after the national election, then I'll wonder this. Or "$this".

lrytz · 2018-06-18T09:43:30Z

src/reflect/scala/reflect/internal/StdNames.scala

+    private class NameContext(s: String) {
+      def n(args: Any*): TermName = { require(args.isEmpty) ; createNameType("$" + s) }
+    }
+    private def StringContext(parts: String*) = { require(parts.size == 1) ; new NameContext(parts.head) }


👍 haven't seen this before

The puzzler guy didn't like my one-liner StringContext puzzler. :( It was too improbable.

For some reason, we aren't allowed to have fun things.

lrytz · 2018-06-18T09:46:47Z

LGTM otherwise, let's merge quickly after the rebase.

The consequence of introducing `StringContext` is that you can't use it as one. Just rename it to something that says it's just a name.

A few `$name` happen to look like an interpolation of a value `name`. Avoid a lint warning by using an interpolator. To avoid the idiom for dollar literal, let the interpolator also add the dollar. Another day, another dollar. Also, name it the n-terpolator.

Not all unused things are removed outright.

som-snytt · 2018-06-19T06:22:35Z

@lrytz Thanks for the review of tedious material. You make a PR better than it deserves to be!

som-snytt · 2018-06-20T03:26:29Z

There are no plain strings. Added f for emphasis.

[error] /Users/andrew/workspace/scala/src/compiler/scala/tools/util/SocketServer.scala:98: conversions must follow a splice; use %% for literal %, %n for newline
[error]           warn(f"Accept on port %d failed")

One could lint for format strings. Yesterday, in Java land, I added a private utility for the formatted message style my colleague prefers, formatted("{} and {}", objects...), when I happened to notice that I omitted the ted and it still compiled: the file had a static import of String.format.

Also the f-interpolator could be improved, as it's obviously a missing arg.

scala-jenkins added this to the 2.13.0-M5 milestone Jun 15, 2018

lrytz requested changes Jun 15, 2018

View reviewed changes

som-snytt commented Jun 15, 2018

View reviewed changes

som-snytt force-pushed the topic/imports-used branch from 8957ad7 to bb17366 Compare June 15, 2018 20:53

som-snytt commented Jun 15, 2018

View reviewed changes

som-snytt force-pushed the topic/imports-used branch from bb17366 to 49863d1 Compare June 17, 2018 05:53

lrytz reviewed Jun 18, 2018

View reviewed changes

som-snytt force-pushed the topic/imports-used branch from a2ddb96 to a3f941e Compare June 18, 2018 14:40

som-snytt added 2 commits June 18, 2018 14:03

Avoid naming a term StringContext if it isn't one

7a57059

The consequence of introducing `StringContext` is that you can't use it as one. Just rename it to something that says it's just a name.

som-snytt added 2 commits June 18, 2018 14:06

Observe some -Xlint recommendations

2cd51ee

Not all unused things are removed outright.

Add intended but missing Serializable in View

8911b4b

som-snytt force-pushed the topic/imports-used branch from a3f941e to 8911b4b Compare June 18, 2018 21:08

lrytz approved these changes Jun 19, 2018

View reviewed changes

lrytz merged commit 9df13fd into scala:2.13.x Jun 19, 2018

som-snytt deleted the topic/imports-used branch June 19, 2018 06:22

som-snytt mentioned this pull request Jul 11, 2019

[nomerge] remove unused import. #8212

Merged

som-snytt restored the topic/imports-used branch June 26, 2020 20:49

som-snytt deleted the topic/imports-used branch December 19, 2020 21:13

Conversation

som-snytt commented Jun 15, 2018

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

som-snytt Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

som-snytt commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

som-snytt Jun 15, 2018 •

edited

Loading

som-snytt commented Jun 20, 2018 •

edited

Loading