Skip to content
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

Verify that lint is wconfable #10756

Open
wants to merge 2 commits into
base: 2.13.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/compiler/scala/tools/nsc/Reporting.scala
Expand Up @@ -624,6 +624,7 @@ object Reporting {
LintPerformance,
LintIntDivToFloat,
LintUniversalMethods,
LintCloneable,
LintNumericMethods
= lint()

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
Expand Up @@ -902,7 +902,7 @@ abstract class RefChecks extends Transform {
}
}
if (warnCloneable && baseClass.eq(JavaCloneableClass))
reporter.warning(clazz.pos, s"$clazz should not extend Cloneable.")
refchecksWarning(clazz.pos, s"$clazz should not extend Cloneable.", WarningCategory.LintCloneable)
val remaining = tp.parents.filterNot(seenParents)
seenParents ++= remaining
remaining.foreach(register)
Expand Down
96 changes: 64 additions & 32 deletions src/partest/scala/tools/partest/nest/Runner.scala
Expand Up @@ -78,8 +78,12 @@ class Runner(val testInfo: TestInfo, val suiteRunner: AbstractRunner) {

private val _transcript = new TestTranscript

// start log event
def pushTranscript(msg: String) = _transcript.add(msg)

// append to last log in transcript
def appendTranscript(log: String) = _transcript.append(log)

lazy val outDir = { outFile.mkdirs() ; outFile }

// if there is a checkfile, log message for diff; otherwise log stack trace for post-mortem
Expand Down Expand Up @@ -428,14 +432,10 @@ class Runner(val testInfo: TestInfo, val suiteRunner: AbstractRunner) {

def newCompiler = new DirectCompiler(this)

def attemptCompile(sources: List[File]): TestState = {
val badflags = (testFile :: (if (testFile.isDirectory) sources else Nil)).map(_.changeExtension("flags")).find(_.exists)
if (badflags.isDefined) genFail(s"unexpected flags file ${badflags.get} (use source comment // scalac: -Werror)")
else
newCompiler.compile(flagsForCompilation(sources), sources).tap { state =>
if (!state.isOk) _transcript.append("\n" + logFile.fileContents)
}
}
def attemptCompile(sources: List[File], extraFlags: List[String] = Nil): TestState =
newCompiler.compile(flagsForCompilation(sources) ::: extraFlags, sources).tap { state =>
if (!state.isOk) _transcript.append("\n" + logFile.fileContents)
som-snytt marked this conversation as resolved.
Show resolved Hide resolved
}

// all sources in a round may contribute flags via // scalac: -flags
// under --realeasy, if a javaVersion isn't specified, require the minimum viable using -release 8
Expand All @@ -455,42 +455,62 @@ class Runner(val testInfo: TestInfo, val suiteRunner: AbstractRunner) {

// for each file, cache the args for each tool
private val fileToolArgs = new mutable.HashMap[Path, Map[ToolName, List[String]]]
private val optionsPattern = raw"\s*//>\s*using\s+(?:([^.]+)\.)?option(s)?\s+(.*)".r

// Inspect given files for tool args in header line comments of the form `// tool: args`.
// If the line comment starts `//>`, accept `scalac:` options in the form of using pragmas.
// If the line comment starts `//>`, accept `using option` or `using options` pragmas
// to define options to`scalac`. Or take `using test.options`, where test scope is used for test options.
// (`test` scope is not used that way by scala-cli, where framework args are passed on command line.)
// (One could imagine `using test.testOpt` for framework args.)
// If `filter:`, return entire line as if quoted, else parse the args string as command line.
// Currently, we look for scalac, javac, java, javaVersion, filter, hide.
// Currently, we look for scalac, javac, java, javaVersion, filter, hide, test.
//
def toolArgsFor(files: List[File])(tool: ToolName): List[String] = {
def argsFor(f: File): List[String] = fileToolArgs.getOrElseUpdate(f.toPath, readToolArgs(f)).apply(tool)
def readToolArgs(f: File): Map[ToolName, List[String]] = {
val header = readHeaderFrom(f)
val options = optionsFromHeader(header)
if (options.isEmpty) oldStyle(header)
else options
}
def optionsFromHeader(header: List[String]) = {
import scala.sys.process.Parser.tokenize
def matchLine(line: String): List[(ToolName, List[String])] = line match {
case optionsPattern(scope, plural, rest) =>
val named = Try {
if (scope == null) ToolName.scalac
else ToolName.named(scope)
}.toOption
named match {
case None => Nil
case Some(name) =>
val settings =
if (plural == null) List(rest.trim)
else tokenize(rest).filter(_ != ",").map(_.stripSuffix(","))
if (settings.isEmpty) Nil
else (name, settings) :: Nil
}
case _ => Nil
}
header.flatMap(matchLine).toMap.withDefaultValue(List.empty[String])
}
def oldStyle(header: List[String]) =
ToolName.values.toList
.map(name => name -> fromHeader(name, header))
.filterNot(_._2.isEmpty)
.toMap[ToolName, List[String]]
.withDefaultValue(List.empty[String])
}
def fromHeader(name: ToolName, header: List[String]) = {
def fromHeader(name: ToolName, header: List[String]): List[String] = {
import scala.sys.process.Parser.tokenize
val namePattern = raw"\s*//\s*$name:\s*(.*)".r
val optionsPattern = raw"\s*//>\s*using\s+option(s)?\s+(.*)".r
def matchLine(line: String): List[String] =
line match {
case namePattern(rest) => if (name == ToolName.filter) List(rest.trim) else tokenize(rest)
case _ if name == ToolName.scalac =>
line match {
case optionsPattern(plural, rest) =>
if (plural == null) List(rest.trim)
else tokenize(rest).filter(_ != ",").map(_.stripSuffix(","))
case _ => Nil
}
case _ => Nil
}
def matchLine(line: String): List[String] = line match {
case namePattern(rest) => if (name == ToolName.filter) List(rest.trim) else tokenize(rest)
case _ => Nil
}
header.flatMap(matchLine)
}
def readHeaderFrom(f: File): List[String] =
Using.resource(Files.lines(f.toPath, codec.charSet))(stream => stream.limit(10).toArray()).toList.map(_.toString)
Using.resource(Files.lines(f.toPath, codec.charSet))(_.limit(10).toArray()).toList.map(_.toString)
files.flatMap(argsFor)
}

Expand Down Expand Up @@ -559,14 +579,25 @@ class Runner(val testInfo: TestInfo, val suiteRunner: AbstractRunner) {
else runTestCommon()()

def runNegTest(): TestState = {
// pass if it checks and didn't crash the compiler
// or, OK, we'll let you crash the compiler with a FatalError if you supply a check file
// a "crash test" passes if the error is not FatalError and there is a check file to compare.
// a neg test passes if the log compares same to check file.
// under "//> using test.option --recompile=-some-flags", also check compilation after adding the extra flags.
def checked(r: TestState) = r match {
case s: Skip => s
case crash @ Crash(_, t, _) if !checkFile.canRead || !t.isInstanceOf[FatalError] => crash
case _ => diffIsOk
case _ =>
val negRes = diffIsOk
toolArgs(ToolName.test) match {
case s :: Nil if s.startsWith("--recompile=") && !negRes.isSkipped && negRes.isOk =>
// assume -Werror and that testFile is a regular file; transcript visible under partest --verbose or failure
val extraFlags = List(s.stripPrefix("--recompile="))
val debug = s"recompile $testIdent with extra flags ${extraFlags.mkString(" ")}"
suiteRunner.verbose(s"% $debug")
pushTranscript(debug)
attemptCompile(List(testFile), extraFlags = extraFlags)
case _ => negRes
}
}

runTestCommon(checked, expectCompile = false)(identity)
}

Expand Down Expand Up @@ -786,7 +817,7 @@ final class TestTranscript {
def toList = buf.toList
}

// Tool names in test file header: scalac, javac, java, javaVersion, filter, hide.
// Tool names in test file header: scalac, javac, java, javaVersion, filter, hide, test.
sealed trait ToolName
object ToolName {
case object scalac extends ToolName
Expand All @@ -795,6 +826,7 @@ object ToolName {
case object javaVersion extends ToolName
case object filter extends ToolName
case object hide extends ToolName
val values = Array(scalac, javac, java, javaVersion, filter, hide)
case object test extends ToolName
val values = Array(scalac, javac, java, javaVersion, filter, hide, test)
def named(s: String): ToolName = values.find(_.toString.equalsIgnoreCase(s)).getOrElse(throw new IllegalArgumentException(s))
}
2 changes: 1 addition & 1 deletion test/files/neg/cloneable.check
@@ -1,4 +1,4 @@
cloneable.scala:6: warning: object X should not extend Cloneable.
cloneable.scala:7: warning: object X should not extend Cloneable.
object X extends Base
^
error: No warnings can be incurred under -Werror.
Expand Down
1 change: 1 addition & 0 deletions test/files/neg/cloneable.scala
@@ -1,5 +1,6 @@

//> using options -Werror -Xlint:cloneable
//> using test.options --recompile=-Wconf:cat=lint-cloneable:s
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about //> using retest.options -Wconf:cat=lint-cloneable:s. Leverage the "scope"; don't pollute semantics of test.options which I assume means compile options for tests, which is technically redundant here; also avoids the awkward --recompile; but the semantics of retest are constrained, as it doesn't mean "retest as neg" but "as pos", or maybe it would also invert pos to neg on retest.


class Base extends Cloneable

Expand Down