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

Broken class Signature in GenTraversableFactory #4789

Closed
scabug opened this issue Jul 11, 2011 · 15 comments
Closed

Broken class Signature in GenTraversableFactory #4789

scabug opened this issue Jul 11, 2011 · 15 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jul 11, 2011

Commit r25230 reveals some problem in scalac that results in broken class signature emitted for GenTraversableFactory. As for now, scala library from trunk is completely unusable for Eclipse users.

In order to see the problem it's enough to create a following file in Eclipse:

import scala.collection.immutable.List;
import scala.collection.immutable.List$;

public class Test {
  void foo() {
    Object x = List$.MODULE$;
  }
}

Eclipse will complain about the first import with following error message:
Multiple markers at this line
- The class file GenTraversableFactory contains a signature '()Lscala/collection/generic/GenTraversableFactory<TCC;>.ReusableCBF;' ill-formed at
position 56
- The class file GenTraversableFactory contains a signature '()Lscala/collection/generic/GenTraversableFactory<TCC;>.ReusableCBF;' ill-formed at
position 56

A bit more involved example exposes this issue with javac:

mac-grek:src grek$ cat scala/collection/Test.java
package scala.collection;

import scala.collection.generic.GenTraversableFactory;
import scala.collection.immutable.List;

public class Test {

  public static void main(String[] args) {
    GenTraversableFactory<List<Object>> x = null;
    x.ReusableCBF();
  }

}

mac-grek:src grek$ javac -classpath ~/tmp/badsigs/scala-library-2.10.0-20110711.015803-87.jar scala/collection/Test.java
scala/collection/Test.java:10: cannot access scala.collection.generic.GenTraversableFactory$ReusableCBF
class file for scala.collection.generic.GenTraversableFactory$ReusableCBF not found
    x.ReusableCBF();
                 ^
1 error

so javac doesn't crash but fails to find a method that is defined in class file:

mac-grek:generic grek$ javap GenTraversableFactory | grep Reusable
    public volatile scala.collection.generic.GenTraversableFactory$ReusableCBF$ ReusableCBF$module;
    public final scala.collection.generic.GenTraversableFactory$ReusableCBF$ ReusableCBF();

Iulian proposed following work-around:

