new approach to field publicizing in Inliner

[magarciaEPFL]

Reworked version derived from

https://groups.google.com/d/topic/scala-internals/iPkMCygzws4/discussion

In particular,

https://groups.google.com/d/msg/scala-internals/iPkMCygzws4/LZHydgSjFSUJ
https://groups.google.com/d/msg/scala-internals/iPkMCygzws4/vEjJ4unAoFEJ
https://groups.google.com/d/msg/scala-internals/iPkMCygzws4/MIDPK6-3vwMJ

review by @VladUreche

Comments on performance impact are welcome /cc @gkossakowski

[scala-jenkins]

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/198/

[scala-jenkins]

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/904/

[scala-jenkins]

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/904/

[scala-jenkins]

jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/198/

[gkossakowski]

Any chance for tests? We really need inliner tests and we have infrastructure to write them now (see 4caa766).

[VladUreche]

This should already be public after superaccessors, no? If so, why add them to toBecomePublic? I'd rather do assert(f.isPublic /* or whatever the incantation is*/).

[VladUreche]

And as Greg pointed out, I'd love to see some tests for it.

[magarciaEPFL]

As I said I'm focusing first on finding out what's wrong with the approach, tests afterwards.

Looking at the conditions under which canRegardAsPublic(f) is invoked:

def checkField(f: Symbol)   = check(f, f.isPrivate && !canRegardAsPublic(f))

The following assertion should hold on entry to canRegardAsPublic(f)

assert(!hasInline(calleeSym),
       "All accesses in @inline-marked methods should have resulted in ExplicitOuter publicizing the accessed field.")

I'll check whether it actually holds. Probably not, because on second thought, I guess #1121 should have been stated as follows:

      if (currentClass != sym.owner ||
          (currentMethod hasAnnotation ScalaInlineClass)) {
        assert(currentRun.compiles(sym), "Trying to publicize a separately-compiled field.")
        sym.makeNotPrivate(sym.owner)  
      }

WDYT?

[VladUreche]

You're right, it probably doesn't hold, but for a completely different reason -- the entry might be protected instead of private. I think the check should be !f.isPublic.

Regarding the patch in #1121, you're right, it should check it's in the same compilation unit, else it will over-approximate the fields that are public.

[magarciaEPFL]

The following reformulation of #1121 seems all we can do in ExplicitOuter:

      // make not private symbol acessed from inner classes, as well as
      // symbols accessed from @inline methods
      if ((currentMethod hasAnnotation ScalaInlineClass) && currentRun.compiles(sym)) {
        sym.makeNotPrivate(sym.owner)
        if(sym.isProtected) { sym.setFlag(notPROTECTED) }
      }
      else if (currentClass != sym.owner) {
        sym.makeNotPrivate(sym.owner)
      }

Better yet, the idea of closestEnclMethod() from 2fd4df8 could be brought back, because in case currentMethod isn't a sourceMethod, it walks up till one is found.

[VladUreche]

I didn't follow the member public-izing problem closely, so I don't exactly know all the aspects. Why do you do:

      else if (currentClass != sym.owner) {
        sym.makeNotPrivate(sym.owner)
      }

?

[magarciaEPFL]

That pre-existed publicizing-for-inlining, it's there so that after flatten has run, accesses from an inner-class reaching out to outer members also work once the inner-class isn't an inner class anymore (after flattten).

[magarciaEPFL]

WIP: The above seems to work, yet doesn't quite do what we want: there are outer fields accessed in at-inline methdos that reach Inliner being private.

[VladUreche]

Thanks for the clarification Miguel.