getFieldAccesses() ignores accesses from lambdas

[abeyecon]

I'm trying to create methodsWithNoInstanceAccessShouldBeStatic rule to stop developers from checking in non-static methods that should be static. However it brings up false positives when a method contains a lambda. See minimal test case below which should be green but isn't. handleEndGame should not get flagged because it has a lambda which accesses a member variable (executor). I would have expected that input.getFieldAccesses() would have included the access of executor in handleEndGame().

[abeyecon]

package scratch;

import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods;

import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.stream.Collectors;

import org.junit.runner.RunWith;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.annotation.Bean;

import com.tngtech.archunit.base.DescribedPredicate;
import com.tngtech.archunit.base.Optional;
import com.tngtech.archunit.core.domain.JavaAnnotation;
import com.tngtech.archunit.core.domain.JavaClass;
import com.tngtech.archunit.core.domain.JavaClassList;
import com.tngtech.archunit.core.domain.JavaCodeUnit;
import com.tngtech.archunit.core.domain.JavaField;
import com.tngtech.archunit.core.domain.JavaMethod;
import com.tngtech.archunit.core.domain.JavaModifier;
import com.tngtech.archunit.junit.AnalyzeClasses;
import com.tngtech.archunit.junit.ArchTest;
import com.tngtech.archunit.junit.ArchUnitRunner;
import com.tngtech.archunit.lang.ArchRule;

@RunWith(ArchUnitRunner.class)
@AnalyzeClasses(packages = "scratch")
public class ScratchTest {
    private static final Logger LOG = LoggerFactory.getLogger(ScratchTest.class);

    @ArchTest
    public static final ArchRule methodsWithNoInstanceAccessShouldBeStatic = methods()
            .that(haveImplementation())
            .and(dontAccessMemberVariables())
            .and(dontAccessInstanceMethods())
            .and(arentAnnotatedWith(new Class[] { Bean.class }))
            .and(arentInherited())
            .should().beStatic();

    private static DescribedPredicate<? super JavaMethod> arentAnnotatedWith(final Class[] classes) {
        for (final Class annotation : classes) {
            if (!annotation.isAnnotation()) {
                throw new RuntimeException(annotation.getCanonicalName() + " is not an annotation");
            }
        }
        final List<String> annotationList = Arrays.stream(classes).map((c) -> c.getCanonicalName())
                .collect(Collectors.toList());
        return new DescribedPredicate<JavaMethod>("aren't annotated with %s", classes) {
            @Override
            public boolean apply(final JavaMethod input) {
                final List<JavaAnnotation> matches = input.getAnnotations().stream()
                        .filter((annotation) -> annotationList.contains(annotation.getRawType().getFullName()))
                        .collect(Collectors.toList());
                return matches.isEmpty();
            }
        };
    }

    private static DescribedPredicate<? super JavaMethod> haveImplementation() {
        return new DescribedPredicate<JavaMethod>("have implementation") {
            @Override
            public boolean apply(final JavaMethod input) {
                return (!input.getModifiers().contains(JavaModifier.ABSTRACT)) && (!input.getOwner().isInterface());
            }
        };
    }

    private static DescribedPredicate<JavaMethod> dontAccessMemberVariables() {
        return new DescribedPredicate<JavaMethod>("don't access member variables") {
            @Override
            public boolean apply(final JavaMethod input) {
                return !input.getFieldAccesses().stream().filter(
                        (access) -> {
                            final Optional<JavaField> optionalField = access.getTarget().resolveField();
                            if (!optionalField.isPresent()) {
                                LOG.warn("Cannot resolve target for " + access.getTarget().getFullName());
                                return false;
                            }
                            return !optionalField.get().getModifiers()
                                    .contains(JavaModifier.STATIC);
                        })
                        .findAny().isPresent();
            }
        };
    };

    private static DescribedPredicate<JavaMethod> dontAccessInstanceMethods() {
        return new DescribedPredicate<JavaMethod>("don't access instance methods") {
            @Override
            public boolean apply(final JavaMethod input) {
                return !input.getCallsFromSelf().stream().filter(
                        (access) -> {
                            final Set<? extends JavaCodeUnit> targets = access.getTarget().resolve();
                            if (targets.isEmpty()) {
                                LOG.warn("Cannot resolve target for " + access.getTarget().getFullName());
                                return false;
                            }
                            final JavaCodeUnit target = targets.stream().findAny().get();
                            return target.getOwner().equals(input.getOwner())
                                    && !target.getModifiers().contains(JavaModifier.STATIC);
                        })
                        .findAny().isPresent();
            }
        };
    };

    private static DescribedPredicate<? super JavaMethod> arentInherited() {
        return new DescribedPredicate<JavaMethod>("aren't inherited") {
            @Override
            public boolean apply(final JavaMethod input) {
                return !overridesOrImplements(input.getName(), input.getRawParameterTypes(), input.getOwner());
            }
        };
    }

    private static boolean overridesOrImplements(final String name, final JavaClassList rawParameterTypes,
            final JavaClass owner) {
        final Set<JavaClass> classesToCheck = new HashSet<>();
        classesToCheck.addAll(owner.getAllSuperClasses());
        classesToCheck.addAll(owner.getAllInterfaces());
        for (final JavaClass c : classesToCheck) {
            for (final JavaMethod method : c.getAllMethods()) {
                if (name.equals(method.getName())
                        && parametersMatch(rawParameterTypes, method.getRawParameterTypes())) {
                    return true;
                }
            }
        }
        return false;
    }

    private static boolean parametersMatch(final JavaClassList a, final JavaClassList b) {
        if (a.size() != b.size()) {
            return false;
        }
        for (int i = 0; i < a.size(); i++) {
            final JavaClass first = a.get(i);
            final JavaClass second = b.get(i);
            if (!first.equals(second)) {
                if (!first.isAssignableTo(second.reflect())) {
                    return false;
                }
            }
        }
        return true;
    }

    private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);

    private static class Game {
    }

    private void handleEndGame(final List<Game> games) {
        games.forEach((game) -> executor.execute(() -> end(game)));//Good
    }

    private static void end(final Game game) {

    }
}

[Stephan202]

I'm trying to create methodsWithNoInstanceAccessShouldBeStatic rule to stop developers from checking in non-static methods that should be static.

@abeyecon for this sort of rules a tool such as Error Prone may be more suitable. It has the MethodCanBeStatic check.

[codecholeric]

Related to #215

[KorSin]

I wonder how this is testable, the same goes for #215, as we do not support Java 8 inside ArchUnit. Should we just test a solution with an external project then?

[hankem]

FYI: archunit/src/ already contains jdk9test and jdk15test, which use JavaVersions > 7.

[codecholeric]

@KorSin if you wanna look into that ping me first, because I already did some investigation and started with some draft (which I'm not going on with for the next months, so I can always pass it on 😉) It's a somewhat challenging issue though 😉 Also needs an extension of the domain model I think.

[codecholeric]

Should be fixed by #847