mac-grek:scalagwt-scala grek$ git diff
diff --git a/src/library/scala/collection/generic/GenTraversableFactory.scala b/src/library/scala/collection/generic/GenTraversableFactory.scala
index 4695f7b..c5c9525 100644
--- a/src/library/scala/collection/generic/GenTraversableFactory.scala
+++ b/src/library/scala/collection/generic/GenTraversableFactory.scala
@@ -38,9 +38,11 @@ abstract class GenTraversableFactory[CC[X] <: GenTraversable[X] with GenericTrav
   
   // A default implementation of GenericCanBuildFrom which can be cast
   // to whatever is desired.
-  private[collection] object ReusableCBF extends GenericCanBuildFrom[Nothing] {
+  private[collection] class reusableCBF extends GenericCanBuildFrom[Nothing] {
     override def apply() = newBuilder[Nothing]
   }
+  
+  lazy val ReusableCBF = new reusableCBF
 
   /** A generic implementation of the `CanBuildFrom` trait, which forwards
    *  all calls to `apply(from)` to the `genericBuilder` method of

I can confirm that it works.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 11, 2011

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 11, 2011

@dcsobral said:
Is it worth the trouble to make it {{lazy}}? Only objects extend this class, which already provides an element of laziness. And since this {{CBF}} is used so much, I'd rather not see Scala going through unnecessary hoops.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 11, 2011

@gkossakowski said:
I tried to preserve semantics of the original code as much as I could. I'm not an expert in this area to judge what's the best solution.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 11, 2011

@hubertp said:
That's strange. ReusableCBF is a nested object, so in practice after Refchecks it is translated exactly to what you are proposing (i.e. nested objects are implemented using lazy vals).
So if this case is problematic then any other usage of nested objects in the library triggers the same situation.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 11, 2011

@paulp said:
The error message isn't very helpful: in case anyone is misled, the signature is syntactically valid. Position 56 is the . after the type parameters. Here is some java code which creates a similar signature.

public class J<CC> {
  public class Inner { }
}

class K<CC> {
  // (LJ<TCC;>.Inner;)V;
  public void f(J<CC>.Inner param) { }
}

The example with javac fails because

GenTraversableFactory$ReusableCBF$ // this exists
GenTraversableFactory$ReusableCBF // this doesn't.

And indeed the accessor is pointing at the one which doesn't exist:

private volatile scala.collection.generic.GenTraversableFactory$ReusableCBF$ ReusableCBF$module;
public final scala.collection.generic.GenTraversableFactory$ReusableCBF ReusableCBF();

Changing it to a lazy val fixes it not because the signature changes but because GenTraversableFactory$ReusableCBF is generated.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 11, 2011

@paulp said:
I see you covered a lot of that in the report, sorry, I kind of read backward from hubert's comment.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 11, 2011

@hubertp said:
Yeah, and presentation compiler runs only until Typer, so it has no chance of getting the right accessor unless we separate some of the logic from refchecks (including some valuable error checking that is there) and include it in PC.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 12, 2011

@paulp said:
I committed the lazy val workaround yesterday.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 13, 2011

@gkossakowski said:
The same issue with immutable.RedBlack. Just create following file in Eclipse:

import scala.collection.immutable.RedBlack;

public class Test {}

and you'll get following error message:
Multiple markers at this line
- The class file RedBlack contains a signature '()Lscala/collection/immutable/RedBlack<TA;>.Empty;' ill-formed at
position 44
- The class file RedBlack contains a signature '()Lscala/collection/immutable/RedBlack<TA;>.Empty;' ill-formed at
position 44

I believe there is other kind of broken signature (probably unrelated to this ticket) but it's hard to check it until this issue is fixed.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 19, 2011

@paulp said:
I think this is fixed, possibly not for all cases, in r25326. I don't have time right now to do a lot more investigating, so if anyone wants to give me an update on what's still broken it would help.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 19, 2011

@gkossakowski said:
Paul, I'm currently trying to setup jenkis job for that, here:

https://scala-webapps.epfl.ch/jenkins/view/Misc/job/scala-signatures/

Will post update specific to this issue and will keep general discussion on internals mailing list.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 19, 2011

@paulp said:
I find that "badsigs" still reports the same number of errors, seems no happier, but now I have less idea what the problem is. I'm pretty sure that fix is right regardless of whether we have yet made ecj happy.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 19, 2011

@soc said:
Maybe it is a mistake on ecj's side?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 19, 2011

@paulp said:
I am open to this possibility, and it would certainly be convenient, but for now evidence and intuition both suggest it is us.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 9, 2011

Commit Message Bot (anonymous) said:
(grek in r25639) Fix various InnerClasses bugs.

This commit fixes two major problems:

  1. InnerClasses table missed entries
    that would close the chain between
    nested and top-level class.

  2. In some situations, classes
    corresponding to objects would be
    not be reported in the InnerClasses
    table.

For details it's the best to check #4819, #4820 and
#4983.

First problem mentioned above was straightforward to
fix so I won't be going into details.

The second one deserves more attention. From now, classes
corresponding to objects are properly reported as inner
classes. Also, members (classes, objects) of objects are
reported as inner classes of classes corresponding to
objects.

There's one caveat though: top level objects get two
classes (regular and mirror). Members of top-level
objects are declared as inner classes of mirror class
and not regular one. The reason for that is to allow
importing them from Java. For example:

object A { class B }

will be compiled into following classes: A, A$, A$B.
If we declared A$B as inner class of A$ (regular class
for objects) then it would be impossible to import
B using "import A.B" or "import A$.B" constructs. The
reason for that is that Java compiler seems to blindly
put dollars instead of looking at InnerClasses attribute.

Since non-top-level objects don't have a mirror class
it's impossible to use the same solution. Thus, in case
like this:

object A { object B { class C } }

it's impossible to import C from Java. That's the tradeoff
for fixing other (more serious) problems. It's never been
possible to do that in a clean way so we are not making
situation worse.

As a nice consequence of this change, we get better way to
refer to inner members of top-level objects. It's been
reflected in one of test-cases that is updated by this
change.

Fixes #4789 #4819 #4820 #4983 and possibly some
other tickets related to reflection.

Review by extempore, dragos.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.