Skip to content
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

Scala compiler crashes during lamdalift due to assertion failure #6562

Closed
scabug opened this issue Oct 23, 2012 · 17 comments
Closed

Scala compiler crashes during lamdalift due to assertion failure #6562

scabug opened this issue Oct 23, 2012 · 17 comments

Comments

@scabug
Copy link

scabug commented Oct 23, 2012

The following code causes the compiler to crash. When the @inline annotation is removed, the compiler seems to work ok.

// Test.scala
class Test {

  @inline
  private def foo {
      def it {
      }
    List().foreach { _ => it }
  }
}
@scabug
Copy link
Author

scabug commented Oct 23, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6562?orig=1
Reporter: Robby (robby)
Affected Versions: 2.10.0-RC1
Other Milestones: 2.10.0
See #6608
Attachments:

@scabug
Copy link
Author

scabug commented Oct 23, 2012

Robby (robby) said:
Attached is the compiler error output.

@scabug
Copy link
Author

scabug commented Oct 23, 2012

@paulp said:
Note that this is not an optimizer bug - it crashes under regular compilation with @inline, and doesn't without.

@scabug
Copy link
Author

scabug commented Oct 23, 2012

@retronym said:
Here's the regression:

https://github.com/scala/scala/commit/v2.10.0-M7~27

@scabug
Copy link
Author

scabug commented Oct 25, 2012

@retronym said:
This will fix it, but it's a bit too shameful to submit as is:

retronym/scala@2.10.0-wip...ticket/6562

SI-6562 Assume less about the name of the OUTER_LOCAL …
Since 07f9429, the name of this field can be expanded.
Not sure which is the best way around this:

  - avoid expanding synthetic names in `makeNonPrivate`?
  - try with/withou the same expansion in `outerField`?
  - loosen the member lookup `name.endsWith(OUTER_LOCAL)`

For now, I've gone with the last choice.

The suggestion box awaits your comments.

@scabug
Copy link
Author

scabug commented Oct 25, 2012

@retronym said:
Bumping to critical, we should fix this for 2.10.0, if only through a reversion.

@scabug
Copy link
Author

scabug commented Oct 26, 2012

@retronym said (edited on Oct 27, 2012 7:32:48 AM UTC):
Compare scala/scala@e6b4204604

         case Select(qual, name) =>
-          /** return closest enclosing method, unless shadowed by an enclosing class;
-           *  no use of closures here in the interest of speed.
-           */
-          def closestEnclMethod(from: Symbol): Symbol =
-            if (from.isSourceMethod) from
-            else if (from.isClass) NoSymbol
-            else closestEnclMethod(from.owner)
-
-          if (currentClass != sym.owner ||
-              (closestEnclMethod(currentOwner) hasAnnotation ScalaInlineClass))
+          if (currentClass != sym.owner)
             sym.makeNotPrivate(sym.owner)

With scala/scala@5d4e4e2 (the point of the regression, and a seeming half-reversion of e6b4204604).

-          if (currentClass != sym.owner)
+          // make not private symbol acessed from inner classes, as well as
+          // symbols accessed from @inline methods
+          if (currentClass != sym.owner ||
+              (sym.owner.enclMethod hasAnnotation ScalaInlineClass))

Martin reasoned:

@magarciaEPFL: I convinced myself otherwise. The only difference is if we have an @inline method that has a nested
class
...
But in that case the whole discussion is moot anyway because the class does not escape the method. So we can live with the standard @enclMethod code and do not need a new concept.

Unless there are other comments I'd vote on merging this now.

Later, the inliner was changed:

scala/scala@f71dca29

Made inliner work for @inline methods that access private variables.
We need to disable the previous "potentiallyPublished" logic for this because that one disables inlining as long as the inlined method has a reference to a field with a $. But making fields public in @inline method will generate fields with $. We need to complement this with reverting the previous publication logic completely. I leave that to Vlad or Miguel.

So we've got the changes to SuperAccessors, ExplicitOuter, and Inliners. It all seems like loose ends in need of a sturdy knot.

@scabug
Copy link
Author

scabug commented Oct 26, 2012

@retronym said:
This regression might need attention for 2.10.0.

@scabug
Copy link
Author

scabug commented Oct 27, 2012

@retronym said (edited on Oct 27, 2012 6:37:29 PM UTC):
Here's my suggestion:

https://github.com/retronym/scala/tree/ticket/6562-2

But before submitting a PR, I'm going to try to create a test case for the problem Miguel described:

https://groups.google.com/d/msg/scala-internals/iPkMCygzws4/hjosnYfLMicJ

@scabug
Copy link
Author

scabug commented Oct 28, 2012

@retronym said:
Here's a more ambitious version, that removes access widening from SuperAccessors altogether.

retronym/scala@2.10.0-wip...ticket/6562-3

I suspect there are subtle issues with binary compatibility if we defer this change to 2.10.1. The assumptions that we have encoded into Inliners about what happened earlier in ExplicitOuter only hold if the inlinable method and the call site are compiled with the same version of the compiler.

Perhaps we could harden things by validating those assumptions (checking the Java types), or offering the improvements behind a -Y option for use only by people with homogenenously compiled libraries.

@scabug
Copy link
Author

scabug commented Oct 30, 2012

@odersky said:
The fix looks right to me. Much better to concentrate makeNotPrivate logic in ExplicitOuter. Can you make a pull-request against 2.10.0-wip? Maybe with me and Miguel as reviewers?

Thanks

  • Martin

@scabug
Copy link
Author

scabug commented Oct 30, 2012

@retronym said (edited on Oct 30, 2012 5:44:11 PM UTC):
Unfortunately it isn't fully functional: If the @inline-d method references, for example, a private var in an enclosing class, ExplicitOuter will expand its name, but the Inliners will generate a call to the original name, as it stood at the time of pickling.

Inliners will either have to replicate the makeNotPrivate calls (fragile?) or figure out that names are expanded based on the Java signature (major surgery?).

I'll commit the failing test case to the branch tonight to make this clearer.

I'm also worried about references to package-qualified private members of other compilation units. ExplicitOuter should only makeNotPrivate symbols from the current compilation unit. Some cases work at the moment, as those members are public in bytecode anyway, but I need to write some more tests. Again, the interesting case comes when names are expanded, ie when the referenced symbol is a class/trait member.

@scabug
Copy link
Author

scabug commented Oct 30, 2012

@retronym said:
Here's the now-breaking test:

retronym/scala@d6d6ad6

My concern about qualified private members was misplaced: these are already !isPrivate, and as such not subject to the whims of makeNotPrivate.

@scabug
Copy link
Author

scabug commented Oct 30, 2012

@retronym said:

Inliners will either have to replicate the makeNotPrivate calls (fragile?) or figure out that names are expanded based on the Java signature (major surgery?).

A tentative fix drawn from the fragile basket:

retronym/scala@b4aedd23

@scabug
Copy link
Author

scabug commented Nov 2, 2012

@retronym said:
scala/scala#1556

@scabug
Copy link
Author

scabug commented Nov 3, 2012

@adriaanm said:
Workaround in scala/scala#1561. Lowering priority, shifting gaze.

@scabug
Copy link
Author

scabug commented Nov 3, 2012

@retronym said:
Moved the (serious, IMO) residual issues to #6608

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants