Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

SI-5905 Sanity check -language options #3597

Merged
merged 3 commits into from

3 participants

@som-snytt

The option names are hardcoded, but checked by a test.

There are no hooks to verify options after the compiler
is constructed.

Introduced a MultiChoiceSetting required for the
setting creation framework.

@som-snytt som-snytt SI-5905 Sanity check -language options
The option names are hardcoded, but checked by a test.

There are no hooks to verify options after the compiler
is constructed.

Introduced a `MultiChoiceSetting` required for the
setting creation framework.
a40af3e
@som-snytt som-snytt added tested and removed needs-attention labels
@som-snytt som-snytt closed this
@som-snytt

Closed until there's way to propose a PR that's not RC.

@som-snytt som-snytt reopened this
@som-snytt som-snytt added tested and removed tested labels
@som-snytt som-snytt closed this
@som-snytt

Reopening for tweaked underscore handling.

@som-snytt som-snytt reopened this
@som-snytt som-snytt added tested and removed tested labels
@som-snytt som-snytt SI-5905 Restore -language:_
Underscore means all.

-x:c,b,a,_ results in value c,b,a,a,b,c,d,...

Currently, -Xprint does not present phases as a closed
set of choices; there is ad hoc checking in Global.
That would be a nice unification. (You don't know the
list of choices until after global is constructed.)
534cadc
@som-snytt som-snytt added tested and removed tested labels
@som-snytt

Suspended animation.

@som-snytt som-snytt closed this
@som-snytt

Reanimating.

@som-snytt som-snytt reopened this
@som-snytt

I see from another PR that there's an on-hold label used to park PRs? That would be a PRK.

@som-snytt

@adriaanm I know it's not your job anymore, but should I ping someone before I have to rebase? Pretty soon it will be summer vacation and all of Europe will be hiking in the Alps.

@adriaanm
Owner

I'm back -- in the Olde Country safe haven. Taking a few more days to adapt to the air, but then I'll be back to terrorizing the PR queue. @retronym is tending to our branches. I believe this one is good to go for review & inclusion in 2.11.1.

test/files/run/t5905-features.scala
@@ -0,0 +1,29 @@
+
+import tools.partest.DirectTest
+
+// verify that all languageFeature names are accepted by -language
+object Test extends DirectTest {
+ override def code = "class Code { def f = (1 to 10) size }" // exercise a feature
@retronym Owner
retronym added a note

I don't understand the need for this.

The following variation of this test passes without this.

import tools.partest.DirectTest

// verify that all languageFeature names are accepted by -language
object Test extends DirectTest {
  override def code = ""

  override def extraSettings = s"-usejavacp -d ${testOutput.path}"

  override def show() = {
    val global = newCompiler("")
    new global.Run()
    import global._, global.definitions._

    exitingTyper {
      def isFeature(s: Symbol) = s hasAnnotation LanguageFeatureAnnot
      val langf  = languageFeatureModule.typeSignature
      val feats  = langf.declarations filter (s => isFeature(s)) map (_.name.decoded)
      val xmen   = langf.member(TermName("experimental")).typeSignature.declarations filter (s =>
         isFeature(s)) map (s => s"experimental.${s.name.decoded}"
      )
      val all    = (feats ++ xmen) mkString ","

      assert(feats.nonEmpty, "Test must find feature flags.")

      //dynamics,postfixOps,reflectiveCalls,implicitConversions,higherKinds,existentials,experimental.macros
      compile(s"-language:$all")
    }
  }
}

Searching my memory with async i/o, so while I'm waiting for that result, my guess is that I wanted it to emit a warning if the feature flag was wrongly omitted from the test. Properly, it would exercise all features and ensure no warnings, including no warning about bad flags.

No wait, I didn't read "after the jump." I'll check on it and amend with corrections and explanatory comments.

Rhetorical edit: was I just being paranoid about global init?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@retronym
Owner

Can I buy an "S" for the title of the latest commit?

@som-snytt som-snytt SI-5905 Clarify test case
The language feature options are discovered reflectively, but it
is nice to enforce that expected options are supplied.

Short of that, the code string includes a rowdy postfix operator.

It still does enforce that at least one option was discovered.

Delete -nowarn flags file. Let's see if that was to suppress
a warning in the standard build.
78bd175
@som-snytt

orry about that.

Grabbing text with the mouse in a terminal window is often off by one for me. I can't get the hang of these newfangled UIs.

@adriaanm
Owner

LGTM

@retronym retronym merged commit 6f33b24 into scala:master
@adriaanm
Owner

somehow this commit snuck past PR validation, but not past scala-checkin: https://scala-webapps.epfl.ch/jenkins/job/scala-checkin/8994/consoleFull /cc @retronym, @lrytz

Owner

I'll submit a fix.

Owner

thanks

Let's see if that was to suppress a warning in the standard build.

I guess that would be a yes.

@retronym retronym referenced this pull request from a commit in retronym/scala
@retronym retronym Revert "SI-5905 Clarify test case"
This reverts commit 78bd175.

See discussion:

  scala#3597 (comment)
bdb1258
@retronym
Owner

I've submitted a PR to revert the last commit here: https://github.com/scala/scala/pull/3732/files

@som-snytt

OK, shall I submit the corrected commit against master or 2.11? I didn't notice it was merged...

@retronym
Owner

I just submitted a reversion against master. That will be merged to 2.11.x in an hour or two. You should target the re-refinement against 2.11.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 28, 2014
  1. @som-snytt

    SI-5905 Sanity check -language options

    som-snytt authored
    The option names are hardcoded, but checked by a test.
    
    There are no hooks to verify options after the compiler
    is constructed.
    
    Introduced a `MultiChoiceSetting` required for the
    setting creation framework.
Commits on Mar 3, 2014
  1. @som-snytt

    SI-5905 Restore -language:_

    som-snytt authored
    Underscore means all.
    
    -x:c,b,a,_ results in value c,b,a,a,b,c,d,...
    
    Currently, -Xprint does not present phases as a closed
    set of choices; there is ad hoc checking in Global.
    That would be a nice unification. (You don't know the
    list of choices until after global is constructed.)
Commits on May 8, 2014
  1. @som-snytt

    SI-5905 Clarify test case

    som-snytt authored
    The language feature options are discovered reflectively, but it
    is nice to enforce that expected options are supplied.
    
    Short of that, the code string includes a rowdy postfix operator.
    
    It still does enforce that at least one option was discovered.
    
    Delete -nowarn flags file. Let's see if that was to suppress
    a warning in the standard build.
This page is out of date. Refresh to see the latest.
View
2  src/compiler/scala/tools/nsc/settings/AbsScalaSettings.scala
@@ -16,6 +16,7 @@ trait AbsScalaSettings {
type ChoiceSetting <: Setting { type T = String }
type IntSetting <: Setting { type T = Int }
type MultiStringSetting <: Setting { type T = List[String] }
+ type MultiChoiceSetting <: Setting { type T = List[String] }
type PathSetting <: Setting { type T = String }
type PhasesSetting <: Setting { type T = List[String] }
type StringSetting <: Setting { type T = String }
@@ -28,6 +29,7 @@ trait AbsScalaSettings {
def ChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String], default: String): ChoiceSetting
def IntSetting(name: String, descr: String, default: Int, range: Option[(Int, Int)], parser: String => Option[Int]): IntSetting
def MultiStringSetting(name: String, helpArg: String, descr: String): MultiStringSetting
+ def MultiChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String]): MultiChoiceSetting
def OutputSetting(outputDirs: OutputDirs, default: String): OutputSetting
def PathSetting(name: String, descr: String, default: String): PathSetting
def PhasesSetting(name: String, descr: String, default: String): PhasesSetting
View
25 src/compiler/scala/tools/nsc/settings/MutableSettings.scala
@@ -211,6 +211,11 @@ class MutableSettings(val errorFn: String => Unit)
add(new ChoiceSetting(name, helpArg, descr, choices, default))
def IntSetting(name: String, descr: String, default: Int, range: Option[(Int, Int)], parser: String => Option[Int]) = add(new IntSetting(name, descr, default, range, parser))
def MultiStringSetting(name: String, arg: String, descr: String) = add(new MultiStringSetting(name, arg, descr))
+ def MultiChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String]): MultiChoiceSetting = {
+ val fullChoix = choices.mkString(": ", ",", ".")
+ val fullDescr = s"$descr$fullChoix"
+ add(new MultiChoiceSetting(name, helpArg, fullDescr, choices))
+ }
def OutputSetting(outputDirs: OutputDirs, default: String) = add(new OutputSetting(outputDirs, default))
def PhasesSetting(name: String, descr: String, default: String = "") = add(new PhasesSetting(name, descr, default))
def StringSetting(name: String, arg: String, descr: String, default: String) = add(new StringSetting(name, arg, descr, default))
@@ -548,8 +553,16 @@ class MutableSettings(val errorFn: String => Unit)
}
}
+ class MultiChoiceSetting private[nsc](
+ name: String,
+ arg: String,
+ descr: String,
+ override val choices: List[String])
+ extends MultiStringSetting(name, arg, descr)
+
/** A setting that accumulates all strings supplied to it,
- * until it encounters one starting with a '-'. */
+ * until it encounters one starting with a '-'.
+ */
class MultiStringSetting private[nsc](
name: String,
val arg: String,
@@ -558,11 +571,15 @@ class MutableSettings(val errorFn: String => Unit)
type T = List[String]
protected var v: T = Nil
def appendToValue(str: String) { value ++= List(str) }
+ def badChoice(s: String, n: String) = errorFn(s"'$s' is not a valid choice for '$name'")
def tryToSet(args: List[String]) = {
val (strings, rest) = args span (x => !x.startsWith("-"))
- strings foreach appendToValue
-
+ strings foreach {
+ case "_" if choices.nonEmpty => choices foreach appendToValue
+ case s if choices.isEmpty || (choices contains s) => appendToValue(s)
+ case s => badChoice(s, name)
+ }
Some(rest)
}
override def tryToSetColon(args: List[String]) = tryToSet(args)
@@ -570,6 +587,8 @@ class MutableSettings(val errorFn: String => Unit)
def clear(): Unit = (v = Nil)
def unparse: List[String] = value map (name + ":" + _)
+ def contains(s: String) = value contains s
+
withHelpSyntax(name + ":<" + arg + ">")
}
View
10 src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
@@ -62,8 +62,14 @@ trait ScalaSettings extends AbsScalaSettings
/*val argfiles = */ BooleanSetting ("@<file>", "A text file containing compiler arguments (options and source files)")
val classpath = PathSetting ("-classpath", "Specify where to find user class files.", defaultClasspath) withAbbreviation "-cp"
val d = OutputSetting (outputDirs, ".")
- val nospecialization = BooleanSetting ("-no-specialization", "Ignore @specialize annotations.")
- val language = MultiStringSetting("-language", "feature", "Enable one or more language features.")
+ val nospecialization = BooleanSetting ("-no-specialization", "Ignore @specialize annotations.")
+
+ // Would be nice to build this dynamically from scala.languageFeature.
+ // The two requirements: delay error checking until you have symbols, and let compiler command build option-specific help.
+ val language = {
+ val features = List("dynamics", "postfixOps", "reflectiveCalls", "implicitConversions", "higherKinds", "existentials", "experimental.macros")
+ MultiChoiceSetting("-language", "feature", "Enable one or more language features", features)
+ }
/*
* The previous "-source" option is intended to be used mainly
View
2  src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -738,7 +738,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
val featureName = (nestedOwners map (_.name + ".")).mkString + featureTrait.name
def action(): Boolean = {
def hasImport = inferImplicit(EmptyTree: Tree, featureTrait.tpe, reportAmbiguous = true, isView = false, context).isSuccess
- def hasOption = settings.language.value exists (s => s == featureName || s == "_")
+ def hasOption = settings.language contains featureName
val OK = hasImport || hasOption
if (!OK) {
val Some(AnnotationInfo(_, List(Literal(Constant(featureDesc: String)), Literal(Constant(required: Boolean))), _)) =
View
31 test/files/run/t5905-features.scala
@@ -0,0 +1,31 @@
+
+import tools.partest.DirectTest
+
+// verify that all languageFeature names are accepted by -language
+object Test extends DirectTest {
+ override def code = "class Code { def f = (1 to 10) size }" // exercise a feature to sanity-check coverage of -language options
+
+ override def extraSettings = s"-usejavacp -d ${testOutput.path}"
+
+ override def show() = {
+ val global = newCompiler("-Ystop-after:typer")
+ compileString(global)("") // warm me up, scotty
+ import global._
+ exitingTyper {
+ //def isFeature(s: Symbol) = s.annotations.exists((a: AnnotationInfo) => a.tpe <:< typeOf[scala.annotation.meta.languageFeature])
+ def isFeature(s: Symbol) = s hasAnnotation definitions.LanguageFeatureAnnot
+ val langf = definitions.languageFeatureModule.typeSignature
+ val feats = langf.declarations filter (s => isFeature(s)) map (_.name.decoded)
+ val xmen = langf.member(TermName("experimental")).typeSignature.declarations filter (s => isFeature(s)) map (s => s"experimental.${s.name.decoded}")
+ val all = (feats ++ xmen) mkString ","
+
+ assert(feats.nonEmpty, "Test must find feature flags.")
+
+ //compile("junk") // tragically, does not fail the test
+
+ //dynamics,postfixOps,reflectiveCalls,implicitConversions,higherKinds,existentials,experimental.macros
+ compile(s"-language:$all")
+ }
+ }
+}
+
View
1  test/files/run/t5905b-features.check
@@ -0,0 +1 @@
+'noob' is not a valid choice for '-language'
View
15 test/files/run/t5905b-features.scala
@@ -0,0 +1,15 @@
+
+import tools.partest.DirectTest
+
+// verify that only languageFeature names are accepted by -language
+object Test extends DirectTest {
+ override def code = "class Code"
+
+ override def extraSettings = s"-usejavacp -d ${testOutput.path}"
+
+ override def show() = {
+ //compile("-language", "--") // no error
+ compile(s"-language:noob")
+ }
+}
+
Something went wrong with that request. Please try again.