Clean up Java support in metacp #1367

olafurpg · 2018-02-26T17:08:16Z

Partially addressed the checkboxes in #1362.

remove global mutable scope
skip anonymous classes
merge inner classes with outer classes
add p.STATIC
run javacp against JDK in tests

* remove this$0 fields for inner classes * merge inner classes with their outer class files * skip anonymous classes

ClassNode.access for inner classes does not contain whether a class is static or not, it's only available through ClassNode.innerClasses for the outer class.

xeno-by · 2018-02-26T17:16:50Z

semanticdb/integration/src/main/java/com/javacp/Javacp.java

@@ -59,4 +59,7 @@ void packagePrivateMethod() { }

    public Serializable anonymous = new Serializable() { };

+    static void staticMethod() {}
+    static class StaticClass {}


Let's test a static field too, if you don't mind.

xeno-by · 2018-02-26T17:17:55Z

tests/jvm/src/test/resources/metacp.expect

@@ -36,73 +36,6 @@ _empty_.B#`<init>`(). => primaryctor <init>: (): B
 _empty_.B#a(). => def a: : A
  A => _empty_.A#

-com/javacp/ClassSuffix$Inner$Bar$Fuz.class
------------------------------------------


Nested classes are now stored in their owner's semanticdb files, right?

Correct, these entries are included below see https://github.com/scalameta/scalameta/pull/1367/files#diff-026fd03a8437acdb4213e4e80bf077d1R71

xeno-by · 2018-02-26T17:24:38Z

tests/jvm/src/test/resources/metacp.expect


 Symbols:
 _root_. => package _root_
 _root_.com. => package com
 _root_.com.javacp. => package javacp
-_root_.com.javacp.Javacp# => class Javacp[A <: CharSequence with Serializable, B <: Object].{+30 decls}
+_root_.com.javacp.Javacp# => class Javacp[A <: CharSequence with Serializable, B <: Object].{+31 decls}


Why is it just +1 decl when you added both a static class and a static method?

Good catch, I've updated the logic to include direct inner classes as ClassInfo.declarations.

xeno-by · 2018-02-26T17:31:50Z

tests/jvm/src/test/resources/metacp.expect

-  extends Serializable
-_root_.com.javacp.Javacp#1#<init>(Javacp). => private[javacp] def <init>: (arg0: Javacp): Unit
-  arg0 => _root_.com.javacp.Javacp#1#<init>(Javacp).(arg0)
+_root_.com.javacp.ClassSuffix#Inner#<init>(ClassSuffix). => private[javacp] def <init>: (arg0: ClassSuffix): Unit


I like that this$N outer fields no longer show up in SemanticDB. Can we also filter out the corresponding outer parameters from inner class constructors? The heuristic would be "if the class contains a field such that field.name.startsWith("this$"), then drop the first parameters of the constructor".

xeno-by · 2018-02-26T17:33:57Z

semanticdb/metacp/src/main/scala/scala/meta/internal/metacp/Main.scala

+            .filter { f =>
+              !isVisited(f) &&
+              Files.isRegularFile(f)
+            }


Do we still need sorting? If not, let's go back to having all logic in visitFile.

We still need sorting in order to ensure we visit Foo.class before Foo$Inner.class. I propose we keep this unchanged for now.

* avoid duplicate addInfo for _root_ package * add direct inner classes to ClasInfo.declarations

It was reporting parse errors for the javacp test files.

olafurpg

Thanks for the review!

olafurpg · 2018-02-26T18:31:59Z

semanticdb/integration/src/main/java/com/javacp/Javacp.java

@@ -59,4 +59,7 @@ void packagePrivateMethod() { }

    public Serializable anonymous = new Serializable() { };

+    static void staticMethod() {}
+    static class StaticClass {}


olafurpg · 2018-02-26T18:34:42Z

semanticdb/metacp/src/main/scala/scala/meta/internal/metacp/Main.scala

+            .filter { f =>
+              !isVisited(f) &&
+              Files.isRegularFile(f)
+            }


We still need sorting in order to ensure we visit Foo.class before Foo$Inner.class. I propose we keep this unchanged for now.

olafurpg · 2018-02-26T19:15:05Z

tests/jvm/src/test/resources/metacp.expect

@@ -36,73 +36,6 @@ _empty_.B#`<init>`(). => primaryctor <init>: (): B
 _empty_.B#a(). => def a: : A
  A => _empty_.A#

