Merge pull request #299 from thiakil/validation-changes

Validation improvements

7 files changed, 258 insertions, 27 deletions

diff --git a/enigma/build.gradle b/enigma/build.gradle
index 7cebcd16..8d081457 100644
--- a/enigma/build.gradle
+++ b/enigma/build.gradle
@@ -53,7 +53,7 @@ file('src/test/java/cuchaz/enigma/inputs').listFiles().each { theFile ->
 
             args '@src/test/resources/proguard-test.conf', '-injars', file('build/test-inputs/' +
                     "${theFile.name}.jar"), '-libraryjars', libraryJarsArg,
-                    '-outjars', file('build/test-obf/' + "${theFile.name}.jar")
+                    '-outjars', file('build/test-obf/' + "${theFile.name}.jar"), '-printmapping', file("build/test-obf/${theFile.name}-mapping.txt")
         }
 
         test.dependsOn "${theFile.name}TestObf"
diff --git a/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java b/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java
index f9f3b88e..134c05ce 100644
--- a/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java
+++ b/enigma/src/main/java/cuchaz/enigma/translation/mapping/MappingValidator.java
@@ -1,15 +1,19 @@
 package cuchaz.enigma.translation.mapping;
 
 import java.util.Collection;
-import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 import cuchaz.enigma.analysis.index.InheritanceIndex;
 import cuchaz.enigma.analysis.index.JarIndex;
 import cuchaz.enigma.translation.Translator;
 import cuchaz.enigma.translation.mapping.tree.EntryTree;
+import cuchaz.enigma.translation.representation.AccessFlags;
 import cuchaz.enigma.translation.representation.entry.ClassEntry;
+import cuchaz.enigma.translation.representation.entry.DefEntry;
 import cuchaz.enigma.translation.representation.entry.Entry;
+import cuchaz.enigma.translation.representation.entry.FieldEntry;
 import cuchaz.enigma.utils.validation.Message;
 import cuchaz.enigma.utils.validation.ValidationContext;
 
