CPS: enable return expressions in CPS code if they are in tail position #720

Merged
merged 4 commits into from Jun 18, 2012

5 participants

@phaller

Adds a stack of context trees to AnnotationChecker(s). Here, it is used to
adapt the annotation of a return expression if the expected type is a CPS type.

The remove-tail-return transform is reasonably general, covering cases such as
try-catch-finally. Moreover, an error is thrown if, in a CPS method, a return
is encountered which is not in a tail position such that it will be removed
subsequently.

Review by @adriaanm or @TiarkRompf

phaller added some commits May 25, 2012
@phaller phaller CPS: enable return expressions in CPS code if they are in tail position
Adds a stack of context trees to AnnotationChecker(s). Here, it is used to
enforce that adaptAnnotations will only adapt the annotation of a return
expression if the expected type is a CPS type.

The remove-tail-return transform is reasonably general, covering cases such as
try-catch-finally. Moreover, an error is thrown if, in a CPS method, a return
is encountered which is not in a tail position such that it will be removed
subsequently.
796024c
@phaller phaller Remove stray comment cdfbe8e
@adriaanm
The Scala Programming Language member

Could we simply use context.enclMethod.returnsSeen? If you really need a stack (although it seems you only inspect its head), I would like a more specific name, as it's only used for returns.

The Scala Programming Language member

Scratch that, as you need to know whether you're nested under a return node of course. It still feels wrong to use a stack. Maybe add a flag to context and use that instead of inReturnContext?

AnnotationCheckers don't have access to the typer's context, though, and without a major refactoring I don't see a way to enable this. Instead in commit 51c92f0 I replaced the stack with a new typing mode (RETmode) which is set when we are typing a return expression. This is a big simplification, since that way we also avoid extending the AnnotationChecker interface. Among others it also removes inReturnContext.

The Scala Programming Language member

I agree with Adriaan that it doesn't feel very nice to keep an explicit stack (also, shouldn't popAnnotationContext live in a finally block?). However, I don't see how RETmode is able to communicate the return type of the method.

As a third alternative I still stand by my original suggestion (see fbb7b1ed45dcfeea658293295392b94528b4cf56): "... add another hook to the AnnotationChecker interface that would be invoked from typedReturn to delegate how exactly return arg type and method return type should be compared".

@adriaanm
The Scala Programming Language member

In addition to my inline comment, will this work for (admittedly weird) code like

'''
return {foo; bar}
'''

(not a rethoric question)

I'll leave it to Tiark to comment on the cps type state annotations.

@phaller

Code like return { foo; bar } is supported; commit 51c92f0 adds a test case for that (continuations-run/ts-1681-3.scala).

@phaller

Tiark (@TiarkRompf) has already reviewed an earlier version of the type state annotations. Most of his comments should be addressed in the current version. I've asked him to have another look, though.

@jsuereth
The Scala Programming Language member

Should I wait to merge, or can I merge now then?

@phaller

OK, please go ahead and merge. If any other issues pop up I'll address them in a subsequent pull request.

@jsuereth jsuereth merged commit bf4b982 into scala:master Jun 18, 2012
@adriaanm
The Scala Programming Language member

retro-active thumbs-up!

@TiarkRompf
The Scala Programming Language member

I haven't had time for an in-depth review but I've just tried a few things in the REPL (latest master) and run into a couple of problems:

def foo(x:Int): Int @cps[Int] = x

def bar(x:Int): Int @cps[Int] = return foo(x)
error: return expressions in CPS code must be in tail position

Clearly, the return is in tail position, so the error is wrong.

def bar(x:Int): Int @cps[String] = return foo(x)
error: return expressions in CPS code must be in tail position

This should be a type error. Since the emitted error is issued by SelectiveANF, which runs after typer, this tells me that type checking misses the type error.

def bar(x:Int) = return foo(x)

This produces a compiler crash (NPE in hasPlusMarker called from addAnnotations).

I'll have a close look at the implementation, too, but these issues need to be addressed before I can give green light.

EDIT: Another one, which does not even involve cps code:

def bar(x:Int): Int = { return x; x }  // error: return expressions in CPS code must be in tail position
def bar(x:Int) = { return x; x }  // NPE
@TiarkRompf

As mentioned previously, I don't think changing the type of Return statements is the right thing. Consider { foo; return e}, which has type Nothing. Changing the type of 'return e' to e's type in isolation is not likely going to work.

Note that we are changing the type only if it's a Return with an expression with a plus marker. This means that it's a Return that will either (a) be removed (because it's in tail position), or (b) trigger an error. Changing the type only at the point of the removal might be less complicated, so I'll have a look at that.

@TiarkRompf

Since these methods are used exclusively by SelectiveANFTransform, they should live in SelectiveANFTransform.scala. Why is it necessary to keep a list of tail returns? All tail returns are removed, so any remaining returns are errors. It seems like issuing non-tail errors could be done from transExpr and friends and does not require a separate traversal. Also, 'removeTailReturn' seems to be missing a 'Return' case. I assume this fill fix the 'def foo = return e' case.

@TiarkRompf

see note above

@phaller

Thanks for your REPL experiments, Tiark. The problems you ran into should all be easy to fix. Will do that as soon as I get a chance.

@phaller phaller added a commit to phaller/scala that referenced this pull request Jun 27, 2012
@phaller phaller Revert pull request #720 (CPS: enable return expressions in CPS code …
…if they are in tail position)

Reverts commit 0ada070.
Reverts commit 51c92f0.
Reverts commit cdfbe8e.
Reverts commit 796024c.
1a3976f
@paulp paulp added a commit that referenced this pull request Jun 29, 2012
@paulp paulp Merge branch 'master' into prime
* master:
  Fixed race condition that was caused by Statistics pushTimer.
  Fix SI-5846 and SI-4027.
  Fix SI-5336.
  Fix SI-3326.
  Fix SI-5986.
  Fix SI-5971.
  Revert pull request #720 (CPS: enable return expressions in CPS code if they are in tail position)
  Parallelize convertTo in parallel collection.
  make tests independent of compiler api
  Test that closes SI-5839. Bug itself most probably fixed by #602
  Updated Eclipse project files for `asm`, correcting a small typo in linked source folders.
  Closes SI-5148.
  SI-5899 exhaustiveness for non-class types
  exhaust unit: consider Unit as sealed
  SI-5914 handle null in classtag extractor
  SI-2442 fixed by virtpatmat -- test files only
  better fix for SI-5189 pt1
  minor cleanup in patmat and typers
  SI-5761: fix up #774 (missing check file)
  Fix range positions when applying anonymous classes. Review by @dragos or @odersky
  Added new project files for using the compiler and library inside Eclipse and removed the old ones.
  Added test for SI-5761. Seems that my cleanup of SI-4842 also fixed that one. Review by @paulp or @retronym.
  fix for SI-4935
  Statistics improvements and bug fixes.
  Minor followup on SI-4842: remove awkward condition. Review by @retronym.
  emit JVM InnerClasses attribute for bean info classes
  LongMap: Made life a bit less scary for those doing source archaeology.
  IntMap: Made life a bit less scary for those doing source archaeology.
  Adds missing closing curly brace to ScalaDoc code example.
  Added a key comment.
  SI-5968 Eliminate spurious exhaustiveness warning with singleton types.
  SI-5966 Fix eta expansion for repeated parameters with zero arguments.
  Exported new packages in the MANIFEST file.
  Don't swallow `Throwables` while parsing bytecode. Print a warning and go on.
  removes pre-M4 compatibility stubs for the IDE
  adds `narrow` to the reflection API
  SI-4989 Reject super.x if an intermediate class declares x abstract.

