[REVERTED] Add infer Product with Serializable linter flag #5990

jvz · 2017-07-10T20:38:42Z

This adds a new lint warning, -Xlint:infer-product-with-serializable
which warns when an inferred value or method type is Product with
Serializable. This is oftentimes a programmer error from mixing up what
would otherwise be incompatible types (which could be caught by the
-Xlint:infer-any flag otherwise) that have the common PwS supertype. As
this inferred type tends to be useless, this lint flag helps avoid
errors related to this inference.

Requesting review by @adriaanm in particular as this seems related to his expertise.

hrhino

Glad you got this going!

hrhino · 2017-07-11T00:29:04Z

src/compiler/scala/tools/nsc/settings/Warnings.scala

@@ -74,6 +74,7 @@ trait Warnings {
    val Inaccessible           = LintWarning("inaccessible",              "Warn about inaccessible types in method signatures.",                       true)
    val NullaryOverride        = LintWarning("nullary-override",          "Warn when non-nullary `def f()' overrides nullary `def f'.",                true)
    val InferAny               = LintWarning("infer-any",                 "Warn when a type argument is inferred to be `Any`.",                        true)
+    val InferProductWithSerializable = LintWarning("infer-product-with-serializable", "Warn when a type argument is inferred to be `Product with Serializable`.")


This does not do what it says it does.

> scala -Xlint:infer-any -Xlint:infer-product-with-serializable [info] Running scala.tools.nsc.MainGenericRunner -usejavacp -Xlint:infer-any -Xlint:infer-product-with-serializable Welcome to Scala 2.12.3-20170710-215443-d31a518 (OpenJDK 64-Bit Server VM, Java 1.8.0_131). Type in expressions for evaluation. Or try :help. scala> def f[A](as: A*) = 12 f: [A](as: A*)Int scala> f(1, "2") <console>:13: warning: a type was inferred to be `Any`; this may indicate a programming error. f(1, "2") ^ res0: Int = 12 scala> f((1,2), List(3, 4)) res1: Int = 12 scala>

Instead it does (as the test cases say):

scala> val f = if (true) (1,2) else List(3, 4) <console>:11: warning: a type was inferred to be `Product with Serializable`; this may indicate a programming error val f = if (true) (1,2) else List(3, 4) ^ f: Product with Serializable = (1,2) scala>

which is 👍 👍, but personally I think it should do both of the above.

Ah yes, the problems of copying semi-related examples. I need to update the docs as well. Adding inference checks for parameters makes sense.

hrhino · 2017-07-11T00:29:47Z

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

+      // if enabled, validate that the now inferred val or def type isn't PwS
+      if (settings.warnInferProductWithSerializable && context.reportErrors) {
+        tpe match {
+          case RefinedType(ProductRootTpe :: SerializableTpe :: _, _) =>


Possibly check that scope.isEmpty here? I'm unsure how it would really make a difference, though.

I'm kind of unclear of the purpose there.

RefinedType is defined as case class RefinedType(override val parents: List[Type], override val decls: Scope), you could check that decls is empty.

Almost forgot to fix this check. Updated as well.

hrhino · 2017-07-11T00:30:50Z

src/reflect/scala/reflect/internal/Definitions.scala

@@ -712,6 +712,7 @@ trait Definitions extends api.StandardDefinitions {
    def tupleComponents(tp: Type)      = tp.dealiasWiden.typeArgs

    lazy val ProductRootClass: ClassSymbol = requiredClass[scala.Product]
+    lazy val ProductRootTpe: Type          = typeOf[scala.Product]


ProductRootClass.tpe seems to be more in line with what is done elsewhere in this file.

Makes sense to me!

hrhino · 2017-07-11T00:32:38Z

Also, having changed Definitions.scala, you now are morally obligated by the tests to take a trip to JavaUniverseForce.scala and spread the good word of ProductRootTpe ;)

jvz · 2017-07-11T16:55:03Z

Alright, I've updated the docs, added parameter checking, did a little refactoring for infer-any to support PwS in the same way, added the type to JavaUniverseForce, added more tests for parameter inference, and checked for an empty scope. I think that about covers all the current comments.

lrytz

very nice work!

lrytz · 2017-07-12T11:27:10Z

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

+          case _ =>
+        }
+      }
+      tpe


Do we still need the check here?

The Namer check is for vals and defs, while the Infer check is for generic parameters. I can't find a suitable place in the compiler that would work for both situations without any duplication unfortunately, though that's mainly due to my unfamiliarity with the codebase.

Ah, I see. Would it make sense to add the infer-any check here as well?

That would be handy feature improvement. I'll go ahead and add it in.

lrytz · 2017-07-12T11:28:11Z

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

-          case _ =>
-        })
+      // Ditto for Product with Serializable
+      def canWarnAboutPwS = canWarnAbout(tps => (tps exists (_ contains ProductRootClass)) && (tps exists (_ contains SerializableClass)))


could you write a few more tests that exploit this path?

I can add some more that are similar to the infer-any checks, though I'm not as clear about how this parameter situation arises compared to vals and defs.

szeiger · 2017-07-12T14:36:00Z

Is it clear that the type will always be Product with Serializable rather than Serializable with Product?

jvz · 2017-07-12T14:37:47Z

The typically inferred type is fully Product with Serializable with Object, though I'm not sure what logic dictates the ordering. If it makes sense, I could add a check for RefinedType(SerializableTpe :: ProductRootTpe :: _, _) as well.

Also, I've pushed an update to the partest. I've added a couple more tests there.

som-snytt · 2017-07-12T17:44:07Z

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

+      if (settings.warnInferAny && canWarnAboutAny) {
+        warnIfInferred {
+          _.typeSymbol match {
+            case (AnyClass | AnyValClass) => true


The parens are extra.

som-snytt · 2017-07-12T17:50:26Z

Dangling issue for improvement in this area scala/bug#9211

I remember there was once a partest that occasionally printed Serializable with Product.

jvz · 2017-07-13T14:35:44Z

I've been unable to find a situation to cause the reverse ordering of types like that, but I can change the RefinedType check to look for the opposite ordering. I'm not confident that changing them to check for any location besides the first two types would be appropriate, but I could also go that route if there aren't any issues.

lrytz · 2017-07-14T15:19:41Z

Maybe you can also figure out why the following doesn't warn:

scala> def f(x: Boolean) = List(if(x) (1,2) else Nil)
f: (x: Boolean)List[Product with Serializable]

The same is true for infer-any

szeiger · 2017-07-14T15:20:28Z

AFAIR we had a reversed order at some point during the migration from ant to sbt. I don't remember the exact reason, probably a change in the classpath.

jvz · 2017-07-14T15:32:02Z

I've pushed an update that adds val and def checks for Any and AnyVal (along with neg and neg-neg test updates) along with updates to the command line docs.

som-snytt · 2017-07-14T21:46:29Z

src/compiler/scala/tools/nsc/settings/Warnings.scala

@@ -97,6 +98,7 @@ trait Warnings {
  def warnInaccessible           = lint contains Inaccessible
  def warnNullaryOverride        = lint contains NullaryOverride
  def warnInferAny               = lint contains InferAny
+  def warnInferProductWithSerializable = lint contains InferProductWithSerializable


I wouldn't be averse to using warnInferPwS everywhere. Then you preserve alignment, which is king.

Updated to use the PwS acronym for brevity.

som-snytt · 2017-07-14T21:52:12Z

I was surprised the following doesn't currently warn, and I guess still won't.

scala> def f[A](a: A) = 42
f: [A](a: A)Int

scala> f { if (true) 17 else "" } //print
   $line3.$read.$iw.$iw.f[Any](if (true)
  17
else
  "") // : Int

scala> f { if (true) 17 else "" } //print
res0: Int = 42

This adds a new lint warning, -Xlint:infer-pws which warns when an inferred value or method type is Product with Serializable. This is oftentimes a programmer error from mixing up what would otherwise be incompatible types (which could be caught by the -Xlint:infer-any flag otherwise) that have the common PwS supertype. As this inferred type tends to be useless, this lint flag helps avoid errors related to this inference. In addition, this also extends the same val and def inference check to -Xlint:infer-any to make it consistent with the new linter setting. Variable and method definitions that are inferred to be Any or AnyVal will also add a warning unless they have explicit types ascribed.

jvz · 2017-07-24T16:33:48Z

Is there anything else you all would like me to change in this PR? Or can it be merged for 2.12.4?

adriaanm · 2017-09-25T18:59:06Z

Reviewers, could one of you add the "approved" stamp?

som-snytt · 2017-09-25T21:56:23Z

LGTM

in case that's still the stamp of approval.

adriaanm · 2017-09-25T22:05:37Z

The newfangled github stamp is the preferred indicator these days, but it's good to see scabot can still apply reviewed labels.

som-snytt · 2017-09-25T22:10:40Z

Maybe there's a difference between "leaving review comments" (one-offs) vs "starting a review" where the full review has the approve button?

IATM like the politicians said in ads, "I approved this ~~message~~ modification."

jvz · 2017-09-26T01:56:45Z

Thanks for the reviews!

SethTisue · 2017-09-26T18:02:15Z

under -Xlint this didn't used to warn:

trait T {
  def thunk(): Any
  def value = thunk()
}

but since this PR was merged (at least, I'm assuming this is the PR responsible, I didn't bisect), it gives:

[warn] /Users/tisue/tmp/any-warning/S.scala:3:7: a type was inferred to be `Any`; this may indicate a programming error
[warn]   def value = thunk()
[warn]       ^

this was caught by the Scala 2.12 community build; the new warning causes several projects with -Xfatal-warnings enabled to fail compilation

SethTisue · 2017-09-26T18:07:56Z

ah, I guess this is "adds val and def checks for Any and AnyVal"

SethTisue · 2017-09-26T18:24:01Z

@jvz can you look through the new warnings in https://scala-ci.typesafe.com/view/All/job/scala-2.12.x-integrate-community-build/1992/consoleFull and make sure that they are actually intended...? we can always temporarily disable -Xfatal-warnings for the failing projects in the context of the community build, figuring the project owners will address the warnings after 2.12.4 come out

SethTisue · 2017-09-26T18:26:12Z

(there is a larger discussion to be had... not here on this PR... about how much, and what kind, of linting it's appropriate to keep adding to scalac. it would be really nice if linting were its own phase (or perhaps a compiler plugin) and were Scalafix based, rather than just inserted into compiler code everywhere)

som-snytt · 2017-09-26T22:04:19Z

It's like having to write case _: Throwable, which isn't even lint.

I can attest to the great head ache of wading through community build hiccups. It's like walking on gravel in thin-soled shoes.

Was the contributor sufficiently warned? Besides the CLA, there ought to be a release form you have to fill out.

som-snytt · 2017-09-26T22:12:28Z

The better-files warning is a style challenge. It needs tpolecat's void.

  def apply[A](disposeMethod: A => Any): Disposable[A] = new Disposable[A] {
    override def dispose(resource: A) = {
      val _ = disposeMethod(resource)
    }
  }

som-snytt · 2017-09-26T22:16:45Z

Bookmarking:

[fs2] [error] /home/jenkins/workspace/scala-2.12.x-integrate-community-build/target-0.9.9/project-builds/fs2-9cd5cb7138c541c306011296c1d74d4e95332a41/core/shared/src/main/scala/fs2/AsyncPull.scala:96: a type was inferred to be `Product with Serializable with fs2.internal.FreeC[F,A]`; this may indicate a programming error.
[fs2] [error]       def get = FreeC.Eval(r.get).flatMap(_.fold(FreeC.Fail(_), FreeC.Pure(_)))
[fs2] [error]                                             ^

I thought something like that came in up the discussion?

jvz · 2017-09-27T01:06:02Z

a type was inferred to be Product with Serializable with fs2.internal.FreeC[F,A]

This doesn't sound correct. My check is supposed to only be finding things with just the first two types, and it wouldn't really make sense in that example to declare a type. The discussion did bring up the question about what if a type was inferred to be Serializable with Product instead of the other way around, though I never found that to be the case.

@SethTisue as for the thunk one, that looks like the appropriate warning being detected, though it does seem to be a bit overzealous in that you already do have an explicit type for Any above it. As it is, though, I do prefer to be explicit about types in public functions unless it's extremely trivial (like string constants).

Also, as to adding linter checks to the compiler, yes, I'd rather see it in a separate tool in the future that would not only be easier to modify (scalac's source code is hard to follow), though when I put this PR together, I wasn't really aware of scalacheck, wartremover, et al. Perhaps this would be a nice focus in Dotty?

jvz · 2017-09-27T01:07:45Z

Oh, and that check for the inferred Any, I believe I added that after the PwS check. See the review comments by @lrytz

adriaanm · 2017-10-01T03:02:26Z

I'm sorry we had to revert this (see scala/bug#10535), but the release deadline is too close to refine this in time for 2.12.4. I would love to get a refinement into 2.12.5!

scala-jenkins added this to the 2.12.4 milestone Jul 10, 2017

jvz force-pushed the feature/infer-pws branch from 77c46b2 to d31a518 Compare July 10, 2017 21:55

hrhino reviewed Jul 11, 2017

View reviewed changes

jvz force-pushed the feature/infer-pws branch from d31a518 to b78a4a9 Compare July 11, 2017 16:53

lrytz reviewed Jul 12, 2017

View reviewed changes

jvz force-pushed the feature/infer-pws branch 2 times, most recently from a861726 to 4f3c783 Compare July 12, 2017 14:42

som-snytt reviewed Jul 12, 2017

View reviewed changes

jvz force-pushed the feature/infer-pws branch from 4f3c783 to 10cce61 Compare July 12, 2017 18:15

jvz force-pushed the feature/infer-pws branch from 10cce61 to a68b9b3 Compare July 14, 2017 15:30

som-snytt reviewed Jul 14, 2017

View reviewed changes

jvz force-pushed the feature/infer-pws branch from a68b9b3 to 6827c2a Compare July 17, 2017 15:41

adriaanm added the release-notes worth highlighting in next release notes label Sep 21, 2017

scala-jenkins added the reviewed label Sep 25, 2017

adriaanm merged commit dd162ea into scala:2.12.x Sep 25, 2017

jvz deleted the feature/infer-pws branch September 26, 2017 01:56

adriaanm mentioned this pull request Sep 30, 2017

Revert "Add infer Product with Serializable linter flag" #6106

Merged

SethTisue removed the release-notes worth highlighting in next release notes label Oct 18, 2017

SethTisue removed this from the 2.12.4 milestone Oct 18, 2017

SethTisue changed the title ~~Add infer Product with Serializable linter flag~~ [REVERTED] Add infer Product with Serializable linter flag May 21, 2018

som-snytt mentioned this pull request May 9, 2023

Serializable limits the helpfulness of -Xlint:infer-any scala/bug#12784

Open

[REVERTED] Add infer Product with Serializable linter flag #5990

[REVERTED] Add infer Product with Serializable linter flag #5990

Uh oh!

Conversation

jvz commented Jul 10, 2017

Uh oh!

hrhino 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

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!

hrhino commented Jul 11, 2017

Uh oh!

jvz commented Jul 11, 2017

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeiger commented Jul 12, 2017

Uh oh!

jvz commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

som-snytt commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvz commented Jul 13, 2017

Uh oh!

lrytz commented Jul 14, 2017

Uh oh!

szeiger commented Jul 14, 2017

Uh oh!

jvz commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

som-snytt commented Jul 14, 2017

Uh oh!

jvz commented Jul 24, 2017

Uh oh!

adriaanm commented Sep 25, 2017

Uh oh!

som-snytt commented Sep 25, 2017

Uh oh!

adriaanm commented Sep 25, 2017

Uh oh!

som-snytt commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Jul 12, 2017 •

edited

Loading

som-snytt commented Sep 25, 2017 •

edited

Loading