Skip to content

Fix varargs in methods #2135

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

Merged
merged 4 commits into from
Mar 22, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions compiler/src/dotty/tools/dotc/parsing/Parsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,24 @@ package parsing

import scala.collection.mutable.ListBuffer
import scala.collection.immutable.BitSet
import util.{ SourceFile, SourcePosition }
import util.{SourceFile, SourcePosition}
Copy link
Contributor

Choose a reason for hiding this comment

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

also - these changes here - are they done by your editor or by you? I'd leave them out of the diff

import Tokens._
import Scanners._
import MarkupParsers._
import core._
import Flags._
import Contexts._
import Names._
import ast.Positioned
import ast.{Positioned, Trees, untpd}
import ast.Trees._
import Decorators._
import StdNames._
import util.Positions._
import Constants._
import ScriptParsers._
import Comments._
import scala.annotation.{tailrec, switch}

import scala.annotation.{switch, tailrec}
Copy link
Contributor

Choose a reason for hiding this comment

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

same with this, if you intended to move this so that scala imports are separate from internal imports, then this should be at the top with the other one. Otherwise let's get rid off this change.

import util.DotClass
import rewrite.Rewrites.patch

Expand Down Expand Up @@ -1921,6 +1922,21 @@ object Parsers {
}
}



private def checkVarArgsRules(vparamss: List[List[untpd.ValDef]]): List[untpd.ValDef] = {
def isVarArgs(tpt: Trees.Tree[Untyped]): Boolean = tpt match {
case PostfixOp(_, op) if op.name == nme.raw.STAR => true
case _ => false
}

vparamss.flatMap { params =>
if (params.nonEmpty) {
params.init.filter(valDef => isVarArgs(valDef.tpt))
} else List()
}
}

/** DefDef ::= DefSig (`:' Type [`=' Expr] | "=" Expr)
* | this ParamClause ParamClauses `=' ConstrExpr
* DefDcl ::= DefSig `:' Type
Expand Down Expand Up @@ -1950,6 +1966,10 @@ object Parsers {
val name = ident()
val tparams = typeParamClauseOpt(ParamOwner.Def)
val vparamss = paramClauses(name)
val listOfErrors = checkVarArgsRules(vparamss)
listOfErrors.foreach { vparam =>
syntaxError(VarArgsParamMustComeLast(), vparam.tpt.pos)
}
var tpt = fromWithinReturnType(typedOpt())
if (in.isScala2Mode) newLineOptWhenFollowedBy(LBRACE)
val rhs =
Expand Down
12 changes: 12 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1209,4 +1209,16 @@ object messages {
|${parents.mkString(" - ", "\n - ", "")}
|""".stripMargin
}

case class VarArgsParamMustComeLast()(implicit ctx: Context)
extends Message(IncorrectRepeatedParameterSyntaxID) {
override def msg: String = "*-parameter must come last"
Copy link
Contributor

Choose a reason for hiding this comment

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

let's say "varargs parameter must come last", mixing symbols and text like this feels kind of weird IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied error message from Scala Compiler :)


override def kind: String = "Syntax"

override def explanation: String =
hl"""|The varargs field must be the last field in the method signature.
|Attempting to define a field in a method signature after a varargs field is an error.
|""".stripMargin
}
}
3 changes: 3 additions & 0 deletions compiler/test/dotc/tests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ class tests extends CompilerTest {
@Test def pos_anonClassSubtyping = compileFile(posDir, "anonClassSubtyping", twice)
@Test def pos_extmethods = compileFile(posDir, "extmethods", twice)
@Test def pos_companions = compileFile(posDir, "companions", twice)
@Test def posVarargsInMethodsT1625 = compileFiles(posDir + "varargsInMethodsT1625/")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also apply to constructors - I think your implementation supports this, I guess it's just the name that is misleading.


@Test def pos_all = compileFiles(posDir) // twice omitted to make tests run faster

Expand All @@ -177,6 +178,8 @@ class tests extends CompilerTest {
@Test def neg_all = compileFiles(negDir, verbose = true, compileSubDirs = false)
@Test def neg_typedIdents() = compileDir(negDir, "typedIdents")

@Test def negVarargsInMethodsT1625 = compileFiles(negDir + "varargsInMethodsT1625/")
Copy link
Contributor

Choose a reason for hiding this comment

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

same here


val negCustomArgs = negDir + "customArgs/"

@Test def neg_typers() = compileFile(negCustomArgs, "typers")(allowDoubleBindings)
Expand Down
3 changes: 3 additions & 0 deletions tests/neg/varargsInMethodsT1625/allParamsAreVarArgs.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
trait T3 {
def foo(x: String*, y: String*, c: String*): Int // error // error: *-parameter must come last
}
3 changes: 3 additions & 0 deletions tests/neg/varargsInMethodsT1625/curriedNegExample.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
trait T2 {
def foo(x: String*, y: String*)(z: Boolean*)(u: Int*, l: Int*): Int // error // error: *-parameter must come last
}
3 changes: 3 additions & 0 deletions tests/neg/varargsInMethodsT1625/firstParamIsVarArgs.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
trait T1 {
def foo(x: String*, y: String): Int // error: *-parameter must come last
Copy link
Contributor

Choose a reason for hiding this comment

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

also - just an FYI, the ": error text here" part of the comment is not parsed in any way (currently)

no need to change or anything

}
3 changes: 3 additions & 0 deletions tests/pos/varargsInMethodsT1625/curriedPosExample.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
trait T2 {
def foo(x: String, y: String*)(z: Boolean*)(u: Int, l: Int*): Int
}
3 changes: 3 additions & 0 deletions tests/pos/varargsInMethodsT1625/onlyOneVarArgs.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
trait T1 {
def foo(x: String*): Int
}