Skip to content

Loading…

Ticket/6685 #1686

Merged
merged 1 commit into from

5 participants

@xeno-by

No description provided.

@xeno-by xeno-by referenced this pull request
Closed

Ticket/6685 #1685

@xeno-by

Tests will follow shortly

@hubertp hubertp commented on an outdated diff
src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -4521,7 +4521,8 @@ trait Typers extends Modes with Adaptations with Tags {
if (tag.isEmpty) MissingClassTagError(tree, tagType)
else new ApplyToImplicitArgs(Select(tag, nme.newArray), args)
}
- typed(newArrayApp, mode, pt)
+ if (newArrayApp.isErrorTyped) newArrayApp
@hubertp The Scala Programming Language member
hubertp added a note

I haven't debugged this thing but where is the error coming from? If this is from resolveClassTag then tag should be empty, if the MissingClassTagError is a problem then I don't think newArrayApp needs to be created at all.

@xeno-by
xeno-by added a note

This thing is coming from line 4521 to report an error about the fact that resolveClassTag returned an empty tree.

@hubertp The Scala Programming Language member
hubertp added a note

Ok, so why don't you just return the 'tree' there?

@hubertp The Scala Programming Language member
hubertp added a note

I mean not 'return tree' but '{ MissingClassTagError(tree, tagType); tree }' ?

@hubertp The Scala Programming Language member
hubertp added a note

Actually looking at MissingClassTagError this is not even necessary, just moving typed to ApplyToImplicitArgs should be enough.

@xeno-by
xeno-by added a note

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xeno-by xeno-by SI-6685 fixes error handling in typedApply
When MissingClassTagError doesn't lead to an exception, but rather
silently sets an error, we need to bubble the resulting erroneous tree
up the responsibility chain instead of mindlessly typechecking this again.

This wasn't an issue before, because as far as I can guess the
aforementioned error setter was always throwing exceptions in the most
common usage scenarios (therefore the typecheck-again-fail-again
vicious loop wasn't triggered).
e6441f1
@xeno-by

PLS REBUILD ALL

@xeno-by

There's a curious difference in behaviour between the normal invocation of scalac (#1) and the specially crafted environment (#2, described in JIRA).

Before going into the difference, I'd like to expose a horrible truth about the macro engine. Currently it catches all throwables coming form macro invocations, wraps them and converts them in type errors. By all throwables I mean really all, including stack overflows.

In #1 the engine can actually catch stack overflows originating from unbounded recursion in Typers and report them as errors. But because there's already an error at the position of those reports, it masks the subsequent SO reports, and the end result looks completely innocent.

To the contrast, for the reason unknown, in #2 stackoverflows evade the catch-all clause in Macros and manifest themselves as compiler crashes. Probably, this has to something with a custom classloader used to run the compiler.

Bottom line: 1) this PR fixes the root of the bug, 2) the bug was undetected until now, because of a design issue exception handling in macros, 3) I will fix that problem in a later pull request to 2.10.x, 4) because of 2-3 there's imho not much sense in writing a test case for this PR, especially taking into account the complicated environment setup procedure.

@retronym
The Scala Programming Language member

To the contrast, for the reason unknown, in #2 stackoverflows evade the catch-all clause in Macros and manifest themselves as compiler crashes.

The problems arise from code that ends up catching StackOverflowError before the stack has had a chance to unwind, and triggering a follow-up SOE in the exception handler. This can also happen a shade earlier if LHS of the exception handler pattern triggers classloading, e.g. of NonFatal$. So it seems the environmental difference between SBT and vanilla scalac is either which classes are already loaded before it gets to that point, or how much stack the classloader needs to do its business.

To review the exception handling, we chould induce a SOE (perhaps just in a macro body), and check that even under vanilla scalac that this is reported verbatim and that it's not treated as a type error with a fallback.

We should also be aware that scala.util.control.NonFatal intentionally catches SOE. But we see that it's not always straightforward to recover from it as I had thought.

@retronym
The Scala Programming Language member

This patch lgtm for 2.10.0.

@xeno-by

@retronym I've opened a bug for the macro exception handling part: https://issues.scala-lang.org/browse/SI-6748.

@xeno-by

@adriaanm @gkossakowski @jsuereth please, merge - this is a blocker for 2.10.0

@hubertp
The Scala Programming Language member

lgtm

@xeno-by

2 lgtms, >2 days, merging at will

@xeno-by xeno-by merged commit 70a24f9 into scala:2.10.0-wip
@adriaanm
The Scala Programming Language member

@xeno-by, thanks for keeping things moving

one of the reasons i like to merge myself is that i make sure the milestone field is set when i merge so we can later more easily get statistics out of github -- I'm working on a doc (and ideally, automation) for this

until then, when merging, could you make sure the milestone is set? thanks!

@xeno-by

Oh I'm sorry didn't know about milestones. Sure I will be more thorough in the future. Is there something else I need to know?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 30, 2012
  1. @xeno-by

    SI-6685 fixes error handling in typedApply

    xeno-by committed
    When MissingClassTagError doesn't lead to an exception, but rather
    silently sets an error, we need to bubble the resulting erroneous tree
    up the responsibility chain instead of mindlessly typechecking this again.
    
    This wasn't an issue before, because as far as I can guess the
    aforementioned error setter was always throwing exceptions in the most
    common usage scenarios (therefore the typecheck-again-fail-again
    vicious loop wasn't triggered).
Showing with 2 additions and 3 deletions.
  1. +2 −3 src/compiler/scala/tools/nsc/typechecker/Typers.scala
View
5 src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -4516,12 +4516,11 @@ trait Typers extends Modes with Adaptations with Tags {
// [Eugene] no more MaxArrayDims. ClassTags are flexible enough to allow creation of arrays of arbitrary dimensionality (w.r.t JVM restrictions)
val Some((level, componentType)) = erasure.GenericArray.unapply(tpt.tpe)
val tagType = List.iterate(componentType, level)(tpe => appliedType(ArrayClass.toTypeConstructor, List(tpe))).last
- val newArrayApp = atPos(tree.pos) {
+ atPos(tree.pos) {
val tag = resolveClassTag(tree.pos, tagType)
if (tag.isEmpty) MissingClassTagError(tree, tagType)
- else new ApplyToImplicitArgs(Select(tag, nme.newArray), args)
+ else typed(new ApplyToImplicitArgs(Select(tag, nme.newArray), args))
}
- typed(newArrayApp, mode, pt)
case Apply(Select(fun, nme.apply), _) if treeInfo.isSuperConstrCall(fun) => //SI-5696
TooManyArgumentListsForConstructor(tree)
case tree1 =>
Something went wrong with that request. Please try again.