-com/javacp/ClassSuffix$Inner$Bar$Fuz.class
------------------------------------------


Correct, these entries are included below see https://github.com/scalameta/scalameta/pull/1367/files#diff-026fd03a8437acdb4213e4e80bf077d1R71

olafurpg · 2018-02-26T19:32:47Z

tests/jvm/src/test/resources/metacp.expect

-  extends Serializable
-_root_.com.javacp.Javacp#1#<init>(Javacp). => private[javacp] def <init>: (arg0: Javacp): Unit
-  arg0 => _root_.com.javacp.Javacp#1#<init>(Javacp).(arg0)
+_root_.com.javacp.ClassSuffix#Inner#<init>(ClassSuffix). => private[javacp] def <init>: (arg0: ClassSuffix): Unit


olafurpg · 2018-02-26T19:33:14Z

tests/jvm/src/test/resources/metacp.expect


 Symbols:
 _root_. => package _root_
 _root_.com. => package com
 _root_.com.javacp. => package javacp
-_root_.com.javacp.Javacp# => class Javacp[A <: CharSequence with Serializable, B <: Object].{+30 decls}
+_root_.com.javacp.Javacp# => class Javacp[A <: CharSequence with Serializable, B <: Object].{+31 decls}


Good catch, I've updated the logic to include direct inner classes as ClassInfo.declarations.

While running javacp against the JDK: ``` error: can't convert /java/util/regex/Pattern.class java.lang.UnsupportedOperationException: tail of empty list at scala.collection.immutable.Nil$.tail(List.scala:430) ``` Happens while trying to drop of the first parameter for outer class reference to an inner class ctor.

olafurpg added 7 commits February 26, 2018 15:21

Address feedback

3ac3e58

* remove this$0 fields for inner classes * merge inner classes with their outer class files * skip anonymous classes

Remove global mutable scope environment

787e525

Silence ScopeResolutionErrors

f3a5d94

Handle java/lang/Object superName == null

82d0781

Add p.Static.

e22deb4

ClassNode.access for inner classes does not contain whether a class is static or not, it's only available through ClassNode.innerClasses for the outer class.

Tell IntelliJ to not group imports by prefix

e907d62

Refactor Javacp

9062a8c

xeno-by added the Scalacp label Feb 26, 2018

xeno-by self-requested a review February 26, 2018 17:44

xeno-by self-assigned this Feb 26, 2018

xeno-by modified the milestones: 3.3.2, 3.4.0 Feb 26, 2018

xeno-by requested changes Feb 26, 2018

View reviewed changes

olafurpg added 3 commits February 26, 2018 20:14

Address review and further cleanup

908bba9

* avoid duplicate addInfo for _root_ package * add direct inner classes to ClasInfo.declarations

Drop <init> parameter with reference to outer class

cba6d69

Ignore *.java files from ./bin/scalafmt.

802808e

It was reporting parse errors for the javacp test files.

olafurpg commented Feb 26, 2018

View reviewed changes

xeno-by approved these changes Feb 26, 2018

View reviewed changes

xeno-by merged commit 62315d8 into scalameta:master Feb 26, 2018

xeno-by mentioned this pull request Feb 27, 2018

Further cleanups in Java support in metacp #1373

Merged

olafurpg mentioned this pull request Feb 27, 2018

Assorted fixes in metacp #1363

Merged

olafurpg changed the title ~~Follow up from #1359~~ Clean up Java support in metacp #1359 Feb 28, 2018

olafurpg changed the title ~~Clean up Java support in metacp #1359~~ Clean up Java support in metacp Feb 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up Java support in metacp #1367

Clean up Java support in metacp #1367

olafurpg commented Feb 26, 2018 •

edited

xeno-by Feb 26, 2018

olafurpg Feb 26, 2018

xeno-by Feb 26, 2018

olafurpg Feb 26, 2018

xeno-by Feb 26, 2018

olafurpg Feb 26, 2018

xeno-by Feb 26, 2018

olafurpg Feb 26, 2018

xeno-by Feb 26, 2018

olafurpg Feb 26, 2018

olafurpg left a comment

olafurpg Feb 26, 2018

olafurpg Feb 26, 2018

olafurpg Feb 26, 2018

olafurpg Feb 26, 2018

olafurpg Feb 26, 2018

Clean up Java support in metacp #1367

Clean up Java support in metacp #1367

Conversation

olafurpg commented Feb 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Feb 26, 2018 •

edited