Fwd: script positions

On Thu, May 13, 2010 at 08:20:26PM +0100, Miles Sabin wrote:
> I'd appreciate it if you'd verify that the ant test.positions target
> completes successfully before committing (tho' again, I can't see
> anything obvious in your patch which would affect that).

I found a way it seems. I'm not clear yet on what all the output means
but I assume going from "BUILD SUCCESSFUL" to "BUILD FAILED" is bad.

On Thu, May 13, 2010 at 11:39:52PM -0400, Lex Spoon wrote:
> Writing A.scala instead of "fragment of A.scala" is beautiful. Thanks
> so much for cleaning up that atrocity!

Ah the little things do start to seem big once they've been around long
enough. Thanks very much for your speedy feedback.

> Dropping error messages from the preamble sounds dangerous to me.

I did have this reservation. I thought about having some kind of
option, but it seems like the sort of option one adds to make oneself
feel better and it'd just be noise to almost everyone and unknown to the
few people who might have a need for it. I think we should take enough
responsibility for the wrapper that we can say that if a script doesn't
run, it's because of something in the script. No doubt there will be
some room to be wrong about this, but it's a pretty small piece of code.

On the whole I think it's better left invisible and we make it robust.
If I weigh:

a) difficulty of figuring out problem * frequency of wrapper-caused bug
(if we hide wrapper errors)

vs:

b) difficulty of figuring out problem * frequency of spurious and
irrelevant wrapper-caused error message (if we show them)

then I think net utility says hide them.

> Without such an argument, it seems possible for a compile to fail with
> no reason emitted.

I could middle ground it and show them if there are no errors in the
non-synthetic code.

> Add a script mode to the compiler, and when the compiler is in script
> mode, it parses a compilation unit using parseScript rather than
> whatever it currently uses.

I probably like this better anyway. As you note, both
CompoundSourceFile and SourceFileFragment seem to exist only for this
usage, and getting the positions righ has been unpleasant. I'll take a
look at it from this angle.

> The implementation looks fine to me, except for the complete absence
> of test cases.

Yeah. Testing is a system-wide issue. We have zero script tests. They
were disabled for a long time, and when I took two weeks to rewrite
partest I brought back the grand total of four script tests. Then I had
to revert it all over a performance regression which I couldn't figure
out in a timely manner. That whole partest is off in a tributary:

http://lampsvn.epfl.ch/svn-repos/scala/scala/trunk/src/partest-alternative/

I bring it up as evidence that I take testing seriously, but I don't
want to let it paralyze my development. I have no infrastructure for
writing a script test and having confidence it will pass on ibm's jdk-5
on windows running under virtualbox while angry cannibals hurl spears.
I tried to improve the infrastructure and failed at that too, and I'm
just not willing to make sysadminning a bunch of platforms I don't even
care about into a major part of my existence. I like writing software.
Everything else is just an obstacle to writing software. I fully accept
that a certain amount of "everything else" comes with the territory, and
I do quite a lot of unglamorous work on trunk. But since I don't get
paid there's a line somewhere after which it starts to seem like work,
and while I'm happy to do what I like for free, I don't work for free.

So more briefly, I agree in principle and eagerly await the arrival of
someone who will lower the barrier to writing decent tests.

On Mon, May 17, 2010 at 06:34:28PM -0400, Lex Spoon wrote:
> Okay, I'm convinced. Dropping the preamble/trailer errors is fine for
> now. Someone motivated enough to do better could look at adding the
> new Parser method and thus eliminating the preambles and trailers
> alltogether.

Ha. You are too late. I spent way longer on it than I'd intended.
Miles, can you confirm it'd be OK to remove CompoundSourceFile and
SourceFileFragment? As this I have done in here:

http://github.com/paulp/scala/tree/script-positions

