Add support for local types (fix for #14 and #16)

MinecraftForge / Srg2Source

Applies source level refactors to java source code.

GNU Lesser General Public License v2.1

60 stars 32 forks source link

Add support for local types (fix for #14 and #16) #17

Closed boq closed 7 years ago

boq commented 7 years ago

Without this change declaration of local class triggers call to System.exit(1). This PR:

replaces System.exit(1) call with error throw, to simplify debugging (AssertionError is used, since there is no point in catching this)
fixes generation of local type name (since they don't have qualified names). This is bit of guesswork based on anonymous class names, since I can't find any example of such names in SRG files. Which is also good thing, because it will not break any existing builds (verified by running MinecraftTest). Nested classes inside local types are not considered (though they look like pkg.Outer$1Local.Inner). Maintaners: Are there any rules for naming such cases in SRG files?

Pushing as three commits to make review/commenting easier, will squash if accepted.

mezz commented 7 years ago

@LexManos can you look this over? This bug broke a PR earlier today, and it was tough to figure out since the gradle daemon just kills itself.

mezz commented 7 years ago

@boq what needs to be researched so we can take out the guesswork and be confident in these changes?

boq commented 7 years ago

There are no known examples of local classes in MC codebase, so I wasn't sure how to name them.

My proposal generates something like pkgA.pkgB.Class.Inner$1Local, i.e. package part with dots, and . instead of $ to separate named inner classes from top-level. This is neither qualified name (since both anonymous and local classes don't have one - you can't use them in import statement) nor binary name (which would be pkgA/pkgB/Class$Inner$1Local), but concatenation of both.

This was based on existing scheme for anonymous ones (pkgA.pkgB.Class.Inner$1 vs pkgA.pkgB.Class$Inner$1). Extra substring weirdness is workaround for local classes with $ in simple name.

Is see you have referenced this issue from Forestry. I'm not sure if it's same issue - anonymous inner classes were already explicitly supported before (see here)

mezz commented 7 years ago

Thanks, I wanted to rule it out and now I agree it was not the same issue. After removing all anonymous inner classes I still had the problem. The issue does not seem to be deterministic, and it resolved itself before this PR was accepted. If it happens again I should be able to gather some more information now.