Skip to content

Conversation

@noti0na1
Copy link
Member

This PR should fix issue #6868. My another PR #6893 is also blocked by this issue.

When parsing an annotation class in ClassfileParser, if some of its paramter types are its inner classes, a CyclicReferenceInvolving is thrown.

Here is a small example:

MyScala.scala:

object MyScala {
  def a(mj: MyJava): Unit = {
    println("MyJava")
  }
}

MyJava.java:

public @interface MyJava {
    public MyClassType type();
    public @interface MyClassType { }
}

The output is:

> javac MyJava.java
> ./bin/dotc -explain MyScala.scala
exception caught when loading class MyClassType: CyclicReferenceInvolving(class MyClassType)
exception caught when loading class MyJava: CyclicReferenceInvolving(class MyClassType)
-- [E046] Cyclic Error: MyScala.scala:2:12 -------------------------------------
2 |  def a(mj: MyJava): Unit = {
  |            ^
  |            Cyclic reference involving class MyClassType

Explanation
===========
class MyClassType is declared as part of a cycle which makes it impossible for the
compiler to decide upon MyClassType's type.
To avoid this error, try giving MyClassType an explicit type.


one error found

The issue is when the parser tries to get the type of a paramter, a cyclic reference will be detected as the parent relationship information has not been set up.

By moving adding annotation constructor after the setting classInfo, the annotation class can be parsed correctly.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

  1. Separate subject from body with a blank line
  2. When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:
  3. Limit the subject line to 72 characters
  4. Capitalize the subject line
  5. Do not end the subject line with a period
  6. Use the imperative mood in the subject line ("Add" instead of "Added")
  7. Wrap the body at 80 characters
  8. Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

@noti0na1 noti0na1 changed the title Fix #6868, Move Adding Annotation Constructor after Setting ClassInfo Fix #6868: Move adding annotation constructor after setting classinfo Jul 23, 2019
@smarter
Copy link
Member

smarter commented Jul 23, 2019

Thank you for working on this! However, we'll need tests before this can be merged. Just create a subdirectory inside https://github.com/lampepfl/dotty/tree/master/tests/pos-java-interop-separate and drop the java and scala files inside, make sure to call the java file something that ends with _1.java and the scala file something that ends with _2.scala to have the test infrastructure compile them separately and therefore use the ClassfileParser. To check if your test works, you can run it with testCompilation nameOfTheDirectory from sbt, see http://dotty.epfl.ch/docs/contributing/testing.html for more information.

@noti0na1
Copy link
Member Author

@smarter Thanks for your reply! I have add the test files.

@smarter smarter self-assigned this Jul 25, 2019
@smarter
Copy link
Member

smarter commented Jul 31, 2019

While your PR works, I'm not completely comfortable with it since it means the annotation class is in an inconsistent state (no constructor) between the call to setClassInfo and the call to addAnnotationConstructor, usually we avoid cyclic references by creating symbols where the info is a LazyType which will get computed on demand, I was able to apply this technique in this case after some refactoring/simplification of how we handle annotations and made a new PR (which reuses your test case): #6978. Therefore, I'm going to close this PR, but I'd like to thank you for your work on this !

@smarter smarter closed this Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants