classfile parsing

I remember when Lukas added that comment, and it seemed reasonable. Now I'm pretty sure it's impossible, or very hard to use a single classfile parser for all classfiles. It used to work way before Java generics were integrated, and just because we never needed the info of another class while parsing. However, for parsing generic types we need to know their type parameters, therefore we need full types. The cyclic error mentioned in the comment happens while parsing method 'getClass' on Object, which returns a java.lang.Class<?>, triggering the parsing of java.lang.Class, and hitting the busy flag of the one and only parser instance.

The proper 'fix' for this fixme is to delete the comment. If you're at it and feel like committing a 'no-review' patch, please do so, otherwise I'll do it.

iulian

On Wed, Dec 16, 2009 at 3:31 PM, Paul Phillips <paulp@improving.org> wrote:

So looking through various inner classes bugs, I came across this:

 class ClassfileLoader(val classfile: AbstractFile) extends SymbolLoader {

   /**
    * @FIXME: iulian,
    * there should not be a new ClassfileParser for every loaded classfile, this object
    * should be outside the class ClassfileLoader! This was changed by Sean in r5494.
    *
    * However, when pulling it out, loading "java.lang.Object" breaks with:
    *   "illegal class file dependency between java.lang.Object and java.lang.Class"
    */
   private object classfileParser extends ClassfileParser {
     val global: SymbolLoaders.this.global.type = SymbolLoaders.this.global
   }

   protected def description = "class file "+ classfile.toString

   protected def doComplete(root: Symbol) {
     classfileParser.parse(classfile, root)
   }
 }

Just for fun I rewrote this as follows.  Note that I am not suggesting
this is the least bit sensible, but I wanted to bring it up in case it
hinted anyone toward a solution.  Rewritten as below, at least the
following ticket suddenly starts working:

 https://lampsvn.epfl.ch/trac/scala/ticket/2433

The test suite almost passes - a couple fail with classfile broken type
messages, which I didn't look into much since I had no intention of
pursuing anything like this.

 // moved outside as suggested
 private object classfileParser extends ClassfileParser {
   val global: SymbolLoaders.this.global.type = SymbolLoaders.this.global
 }

 class ClassfileLoader(val classfile: AbstractFile) extends SymbolLoader {

   private object classfileParser2 extends ClassfileParser {
     val global: SymbolLoaders.this.global.type = SymbolLoaders.this.global
   }

   // if at first you don't succeed...
   protected def doComplete(root: Symbol) {
     try classfileParser.parse(classfile, root)
     catch { case e => classfileParser2.parse(classfile, root) }
   }
 }

--
Paul Phillips      | Eschew mastication.
Vivid              |
Empiricist         |
i pull his palp!   |----------* http://www.improving.org/paulp/ *----------

--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

On Wed, Dec 16, 2009 at 04:00:46PM +0100, Iulian Dragos wrote:
> The proper 'fix' for this fixme is to delete the comment. If you're at
> it and feel like committing a 'no-review' patch, please do so,
> otherwise I'll do it.

I'm happy to delete the comment. But that doesn't get me any closer to
seeing how #2433 and related tickets might be addressed.

https://lampsvn.epfl.ch/trac/scala/ticket/2433

For reference the crash error is:

Exception in thread "main" java.lang.AssertionError:
assertion failed: illegal class file dependency between 'class GoogleService' and 'object GoogleService'

On Wed, Dec 16, 2009 at 4:07 PM, Paul Phillips wrote:
> On Wed, Dec 16, 2009 at 04:00:46PM +0100, Iulian Dragos wrote:
>> The proper 'fix' for this fixme is to delete the comment. If you're at
>> it and feel like committing a 'no-review' patch, please do so,
>> otherwise I'll do it.
>
> I'm happy to delete the comment.  But that doesn't get me any closer to
> seeing how #2433 and related tickets might be addressed.
>
>  https://lampsvn.epfl.ch/trac/scala/ticket/2433
>
> For reference the crash error is:
>
> Exception in thread "main" java.lang.AssertionError:
> assertion failed: illegal class file dependency between 'class GoogleService' and 'object GoogleService'
>
Generally, fallbacks if the form try this if you fail try that have to
be carefully analyzed before we let them info the codebase. The
problem is exponential blowup of runtime cost. I am not sure this can
happen for classfiel loading, but it certainly did happen to us
already for compilation.

Cheers

Ok, I'll put aside the 1.6 backend and see what can be done about that ticket...

On Wed, Dec 16, 2009 at 4:07 PM, Paul Phillips <paulp@improving.org> wrote:

On Wed, Dec 16, 2009 at 04:00:46PM +0100, Iulian Dragos wrote:
> The proper 'fix' for this fixme is to delete the comment. If you're at
> it and feel like committing a 'no-review' patch, please do so,
> otherwise I'll do it.

I'm happy to delete the comment.  But that doesn't get me any closer to
seeing how #2433 and related tickets might be addressed.

 https://lampsvn.epfl.ch/trac/scala/ticket/2433

For reference the crash error is:

Exception in thread "main" java.lang.AssertionError:
assertion failed: illegal class file dependency between 'class GoogleService' and 'object GoogleService'

--
Paul Phillips      | Every normal man must be tempted at times
Apatheist          | to spit on his hands, hoist the black flag,
Empiricist         | and begin to slit throats.
pal, i pill push   |     -- H. L. Mencken

--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

It's a true cyclic dependency involving inner classes:

GoogleService implements AuthTokenFactory.TokenListener
AuthTokenFactory has a method that takes a GoogleService.SessionExpiredException

Each class needs to know the members of the other in order to resolve its dependency. We need a lazy type to break the cycle.. It took me some time to understand why the fallback approach worked. Here's a sketch, and a possible scenario in which it wouldn't work:

- the 'unique' parser starts reading GoogleService, needs to read the superclass and triggers loading AuthTokenFactory
- the 'unique' parser is now busy, so the nested parser is used to read AuthTokenFactory, which proceeds until the last method, which needs GoogleService.SessionExpiredException
- the 'unique' parser is still busy, so the nested parser (but another instance) loads 'GoogleService' again (no busy flag since it's a new instance). The catch is here: howcome there's no cycle when reading the superclass, hitting AuthTokenFactory again? The answer is that the 'info' of a class is set immediately after its superclass is parsed (see lines 442-443 in ClassfileParser), so the second try will find most members of AuthTokenFactory in place.

I guess (not tried) that if AuthTokenFactory also implemented an inner interface of GoogleService the fallback approach wouldn't work.

cheers,
iulian


On Wed, Dec 16, 2009 at 4:38 PM, Iulian Dragos <iulian.dragos@epfl.ch> wrote:

Ok, I'll put aside the 1.6 backend and see what can be done about that ticket...

On Wed, Dec 16, 2009 at 4:07 PM, Paul Phillips <paulp@improving.org> wrote:

On Wed, Dec 16, 2009 at 04:00:46PM +0100, Iulian Dragos wrote:
> The proper 'fix' for this fixme is to delete the comment. If you're at
> it and feel like committing a 'no-review' patch, please do so,
> otherwise I'll do it.

I'm happy to delete the comment.  But that doesn't get me any closer to
seeing how #2433 and related tickets might be addressed.

 https://lampsvn.epfl.ch/trac/scala/ticket/2433

For reference the crash error is:

Exception in thread "main" java.lang.AssertionError:
assertion failed: illegal class file dependency between 'class GoogleService' and 'object GoogleService'

--
Paul Phillips      | Every normal man must be tempted at times
Apatheist          | to spit on his hands, hoist the black flag,
Empiricist         | and begin to slit throats.
pal, i pill push   |     -- H. L. Mencken

--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais