Warning! Warning! Yes, that's what's in this co...

Warning! Warning! Yes, that's what's in this commit. Why are you
panicking?

Mostly new command line options:

  -Xlint // basically, the ones which aren't noisy Ywarn-all
  -Ywarn-dead-code Ywarn-inaccessible // try this one on the library:
  -it makes some good points Ywarn-nullary-override Ywarn-nullary-unit
  -Ywarn-numeric-widen Ywarn-value-discard

Some accumulated motivations:

The wontfix resolution of ticket #4506 indicates that "def foo" and "def
foo()" are always going to be treated differently in some situations
and the same in others without users having any way to fix it. Summary
expressed in latest comment with which I agree (and quite sadly, given
that I've done a lot of work to try to make them usable) is "avoid using
structural types like the plague." But the least we can do is warn if
you're using parentheses "wrong".

I think it would be better if the warning about "def foo()" overriding
"def foo" were an error instead. If we have to live with this...

  trait Me { def f(): Int }
  class A { def f: Int = 5 }
  class C extends A with Me { }

  // error: Int does not take parameters
  def f(x: C) = x.f()

  // compiles
  def f(x: Me) = x.f()

  // error: Int does not take parameters. Mmph, how can a method be
  // legal with parameter "Foo" and illegal with parameter "Foo with
  // Bar" ?

  def f(x: Me with C) = x.f()

The warning about a method contains a reference to a type which is less
accessible than the method itself is obviously to those who recall it
a response to GenTraversable being private and appearing in flatMap's
signature during the 2.9.0 RCs. It avoids warning in the case where the
unnormalized type is inaccessible but the normalized version would be,
but it could use further refinement.

10 files changed, 164 insertions, 24 deletions

diff --git a/src/compiler/scala/tools/nsc/ast/TreeGen.scala b/src/compiler/scala/tools/nsc/ast/TreeGen.scala
index 177a89d9ae..ad9961fc49 100644
--- a/src/compiler/scala/tools/nsc/ast/TreeGen.scala
+++ b/src/compiler/scala/tools/nsc/ast/TreeGen.scala
@@ -24,7 +24,7 @@ abstract class TreeGen extends reflect.internal.TreeGen {
       if (tree.tpe != null || !tree.hasSymbol) tree.tpe
       else tree.symbol.tpe
 
-    if (!global.phase.erasedTypes && settings.Xchecknull.value &&
+    if (!global.phase.erasedTypes && settings.warnSelectNullable.value &&
         tpe <:< NotNullClass.tpe && !tpe.isNotNull)
       mkRuntimeCall(nme.checkInitialized, List(tree))
     else
diff --git a/src/compiler/scala/tools/nsc/reporters/AbstractReporter.scala b/src/compiler/scala/tools/nsc/reporters/AbstractReporter.scala
index 49d23b1ab3..5127c2eb19 100644
--- a/src/compiler/scala/tools/nsc/reporters/AbstractReporter.scala
+++ b/src/compiler/scala/tools/nsc/reporters/AbstractReporter.scala
@@ -31,7 +31,7 @@ abstract class AbstractReporter extends Reporter {
 
   protected def info0(pos: Position, msg: String, _severity: Severity, force: Boolean) {
     val severity =
-      if (settings.Xwarnfatal.value && _severity == WARNING) ERROR
+      if (settings.fatalWarnings.value && _severity == WARNING) ERROR
       else _severity
 
     if (severity == INFO) {
diff --git a/src/compiler/scala/tools/nsc/settings/AestheticSettings.scala b/src/compiler/scala/tools/nsc/settings/AestheticSettings.scala
index 0908ea60b6..136e03d9e2 100644
--- a/src/compiler/scala/tools/nsc/settings/AestheticSettings.scala
+++ b/src/compiler/scala/tools/nsc/settings/AestheticSettings.scala
@@ -22,7 +22,7 @@ trait AestheticSettings {
   def declsOnly       = false
   def deprecation     = settings.deprecation.value
   def experimental    = settings.Xexperimental.value
-  def fatalWarnings   = settings.Xwarnfatal.value
+  def fatalWarnings   = settings.fatalWarnings.value
   def logClasspath    = settings.Ylogcp.value
   def printStats      = settings.Ystatistics.value
   def richExes        = settings.YrichExes.value || sys.props.traceSourcePath.isSet
diff --git a/src/compiler/scala/tools/nsc/settings/MutableSettings.scala b/src/compiler/scala/tools/nsc/settings/MutableSettings.scala
index 65ef421b65..da66efe1fa 100644
--- a/src/compiler/scala/tools/nsc/settings/MutableSettings.scala
+++ b/src/compiler/scala/tools/nsc/settings/MutableSettings.scala
@@ -15,8 +15,11 @@ import scala.io.Source
 
 /** A mutable Settings object.
  */
-class MutableSettings(val errorFn: String => Unit) extends scala.reflect.internal.settings.MutableSettings
-                                                      with AbsSettings with ScalaSettings with Mutable {
+class MutableSettings(val errorFn: String => Unit)
+              extends scala.reflect.internal.settings.MutableSettings
+                 with AbsSettings
+                 with ScalaSettings
+                 with Mutable {
   type ResultOfTryToSet = List[String]
 
   /** Iterates over the arguments applying them to settings where applicable.
diff --git a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
index 70f6ea0b89..03e6e508f6 100644
--- a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
+++ b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
@@ -12,7 +12,9 @@ import annotation.elidable
 import scala.tools.util.PathResolver.Defaults
 import scala.collection.mutable.HashSet
 
-trait ScalaSettings extends AbsScalaSettings with StandardScalaSettings {
+trait ScalaSettings extends AbsScalaSettings
+                       with StandardScalaSettings
+                       with Warnings {
   self: MutableSettings =>
 
   import Defaults.scalaUserClassPath
@@ -79,9 +81,6 @@ trait ScalaSettings extends AbsScalaSettings with StandardScalaSettings {
   val showPhases    = BooleanSetting    ("-Xshow-phases", "Print a synopsis of compiler phases.")
   val sourceReader  = StringSetting     ("-Xsource-reader", "classname", "Specify a custom method for reading source files.", "")
 
-  val Xwarnfatal    = BooleanSetting    ("-Xfatal-warnings", "Fail the compilation if there are any warnings.")
-  val Xchecknull    = BooleanSetting    ("-Xcheck-null", "Emit warning on selection of nullable reference.")
-
   // Experimental Extensions
   val Xexperimental = BooleanSetting    ("-Xexperimental", "Enable experimental extensions.") .
                           withPostSetHook(set => List(YdepMethTpes, YmethodInfer) foreach (_.value = set.value)) //YvirtClasses,
@@ -163,11 +162,6 @@ trait ScalaSettings extends AbsScalaSettings with StandardScalaSettings {
   def stop = stopAfter
 
   /**
-   * Warnings
-   */
-  val Ywarndeadcode = BooleanSetting    ("-Ywarn-dead-code", "Emit warnings for dead code")
-
-  /**
    * IDE-specific settings
    */
   val YpresentationVerbose = BooleanSetting("-Ypresentation-verbose", "Print information about presentation compiler tasks.")
diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala
new file mode 100644
index 0000000000..6fbd9892c7
--- /dev/null
+++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala
@@ -0,0 +1,57 @@
+/* NSC -- new Scala compiler
+ * Copyright 2005-2011 LAMP/EPFL
+ * @author  Paul Phillips
+ */
+
+package scala.tools
+package nsc
+package settings
+
+import annotation.elidable
+import scala.tools.util.PathResolver.Defaults
+import scala.collection.mutable.HashSet
+
+/** Settings influencing the printing of warnings.
+ */
+trait Warnings {
+  self: MutableSettings =>
+
+  // Warning semantics.
+  val fatalWarnings = BooleanSetting("-Xfatal-warnings", "Fail the compilation if there are any warnings.")
+
+  // These warnings are all so noisy as to be useless in their
+  // present form, but have the potential to offer useful info.
+  protected def allWarnings = lintWarnings ++ List(
+    warnSelectNullable,
+    warnValueDiscard,
+    warnNumericWiden
+  )
+  // These warnings should be pretty quiet unless you're doing
+  // something inadvisable.
+  protected def lintWarnings = List(
+    warnDeadCode,
+    warnInaccessible,
+    warnNullaryOverride,
+    warnNullaryUnit
+  )
+
+  // Warning groups.
+  val lint = (
+    BooleanSetting("-Xlint", "Enable recommended additional warnings.")
+    withPostSetHook (_ => lintWarnings foreach (_.value = true))
+  )
+  val warnEverything = (
+    BooleanSetting("-Ywarn-all", "Enable all -Y warnings.")
+    withPostSetHook (_ => lintWarnings foreach (_.value = true))
+  )
+
+  // Individual warnings.
+  val warnSelectNullable   = BooleanSetting   ("-Xcheck-null", "Warn upon selection of nullable reference.")
+  val warnDeadCode         = BooleanSetting   ("-Ywarn-dead-code", "Warn when dead code is identified.")
+  val warnValueDiscard     = BooleanSetting   ("-Ywarn-value-discard", "Warn when non-Unit expression results are unused.")
+  val warnNumericWiden     = BooleanSetting   ("-Ywarn-numeric-widen", "Warn when numerics are widened.")
+  val warnNullaryUnit      = BooleanSetting   ("-Ywarn-nullary-unit", "Warn when nullary methods return Unit.")
+  val warnInaccessible     = BooleanSetting   ("-Ywarn-inaccessible", "Warn about inaccessible types in method signatures.")
+  val warnNullaryOverride  = BooleanSetting   ("-Ywarn-nullary-override",
+    "Warn when non-nullary overrides nullary, e.g. `def foo()` over `def foo`.")
+}
diff --git a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
index 83b6168c03..49b67bb29f 100644
--- a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
+++ b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
@@ -98,7 +98,7 @@ abstract class SymbolLoaders {
     private var ok = false
 
     private def setSource(sym: Symbol) {
-      sourcefile map (sf => sym match {
+      sourcefile foreach (sf => sym match {
         case cls: ClassSymbol => cls.sourceFile = sf
         case mod: ModuleSymbol => mod.moduleClass.sourceFile = sf
         case _ => ()
diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index d7aa7bc527..d4ec27a6c0 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -75,6 +75,11 @@ abstract class RefChecks extends InfoTransform {
     }
   }
 
+  def accessFlagsToString(sym: Symbol) = flagsToString(
+    sym getFlag (PRIVATE | PROTECTED),
+    if (sym.hasAccessBoundary) "" + sym.privateWithin.name else ""
+  )
+
   class RefCheckTransformer(unit: CompilationUnit) extends Transformer {
 
     var localTyper: analyzer.Typer = typer;
@@ -272,11 +277,6 @@ abstract class RefChecks extends InfoTransform {
           }
         }
 
-        def accessFlagsToString(sym: Symbol) = flagsToString(
-          sym getFlag (PRIVATE | PROTECTED),
-          if (sym.hasAccessBoundary) "" + sym.privateWithin.name else ""
-        )
-
         def overrideAccessError() {
           val otherAccess = accessFlagsToString(other)
           overrideError("has weaker access privileges; it should be "+ (if (otherAccess == "") "public" else "at least "+otherAccess))
@@ -356,6 +356,11 @@ abstract class RefChecks extends InfoTransform {
             overrideError("must be declared lazy to override a concrete lazy value")
           } else {
             checkOverrideTypes()
+            if (settings.warnNullaryOverride.value) {
+              if (other.paramss.isEmpty && !member.paramss.isEmpty) {
+                unit.warning(member.pos, "non-nullary method overrides nullary method")
+              }
+            }
           }
         }
 
@@ -502,6 +507,7 @@ abstract class RefChecks extends InfoTransform {
               )
             }
             else if (underlying.isMethod) {
+
               // If there is a concrete method whose name matches the unimplemented
               // abstract method, and a cursory examination of the difference reveals
               // something obvious to us, let's make it more obvious to them.
@@ -1173,6 +1179,60 @@ abstract class RefChecks extends InfoTransform {
         unit.warning(pos, sym.fullLocationString + " has changed semantics:\n" + msg)
     }
 
+    private def lessAccessible(otherSym: Symbol, memberSym: Symbol): Boolean = (
+         (otherSym != NoSymbol)
+      && !otherSym.isTypeParameterOrSkolem
+      && !otherSym.isExistentiallyBound
+      && (otherSym isLessAccessibleThan memberSym)
+      && (otherSym isLessAccessibleThan memberSym.enclClass)
+    )
+    private def lessAccessibleSymsInType(other: Type, memberSym: Symbol): List[Symbol] = {
+      val extras = other match {
+        case TypeRef(pre, _, args) =>
+          // checking the prefix here gives us spurious errors on e.g. a private[process]
+          // object which contains a type alias, which normalizes to a visible type.
+          args filterNot (_ eq NoPrefix) flatMap (tp => lessAccessibleSymsInType(tp, memberSym))
+        case _ =>
+          Nil
+      }
+      if (lessAccessible(other.typeSymbol, memberSym)) other.typeSymbol :: extras
+      else extras
+    }
+    private def warnLessAccessible(otherSym: Symbol, memberSym: Symbol) {
+      val comparison = accessFlagsToString(memberSym) match {
+        case ""   => ""
+        case acc  => " is " + acc + " but"
+      }
+      val cannot =
+        if (memberSym.isDeferred) "may be unable to provide a concrete implementation of"
+        else "may be unable to override"
+
+      unit.warning(memberSym.pos,
+        "%s%s references %s %s.".format(
+          memberSym.fullLocationString, comparison,
+          accessFlagsToString(otherSym), otherSym
+        ) + "\nClasses which cannot access %s %s %s.".format(
+          otherSym.decodedName, cannot, memberSym.decodedName)
+      )
+    }
+    /** Warn about situations where a method signature will include a type which
+     *  has more restrictive access than the method itself.
+     */
+    private def checkAccessibilityOfReferencedTypes(tree: Tree) {
+      val member = tree.symbol
+
+      // types of the value parameters
+      member.paramss.flatten foreach { p =>
+        val normalized = p.tpe.normalize
+        if ((normalized ne p.tpe) && lessAccessibleSymsInType(normalized, member).isEmpty) ()
+        else lessAccessibleSymsInType(p.tpe, member) foreach (sym => warnLessAccessible(sym, member))
+      }
+      // upper bounds of type parameters
+      member.typeParams.map(_.info.bounds.hi.widen) foreach { tp =>
+        lessAccessibleSymsInType(tp, member) foreach (sym => warnLessAccessible(sym, member))
+      }
+    }
+
     /** Check that a deprecated val or def does not override a
       * concrete, non-deprecated method.  If it does, then
       * deprecation is meaningless.
@@ -1197,6 +1257,7 @@ abstract class RefChecks extends InfoTransform {
       case _ =>
         false
     }
+
     private def checkTypeRef(tp: Type, pos: Position) = tp match {
       case TypeRef(pre, sym, args) =>
         checkDeprecated(sym, pos)
@@ -1335,6 +1396,19 @@ abstract class RefChecks extends InfoTransform {
       }
     }
 
+    // Warning about nullary methods returning Unit.
+    private def checkNullaryMethodReturnType(sym: Symbol) = sym.tpe match {
+      case NullaryMethodType(restpe) if restpe.typeSymbol == UnitClass =>
+        // this may be the implementation of e.g. a generic method being parameterized
+        // on Unit, in which case we had better let it slide.
+        if (sym.isGetter || sym.allOverriddenSymbols.exists(over => !(over.tpe.resultType =:= sym.tpe.resultType))) ()
+        else unit.warning(sym.pos,
+          "side-effecting nullary methods are discouraged: suggest defining as `def %s()` instead".format(
+           sym.name.decode)
+        )
+        case _ => ()
+    }
+
     override def transform(tree: Tree): Tree = {
       val savedLocalTyper = localTyper
       val savedCurrentApplication = currentApplication
@@ -1355,6 +1429,12 @@ abstract class RefChecks extends InfoTransform {
 
           case ValDef(_, _, _, _) | DefDef(_, _, _, _, _, _) =>
             checkDeprecatedOvers(tree)
+            if (settings.warnNullaryUnit.value)
+              checkNullaryMethodReturnType(sym)
+            if (settings.warnInaccessible.value) {
+              if (!sym.isConstructor && !sym.isEffectivelyFinal && !sym.isSynthetic)
+                checkAccessibilityOfReferencedTypes(tree)
+            }
             tree
 
           case Template(parents, self, body) =>
diff --git a/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala b/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala
index 2abecfa572..45663c8bee 100644
--- a/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala
@@ -421,7 +421,7 @@ trait TypeDiagnostics {
         // Error suppression will squash some of these warnings unless we circumvent it.
         // It is presumed if you are using a -Y option you would really like to hear
         // the warnings you've requested.
-        if (settings.Ywarndeadcode.value && context.unit != null && treeOK(tree) && exprOK) {
+        if (settings.warnDeadCode.value && context.unit != null && treeOK(tree) && exprOK) {
           val saved = context.reportGeneralErrors
           try {
             context.reportGeneralErrors = true
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index a470e80449..ba965290fe 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -892,10 +892,16 @@ trait Typers extends Modes {
                 case TypeRef(_, sym, _) =>
                   // note: was if (pt.typeSymbol == UnitClass) but this leads to a potentially
                   // infinite expansion if pt is constant type ()
-                  if (sym == UnitClass && tree.tpe <:< AnyClass.tpe) // (12)
+                  if (sym == UnitClass && tree.tpe <:< AnyClass.tpe) { // (12)
+                    if (settings.warnValueDiscard.value)
+                      context.unit.warning(tree.pos, "discarded non-Unit value")
                     return typed(atPos(tree.pos)(Block(List(tree), Literal(()))), mode, pt)
-                  else if (isNumericValueClass(sym) && isNumericSubType(tree.tpe, pt))
+                  }
+                  else if (isNumericValueClass(sym) && isNumericSubType(tree.tpe, pt)) {
+                    if (settings.warnNumericWiden.value)
+                      context.unit.warning(tree.pos, "implicit numeric widening")
                     return typed(atPos(tree.pos)(Select(tree, "to"+sym.name)), mode, pt)
+                  }
                 case AnnotatedType(_, _, _) if canAdaptAnnotations(tree, mode, pt) => // (13)
                     return typed(adaptAnnotations(tree, mode, pt), mode, pt)
                 case _ =>
@@ -3616,7 +3622,7 @@ trait Typers extends Modes {
             !(List(Any_isInstanceOf, Any_asInstanceOf) contains result.symbol)  // null.is/as is not a dereference
           }
           // unit is null here sometimes; how are we to know when unit might be null? (See bug #2467.)
-          if (settings.Xchecknull.value && isPotentialNullDeference && unit != null)
+          if (settings.warnSelectNullable.value && isPotentialNullDeference && unit != null)
             unit.warning(tree.pos, "potential null pointer dereference: "+tree)
 
           val selection = result match {