@@ -35,18 +39,16 @@ public class MappingValidator {
 
         private void validateUnique(ValidationContext vc, Entry<?> entry, String name) {
                 ClassEntry containingClass = entry.getContainingClass();
-                Collection<ClassEntry> relatedClasses = getRelatedClasses(containingClass);
-
-                for (ClassEntry relatedClass : relatedClasses) {
-                        Entry<?> relatedEntry = entry.replaceAncestor(containingClass, relatedClass);
-                        Entry<?> translatedEntry = deobfuscator.translate(relatedEntry);
-
-                        Collection<Entry<?>> translatedSiblings = obfToDeobf.getSiblings(relatedEntry).stream()
-                                        .map(deobfuscator::translate)
-                                        .collect(Collectors.toList());
+                InheritanceIndex inheritanceIndex = index.getInheritanceIndex();
 
-                        if (!isUnique(translatedEntry, translatedSiblings, name)) {
-                                Entry<?> parent = translatedEntry.getParent();
+                //Step 1, check it's unique within its own siblings
+                Collection<Entry<?>> directTranslatedSiblings = obfToDeobf.getSiblings(entry).stream()
+                                .map(deobfuscator::translate)
+                                .collect(Collectors.toList());
+                for (Entry<?> sibling : directTranslatedSiblings) {
+                        if (entry.canConflictWith(sibling) && sibling.getName().equals(name) && isSynthetic(entry) == isSynthetic(sibling)) {
+                                // allow clash if one is synthetic and the other is not
+                                Entry<?> parent = entry.getParent();
                                 if (parent != null) {
                                         vc.raise(Message.NONUNIQUE_NAME_CLASS, name, parent);
                                 } else {
@@ -54,26 +56,88 @@ public class MappingValidator {
                                 }
                         }
                 }
-        }
 
-        private Collection<ClassEntry> getRelatedClasses(ClassEntry classEntry) {
-                InheritanceIndex inheritanceIndex = index.getInheritanceIndex();
+                //Step 2, check ancestors, ignoring members invisible to children
+                Set<ClassEntry> ancestors = inheritanceIndex.getAncestors(containingClass);
+                for (ClassEntry ancestor : ancestors) {
+                        Entry<?> reparentedEntry = entry.replaceAncestor(containingClass, ancestor);
+                        Entry<?> translatedEntry = Objects.requireNonNull(deobfuscator.translate(reparentedEntry), "Translation failed");
+                        Collection<Entry<?>> translatedSiblings = obfToDeobf.getSiblings(reparentedEntry).stream()
+                                        .filter(it->!entry.equals(it))//e.g. for root classes, ensure we dont match the name against itself
+                                        .filter(this::isVisibleToChildren)
+                                        .collect(Collectors.toList());
+                        for (Entry<?> parentSibling : translatedSiblings) {
+                                Entry<?> parentSiblingTranslated = Objects.requireNonNull(deobfuscator.translate(parentSibling), "Translation failed");
+                                if (translatedEntry.canConflictWith(parentSiblingTranslated) && parentSiblingTranslated.getName().equals(name) && !isAcceptableOverride(parentSibling, entry)) {
+                                        Entry<?> parent = translatedEntry.getParent();
+                                        if (parent != null) {
+                                                vc.raise(Message.NONUNIQUE_NAME_CLASS, name, parent);
+                                        } else {
+                                                vc.raise(Message.NONUNIQUE_NAME, name);
+                                        }
+                                }
+                        }
+                }
 
-                Collection<ClassEntry> relatedClasses = new HashSet<>();
-                relatedClasses.add(classEntry);
-                relatedClasses.addAll(inheritanceIndex.getChildren(classEntry));
-                relatedClasses.addAll(inheritanceIndex.getAncestors(classEntry));
+                //Step 3, if this entry is visible to children, see if it clashes with any of their names
+                if (isVisibleToChildren(entry)) {
+                        Collection<ClassEntry> children = inheritanceIndex.getDescendants(containingClass);
+                        for (ClassEntry child : children) {
+                                Entry<?> reparentedEntry = entry.replaceAncestor(containingClass, child);
+                                Entry<?> translatedEntry = Objects.requireNonNull(deobfuscator.translate(reparentedEntry), "Translation failed");
+                                Collection<Entry<?>> siblings = obfToDeobf.getSiblings(reparentedEntry).stream()
+                                                .filter(it->!entry.equals(it))//e.g. for root classes, ensure we dont match the name against itself
+                                                .collect(Collectors.toList());
+                                for (Entry<?> childSibling : siblings) {
+                                        Entry<?> childSiblingTranslated = Objects.requireNonNull(deobfuscator.translate(childSibling), "Translation failed");
+                                        if (translatedEntry.canConflictWith(childSiblingTranslated) && childSiblingTranslated.getName().equals(name) && !isAcceptableOverride(entry, childSibling)) {
+                                                Entry<?> parent = translatedEntry.getParent();
+                                                if (parent != null) {
+                                                        vc.raise(Message.NONUNIQUE_NAME_CLASS, name, parent);
+                                                } else {
+                                                        vc.raise(Message.NONUNIQUE_NAME, name);
+                                                }
+                                        }
+                                }
+                        }
+                }
+        }
 
-                return relatedClasses;
+        private boolean isVisibleToChildren(Entry<?> entry) {
+                if (entry instanceof DefEntry) {
+                        return !((DefEntry<?>) entry).getAccess().isPrivate();
+                }
+                AccessFlags accessFlags = index.getEntryIndex().getEntryAccess(entry);
+                if (accessFlags != null) {
+                        return !accessFlags.isPrivate();
+                }
+                return true;//unknown, assume yes
         }
 
-        private boolean isUnique(Entry<?> entry, Collection<Entry<?>> siblings, String name) {
-                for (Entry<?> sibling : siblings) {
-                        if (entry.canConflictWith(sibling) && sibling.getName().equals(name)) {
-                                return false;
-                        }
+        private boolean isAcceptableOverride(Entry<?> ancestor, Entry<?> descendent) {
+                if (ancestor instanceof FieldEntry && descendent instanceof FieldEntry){
+                        return true;//fields don't apply here
+                }
+
+                AccessFlags ancestorFlags = findAccessFlags(ancestor);
+                AccessFlags descendentFlags = findAccessFlags(descendent);
+
+                if (ancestorFlags == null || descendentFlags == null) {
+                        return false;//we can't make any assumptions
                 }
-                return true;
+
+                //bad == accessLevel < superAccessLevel
+                return !(descendentFlags.getAccessLevel() < ancestorFlags.getAccessLevel());
+        }
+
+        private boolean isSynthetic(Entry<?> entry) {
+                AccessFlags accessFlags = findAccessFlags(entry);
+                return accessFlags != null && accessFlags.isSynthetic();
+        }
+
+        private AccessFlags findAccessFlags(Entry<?> entry) {
+                return (entry instanceof DefEntry) ? ((DefEntry<?>) entry).getAccess() : index.getEntryIndex()
+                                .getEntryAccess(entry);
         }
 
 }
diff --git a/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java b/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java
index b280eef2..aa48a5b2 100644
--- a/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java
+++ b/enigma/src/main/java/cuchaz/enigma/translation/representation/AccessFlags.java
@@ -8,6 +8,10 @@ import java.lang.reflect.Modifier;
 public class AccessFlags {
         public static final AccessFlags PRIVATE = new AccessFlags(Opcodes.ACC_PRIVATE);
         public static final AccessFlags PUBLIC = new AccessFlags(Opcodes.ACC_PUBLIC);
+        public static final int ACCESS_LEVEL_PUBLIC = 4;
+        public static final int ACCESS_LEVEL_PROTECTED = 3;
+        public static final int ACCESS_LEVEL_PACKAGE_LOCAL = 2;
+        public static final int ACCESS_LEVEL_PRIVATE = 1;
 
         private int flags;
 
@@ -89,6 +93,23 @@ public class AccessFlags {
                 return this.flags;
         }
 
+        /**
+         * Adapted from https://github.com/JetBrains/intellij-community/blob/6472c347db91d11bbf02895a767198f9d884b119/java/java-psi-api/src/com/intellij/psi/util/PsiUtil.java#L389
+         * @return visibility access level on a 'weakness scale'
+         */
+        public int getAccessLevel() {
+                if (isPrivate()) {
+                        return ACCESS_LEVEL_PRIVATE;
+                }
+                if (isProtected()) {
+                        return ACCESS_LEVEL_PROTECTED;
+                }
+                if (isPublic()) {
+                        return ACCESS_LEVEL_PUBLIC;
+                }
+                return ACCESS_LEVEL_PACKAGE_LOCAL;
+        }
+
         @Override
         public boolean equals(Object obj) {
                 return obj instanceof AccessFlags && ((AccessFlags) obj).flags == flags;
diff --git a/enigma/src/test/java/cuchaz/enigma/TestAllowableClashes.java b/enigma/src/test/java/cuchaz/enigma/TestAllowableClashes.java
new file mode 100644
index 00000000..bfc21de3
--- /dev/null
+++ b/enigma/src/test/java/cuchaz/enigma/TestAllowableClashes.java
@@ -0,0 +1,52 @@
+package cuchaz.enigma;
+
+import cuchaz.enigma.classprovider.ClasspathClassProvider;
+import cuchaz.enigma.translation.mapping.EntryMapping;
+import cuchaz.enigma.translation.mapping.EntryRemapper;
+import cuchaz.enigma.translation.mapping.serde.MappingFormat;
+import cuchaz.enigma.translation.mapping.serde.MappingParseException;
+import cuchaz.enigma.translation.mapping.tree.EntryTree;
+import cuchaz.enigma.translation.mapping.tree.EntryTreeNode;
+import cuchaz.enigma.translation.representation.entry.MethodEntry;
+import cuchaz.enigma.utils.validation.ValidationContext;
+import org.hamcrest.MatcherAssert;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.nio.file.Paths;
+
+/**
+ * Test that we can accept some name clashes that are allowed by javac
+ */
+public class TestAllowableClashes {
+
+        private static final String inputBaseName = "build/test-obf/visibility";
+
+        @Test
+        public void test() throws IOException, MappingParseException {
+                //Load produced mappings
+                Enigma enigma = Enigma.create();
+                EnigmaProject project = enigma.openJar(Paths.get(inputBaseName + ".jar"), new ClasspathClassProvider(), ProgressListener.none());
+                EntryTree<EntryMapping> obfToDeobf = MappingFormat.PROGUARD.read(Paths.get(inputBaseName + "-mapping.txt"), ProgressListener.none(), null);
+
+                //Load them into enigma, none should conflict
+                EntryRemapper mapper = project.getMapper();
+                for (int round=0; round<2; round++) {
+                        for (EntryTreeNode<EntryMapping> node : obfToDeobf) {
+                                Assert.assertNotEquals(null, node.getValue());
+                                if (node.getEntry() instanceof MethodEntry && (node.getEntry()
+                                                .getName()
+                                                .equals("<init>") || node.getEntry().getName().equals("<clinit>"))) {
+                                        //skip proguard's constructor entries
+                                        continue;
+                                }
+                                System.out.println(node.getEntry().toString() + " -> " + node.getValue().getTargetName());
+                                ValidationContext vc = new ValidationContext();
+                                mapper.mapFromObf(vc, node.getEntry(), node.getValue());
+                                MatcherAssert.assertThat(vc, ValidationContextMatcher.INSTANCE);
+                        }
+                }
+        }
+
+}
diff --git a/enigma/src/test/java/cuchaz/enigma/ValidationContextMatcher.java b/enigma/src/test/java/cuchaz/enigma/ValidationContextMatcher.java
new file mode 100644
index 00000000..5404077f
--- /dev/null
+++ b/enigma/src/test/java/cuchaz/enigma/ValidationContextMatcher.java
@@ -0,0 +1,35 @@
+package cuchaz.enigma;
+
+import cuchaz.enigma.utils.validation.ParameterizedMessage;
+import cuchaz.enigma.utils.validation.ValidationContext;
+import org.hamcrest.CustomMatcher;
+import org.hamcrest.Description;
+
+class ValidationContextMatcher extends CustomMatcher<ValidationContext> {
+        public static final ValidationContextMatcher INSTANCE = new ValidationContextMatcher();
+
+        private ValidationContextMatcher() {
+                super("ValidationContext can proceed");
+        }
+
+        @Override
+        public boolean matches(Object item) {
+                return item instanceof ValidationContext && ((ValidationContext) item).canProceed();
+        }
+
+        @Override
+        public void describeMismatch(Object item, Description description) {
+                if (!(item instanceof ValidationContext)) {
+                        description.appendText("expected ValidationContext, was").appendValue(item);
+                        return;
+                }
+                ValidationContext vc = (ValidationContext) item;
+                for (ParameterizedMessage message : vc.getMessages()) {
+                        description.appendText(message.getText());
+                        String longMessage = message.getLongText();
+                        if (longMessage != null && !longMessage.trim().isEmpty()){
+                                description.appendText(longMessage);
+                        }
+                }
+        }
+}
diff --git a/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassA.java b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassA.java
new file mode 100644
index 00000000..458f7c0d
--- /dev/null
+++ b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassA.java
@@ -0,0 +1,28 @@
+package cuchaz.enigma.inputs.visibility;
+
+class ClassA {
+
+        protected Object protectedParentPrivateChild;
+        public Object publicParentPrivateChild;
+
+        public static Object LOGGER = null;
+
+        protected static Object LOGGER2 = null;
+
+        public Object publicPublic;
+
+        public static void equalAccessStatic() {}
+
+        protected static void protectedPublicStatic(){}
+
+        private static void privateStaticParentPublicStaticChild(){}
+
+        private void privateParentPublicStaticChild() {}
+
+        static void packagePrivateParentProtectedChild(){}
+
+        private static void packagePrivateChild(){}
+
+        public ClassA returningSubclass(){return null;}
+
+}
\ No newline at end of file
diff --git a/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassB.java b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassB.java
new file mode 100644
index 00000000..2a1b68a7
--- /dev/null
+++ b/enigma/src/test/java/cuchaz/enigma/inputs/visibility/ClassB.java
@@ -0,0 +1,31 @@
+package cuchaz.enigma.inputs.visibility;
+
+public class ClassB extends ClassA {
+        private Object protectedParentPrivateChild;
+
+        private Object publicParentPrivateChild;
+
+        public Object publicPublic;
+
+        public static Object LOGGER;
+
+        public static Object LOGGER2 = null;
+
+        public static void equalAccessStatic() {
+        }
+
+        public static void protectedPublicStatic() {
+        }
+
+        public static void privateStaticParentPublicStaticChild() {
+        }
+
+        public static void privateParentPublicStaticChild() {
+        }
+
+        protected static void packagePrivateParentProtectedChild(){}
+
+        static void packagePrivateChild(){}
+
+        public ClassB returningSubclass(){return null;}
+}