Conflicts:
	src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
	src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
	src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala
	src/compiler/scala/tools/nsc/typechecker/EtaExpansion.scala
	src/compiler/scala/tools/nsc/typechecker/Infer.scala
	src/continuations/plugin/scala/tools/selectivecps/CPSAnnotationChecker.scala
	src/continuations/plugin/scala/tools/selectivecps/CPSUtils.scala
	src/continuations/plugin/scala/tools/selectivecps/SelectiveANFTransform.scala
	src/library/scala/collection/IterableViewLike.scala
	src/library/scala/collection/SeqViewLike.scala
	src/library/scala/collection/TraversableViewLike.scala
	src/library/scala/collection/immutable/IntMap.scala
	src/library/scala/collection/immutable/LongMap.scala
	src/library/scala/collection/immutable/MapLike.scala
	src/library/scala/collection/immutable/SortedMap.scala
	src/library/scala/collection/parallel/ParIterableLike.scala
	src/library/scala/collection/parallel/immutable/ParIterable.scala
	src/library/scala/collection/parallel/mutable/ParIterable.scala
	src/library/scala/reflect/internal_compat.scala
	src/msil/ch/epfl/lamp/compiler/msil/emit/ILGenerator.scala
	src/reflect/scala/reflect/internal/SymbolTable.scala
e990f84
@szeiger szeiger pushed a commit to szeiger/scala that referenced this pull request Nov 7, 2013
@phaller phaller SI-5314 - CPS transform of return statement fails
Enable return expressions in CPS code if they are in tail position. Note that tail returns are
only removed in methods that do not call `shift` or `reset` (otherwise, an error is reported).

Addresses the issues pointed out in a previous pull request:
scala#720

- Addresses all issues mentioned here:
  scala#720 (comment)

- Move transformation methods to SelectiveANFTransform.scala:
  scala#720 (comment)

- Do not keep a list of tail returns.

Tests:
- continuations-neg/t5314-missing-result-type.scala
- continuations-neg/t5314-type-error.scala
- continuations-neg/t5314-npe.scala
- continuations-neg/t5314-return-reset.scala
- continuations-run/t5314.scala
- continuations-run/t5314-2.scala
- continuations-run/t5314-3.scala
75e3623
@adriaanm adriaanm added a commit to scala/scala-continuations that referenced this pull request Dec 3, 2013
@phaller phaller SI-5314 - CPS transform of return statement fails
Enable return expressions in CPS code if they are in tail position. Note that tail returns are
only removed in methods that do not call `shift` or `reset` (otherwise, an error is reported).

Addresses the issues pointed out in a previous pull request:
scala/scala#720

- Addresses all issues mentioned here:
  scala/scala#720 (comment)

- Move transformation methods to SelectiveANFTransform.scala:
  scala/scala#720 (comment)

- Do not keep a list of tail returns.

Tests:
- continuations-neg/t5314-missing-result-type.scala
- continuations-neg/t5314-type-error.scala
- continuations-neg/t5314-npe.scala
- continuations-neg/t5314-return-reset.scala
- continuations-run/t5314.scala
- continuations-run/t5314-2.scala
- continuations-run/t5314-3.scala
9954b2f
@adriaanm adriaanm pushed a commit to adriaanm/scala that referenced this pull request Jan 22, 2014
@phaller phaller SI-5314 - CPS transform of return statement fails
Enable return expressions in CPS code if they are in tail position. Note that tail returns are
only removed in methods that do not call `shift` or `reset` (otherwise, an error is reported).

Addresses the issues pointed out in a previous pull request:
scala#720

- Addresses all issues mentioned here:
  scala#720 (comment)

- Move transformation methods to SelectiveANFTransform.scala:
  scala#720 (comment)

- Do not keep a list of tail returns.

Tests:
- continuations-neg/t5314-missing-result-type.scala
- continuations-neg/t5314-type-error.scala
- continuations-neg/t5314-npe.scala
- continuations-neg/t5314-return-reset.scala
- continuations-run/t5314.scala
- continuations-run/t5314-2.scala
- continuations-run/t5314-3.scala
2c00346
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment