search

MinecraftForge / Srg2Source

Applies source level refactors to java source code.
GNU Lesser General Public License v2.1
60 stars 32 forks source link

Fix package declaration mapping & add Java Version option #26

Closed phase closed 3 years ago

phase commented 5 years ago

Files were being moved to the correct directory but the package declarations at the top weren't changing. This will assume all package declarations in a file should be changed to the new package we're mapping to.

I also added a Java Version option to the Extractor because the Spigot codebase uses Java 8. It defaults to Java 6.

LexManos commented 5 years ago

Provide tests. Also, probably worth porting the console handlers to JOptSimple.

phase commented 5 years ago

All of the SimpleTests are failing for me, and MinecraftTest is specific to (I'm assuming) your environment.

The SimpleTests have a pattern for the failures. They're expecting range maps for classes in this format:

      @|/AnonClass.java|42|48|Nested|class|AnonClass.Nested
      @|/AnonClass.java|71|77|Object|class|java.lang.Object
      @|/AnonClass.java|71|77|Object|class|java.lang.Object

but S2S is producing:

      @|/AnonClass.java|42|48|Nested|class|AnonClass.Nested|false
      @|/AnonClass.java|71|77|Object|class|java.lang.Object|false
      @|/AnonClass.java|71|77|Object|class|java.lang.Object|false

which includes whether the class is qualified. Updating these tests is trivial except for testLocalClass, which gives the error:

java.lang.AssertionError: Empty field found in line: @|/LocalClass.java|112|117|Local|class||false

    at net.minecraftforge.srg2source.ast.SymbolRangeEmitter.log(SymbolRangeEmitter.java:446)
    at net.minecraftforge.srg2source.ast.SymbolRangeEmitter.emitReferencedClass(SymbolRangeEmitter.java:272)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.visit(SymbolReferenceWalker.java:329)
    at org.eclipse.jdt.core.dom.SimpleName.accept0(SimpleName.java:195)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.walk(SymbolReferenceWalker.java:64)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.visit(SymbolReferenceWalker.java:450)
    at org.eclipse.jdt.core.dom.TypeDeclaration.accept0(TypeDeclaration.java:427)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at org.eclipse.jdt.core.dom.ASTNode.acceptChild(ASTNode.java:2845)
    at org.eclipse.jdt.core.dom.TypeDeclarationStatement.accept0(TypeDeclarationStatement.java:196)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at org.eclipse.jdt.core.dom.ASTNode.acceptChildren(ASTNode.java:2868)
    at org.eclipse.jdt.core.dom.Block.accept0(Block.java:125)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.walk(SymbolReferenceWalker.java:64)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.visit(SymbolReferenceWalker.java:311)
    at org.eclipse.jdt.core.dom.MethodDeclaration.accept0(MethodDeclaration.java:590)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.walk(SymbolReferenceWalker.java:86)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.visit(SymbolReferenceWalker.java:454)
    at org.eclipse.jdt.core.dom.TypeDeclaration.accept0(TypeDeclaration.java:427)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.walk(SymbolReferenceWalker.java:86)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.visit(SymbolReferenceWalker.java:454)
    at org.eclipse.jdt.core.dom.TypeDeclaration.accept0(TypeDeclaration.java:427)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at org.eclipse.jdt.core.dom.ASTNode.acceptChildren(ASTNode.java:2868)
    at org.eclipse.jdt.core.dom.CompilationUnit.accept0(CompilationUnit.java:255)
    at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2797)
    at net.minecraftforge.srg2source.ast.SymbolReferenceWalker.walk(SymbolReferenceWalker.java:64)
    at net.minecraftforge.srg2source.ast.RangeExtractor.generateRangeMap(RangeExtractor.java:182)
    at net.minecraftforge.srg2source.test.SingleTests.testClass(SingleTests.java:93)
    at net.minecraftforge.srg2source.test.SingleTests.testClass(SingleTests.java:74)
    at net.minecraftforge.srg2source.test.SingleTests.testLocalClass(SingleTests.java:51)

which is a sign of the range extractor having a bug. clazz.getErasure().getQualifiedName() here is returning an empty string for some reason... I'm looking for the cause now.

phase commented 5 years ago

This looks like it's failing with all local class declarations. The test has this hierarchy: 

class LocalClass
  class Nested
    fun test
      class Local

I printed out the values of clazz.getName(), clazz.getQualifiedName(), & clazz.getErasure().getQualifiedName():

LocalClass / LocalClass / LocalClass
Nested / LocalClass.Nested / LocalClass.Nested
Local /  /

I'm not sure why JDT doesn't have a qualified name for local classes. Maybe because they're not really a part of the parent class? I think the simplest solution would be to not emit class declarations that are local, but if you want to be feature complete we could append the qualified name of the parent class to the front.

Interestingly enough, if you have the tree

class LocalClass
  class Nested
    fun test
      class Local extends Nested

The second Nested use is caught and handled correctly.

This isn't a huge deal. I'm guessing local classes aren't used much in Forge, and I only saw one instance of it in the Spigot source (which had a local class that extended a class that needed to be remapped).