/** This is the parse entry point for code which is not self-contained, e.g.
* a script which is a series of template statements. They will be
* swaddled in Trees until the AST is equivalent to the one returned
* by compilationUnit().
*/
def scriptBody(): Tree = {
val stmts = templateStatSeq(false)._2
accept(EOF)

/** Here we are building an AST representing the following source fiction,
* where is from -Xscript (defaults to "Main") and are
* the result of parsing the script file.
*
* object {
* def main(argv: Array[String]): Unit = {
* val args = argv
* new AnyRef {
*
* }
* }
* }
*/
import definitions._

def emptyPkg = atPos(0, 0, 0) { Ident(nme.EMPTY_PACKAGE_NAME) }
def emptyInit = DefDef(
NoMods,
nme.CONSTRUCTOR,
Nil,
List(Nil),
TypeTree(),
Block(List(Apply(Select(Super("", ""), nme.CONSTRUCTOR), Nil)), Literal(Constant(())))
)

// def main
def mainParamType = AppliedTypeTree(Ident(nme.Array.toTypeName), List(Ident(nme.String.toTypeName)))
def mainParameter = List(ValDef(NoMods, "argv", mainParamType, EmptyTree))
def mainSetArgv = List(ValDef(NoMods, "args", TypeTree(), Ident("argv")))
def mainNew = makeNew(Nil, emptyValDef, stmts, List(Nil), NoPosition, NoPosition)
def mainDef = DefDef(NoMods, "main", Nil, List(mainParameter), scalaUnitConstr, Block(mainSetArgv, mainNew))

// object Main
def moduleName = ScriptRunner scriptMain settings
def moduleBody = Template(List(scalaScalaObjectConstr), emptyValDef, List(emptyInit, mainDef))
def moduleDef = ModuleDef(NoMods, moduleName, moduleBody)

// package { ... }
makePackaging(0, emptyPkg, List(moduleDef))
}

> I was thinking how such a test might be built. Perhaps simply invoke
> ScriptRunner directly?

That is exactly the sad route I had taken, just for you.

import scala.tools.nsc._
import util.stringFromStream

// Testing "scripts" without the platform delights which accompany actual scripts.
object Scripts {

val test1 =
"""#!/bin/sh
exec scala $0 $@
!#

println("statement 1")
println("statement 2".thisisborked)
println("statement 3")
"""

val output1 =
"""
6: error: value thisisborked is not a member of java.lang.String
println("statement 2".thisisborked)
^
one error found
"""

val test2 =
"""#!scala
// foo
// bar
!#

val x = "line 6"
val y = "line 7"
val z "line 8"
"""

val output2 =
"""
8: error: '=' expected but string literal found.
val z "line 8"
^
8: error: illegal start of simple expression
val z "line 8"
^
two errors found
"""
}

object Test {
import Scripts._

def settings = new GenericRunnerSettings(println _)

def runScript(code: String): String =
stringFromStream(stream =>
Console.withOut(stream) {
Console.withErr(stream) {
ScriptRunner.runCommand(settings, code, Nil)
}
}
)

val tests: List[(String, String)] = List(
test1 -> output1,
test2 -> output2
)
def lines(s: String) = s split """\r\n|\r|\n""" toList

// strip the random temp filename from error msgs
def stripFilename(s: String) = (s indexOf ".scala:") match {
case -1 => s
case idx => s drop (idx + 7)
}
def toLines(text: String) = lines(text.trim()) map stripFilename

def main(args: Array[String]): Unit = {
for ((code, expected) <- tests) {
val out = toLines(runScript(code))
val exp = toLines(expected)
val nomatch = out zip exp filter { case (x, y) => x != y }
val success = out.size == exp.size && nomatch.isEmpty

assert(
success,
nomatch.map(x => "out >> " + x._1 + "\nexp >> " + x._2)
. mkString("Output doesn't match expected:\n", "\n\n", "")
)
}
}
}

Oh, and I should point out that at this moment it has the positions
right but it is still printing the spurious errors; I need to
reintegrate the "ignore errors from synthetic positions" bits from my
original patch.