From fdfdd09d51f5ad3ee30e525145001f02959e3899 Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Wed, 29 Jun 2011 16:59:10 +0000 Subject: 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. --- src/compiler/scala/tools/nsc/ast/TreeGen.scala | 2 +- .../tools/nsc/reporters/AbstractReporter.scala | 2 +- .../tools/nsc/settings/AestheticSettings.scala | 2 +- .../scala/tools/nsc/settings/MutableSettings.scala | 7 +- .../scala/tools/nsc/settings/ScalaSettings.scala | 12 +-- .../scala/tools/nsc/settings/Warnings.scala | 57 ++++++++++++++ .../scala/tools/nsc/symtab/SymbolLoaders.scala | 2 +- .../scala/tools/nsc/typechecker/RefChecks.scala | 90 ++++++++++++++++++++-- .../tools/nsc/typechecker/TypeDiagnostics.scala | 2 +- .../scala/tools/nsc/typechecker/Typers.scala | 12 ++- 10 files changed, 164 insertions(+), 24 deletions(-) create mode 100644 src/compiler/scala/tools/nsc/settings/Warnings.scala (limited to 'src/compiler/scala/tools/nsc') 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, @@ -162,11 +161,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 */ 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 { -- cgit v1.2.3