Skip to content

Commit

Permalink
Use first line number from LineNumberTable for JavaCodeUnit's sourceC…
Browse files Browse the repository at this point in the history
…odeLocation #344

So far, the `sourceCodeLocation` of `JavaMember`s does not contain a line number, as it cannot reliably be inferred from the byte code (cf. #75).

However, if the class file contains a [`LineNumberTable`](https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.12) for a method, using the smallest line number from the `LineNumberTable` for the member's `sourceCodeLocation` – even if this probably corresponds to the first executable statement and not to the method header definition – seems to be more useful to me than using the fallback line number `0` in any case.

So for example, I would infer the line number `10` from the following byte code:
```
  void methodWithBodyStartingInLine10();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=2, locals=1, args_size=1
         0: getstatic     #2                  // Field java/lang/System.out:Ljava/io/PrintStream;
         3: bipush        10
         5: invokevirtual #3                  // Method java/io/PrintStream.println:(I)V
         8: getstatic     #2                  // Field java/lang/System.out:Ljava/io/PrintStream;
        11: bipush        11
        13: invokevirtual #3                  // Method java/io/PrintStream.println:(I)V
        16: getstatic     #2                  // Field java/lang/System.out:Ljava/io/PrintStream;
        19: bipush        12
        21: invokevirtual #3                  // Method java/io/PrintStream.println:(I)V
        24: return
      LineNumberTable:
        line 10: 0
        line 11: 8
        line 12: 16
        line 13: 24
```
(My example's method header is actually defined in line 9, but I prefer 10 as opposed to 0... 😉)

Note that I do even get a line number for an empty method from the following byte code:
```
  void emptyMethodDefinedInLine15();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=0, locals=1, args_size=1
         0: return
      LineNumberTable:
        line 15: 0
```

Even if line numbers inferred in this way do not exactly point to the method header definition, they can be used to  compare different methods (i.e., the ordering should be correct). This would allow for new rules like, for example (irrespective of whether I'd personally want to have them as arch rules or not):
* Public methods are defined _before_ private methods.
* `equals`, `hashCode` and `toString` are not generated by a framework (or a developer... 🙈) in the _same_ line (cf. #337)

With this pull request, line numbers are recorded for `JavaCodeUnit`s (`JavaMethod`s, `JavaConstructor`s, `JavaStaticInitializer`s) if the class file has a `LineNumberTable`.
(The reported line number `0` for `JavaField`s is unchanged, as it cannot be inferred from byte code.)
  • Loading branch information
codecholeric authored Apr 15, 2020
2 parents a080697 + 61e140f commit 72b5ad6
Show file tree
Hide file tree
Showing 10 changed files with 108 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -911,8 +911,8 @@ Stream<DynamicTest> MethodsTest() {

.ofRule("no code units that are declared in classes that reside in a package '..persistence..' "
+ "should be annotated with @" + Secured.class.getSimpleName())
.by(ExpectedConstructor.of(SomeJpa.class).beingAnnotatedWith(Secured.class))
.by(ExpectedMethod.of(OtherJpa.class, "getEntityManager").beingAnnotatedWith(Secured.class))
.by(ExpectedConstructor.of(SomeJpa.class).inLine(15).beingAnnotatedWith(Secured.class))
.by(ExpectedMethod.of(OtherJpa.class, "getEntityManager").inLine(31).beingAnnotatedWith(Secured.class))

.toDynamicTests();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,23 @@ public static ExpectedConstructor.Creator of(Class<?> owner, Class<?>... params)
public static class Creator {
private final Class<?> clazz;
private final Class<?>[] params;
private int lineNumber;

private Creator(Class<?> clazz, Class<?>[] params) {
this.clazz = clazz;
this.params = params;
}

public Creator inLine(int lineNumber) {
this.lineNumber = lineNumber;
return this;
}

public ExpectedMessage beingAnnotatedWith(Class<? extends Annotation> annotationType) {
return new ExpectedMessage(String.format("Constructor <%s> is annotated with @%s in (%s.java:0)",
return new ExpectedMessage(String.format("Constructor <%s> is annotated with @%s in (%s.java:%d)",
formatMethod(clazz.getName(), JavaConstructor.CONSTRUCTOR_NAME, JavaClass.namesOf(params)),
annotationType.getSimpleName(),
clazz.getSimpleName()));
clazz.getSimpleName(), lineNumber));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,19 @@ public static class Creator {
private final Class<?> clazz;
private final String methodName;
private final Class<?>[] params;
private int lineNumber;

private Creator(Class<?> clazz, String methodName, Class<?>[] params) {
this.clazz = clazz;
this.methodName = methodName;
this.params = params;
}

public Creator inLine(int lineNumber) {
this.lineNumber = lineNumber;
return this;
}

public ExpectedMessage toNotHaveRawReturnType(Class<?> type) {
return method("does not have raw return type " + type.getName());
}
Expand All @@ -37,7 +43,7 @@ public ExpectedMessage beingAnnotatedWith(Class<? extends Annotation> annotation

private ExpectedMessage method(String message) {
String methodDescription = format("Method <%s>", formatMethod(clazz.getName(), methodName, JavaClass.namesOf(params)));
String sourceCodeLocation = format("(%s.java:0)", clazz.getSimpleName());
String sourceCodeLocation = format("(%s.java:%d)", clazz.getSimpleName(), lineNumber);
return new ExpectedMessage(format("%s %s in %s", methodDescription, message, sourceCodeLocation));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.tngtech.archunit.Internal;
import com.tngtech.archunit.PublicAPI;
import com.tngtech.archunit.base.DescribedPredicate;
import com.tngtech.archunit.base.HasDescription;
import com.tngtech.archunit.base.Optional;
import com.tngtech.archunit.core.domain.properties.CanBeAnnotated;
import com.tngtech.archunit.core.domain.properties.HasAnnotations;
Expand Down Expand Up @@ -59,7 +58,7 @@ public abstract class JavaMember implements
this.descriptor = checkNotNull(builder.getDescriptor());
this.annotations = builder.getAnnotations(this);
this.owner = checkNotNull(builder.getOwner());
this.sourceCodeLocation = SourceCodeLocation.of(owner);
this.sourceCodeLocation = SourceCodeLocation.of(owner, builder.getFirstLineNumber());
this.modifiers = checkNotNull(builder.getModifiers());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ public abstract static class JavaMemberBuilder<OUTPUT, SELF extends JavaMemberBu
private Set<JavaModifier> modifiers;
private JavaClass owner;
private ClassesByTypeName importedClasses;
private int firstLineNumber;

private JavaMemberBuilder() {
}
Expand All @@ -142,6 +143,10 @@ SELF withModifiers(Set<JavaModifier> modifiers) {
return self();
}

void recordLineNumber(int lineNumber) {
this.firstLineNumber = this.firstLineNumber == 0 ? lineNumber : Math.min(this.firstLineNumber, lineNumber);
}

@SuppressWarnings("unchecked")
SELF self() {
return (SELF) this;
Expand Down Expand Up @@ -178,6 +183,10 @@ public JavaClass getOwner() {
return owner;
}

public int getFirstLineNumber() {
return firstLineNumber;
}

@Override
public final OUTPUT build(JavaClass owner, ClassesByTypeName importedClasses) {
this.owner = owner;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ public void visitCode() {
@Override
public void visitLineNumber(int line, Label start) {
LOG.trace("Examining line number {}", line);
codeUnitBuilder.recordLineNumber(line);
actualLineNumber = line;
accessHandler.setLineNumber(actualLineNumber);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@
import com.tngtech.archunit.core.importer.testexamples.integration.ClassD;
import com.tngtech.archunit.core.importer.testexamples.integration.ClassXDependingOnClassesABCD;
import com.tngtech.archunit.core.importer.testexamples.integration.InterfaceOfClassX;
import com.tngtech.archunit.core.importer.testexamples.methodimport.ClassWithMultipleMethods;
import com.tngtech.archunit.core.importer.testexamples.methodimport.ClassWithObjectVoidAndIntIntSerializableMethod;
import com.tngtech.archunit.core.importer.testexamples.methodimport.ClassWithStringStringMethod;
import com.tngtech.archunit.core.importer.testexamples.methodimport.ClassWithThrowingMethod;
Expand Down Expand Up @@ -739,6 +740,36 @@ public void imports_methods_with_correct_throws_declarations() throws Exception
assertThat(method.getExceptionTypes()).matches(FirstCheckedException.class, SecondCheckedException.class);
}

@Test
public void imports_members_with_sourceCodeLocation() throws Exception {
ImportedClasses importedClasses = classesIn("testexamples/methodimport");
String sourceFileName = "ClassWithMultipleMethods.java";

JavaClass javaClass = importedClasses.get(ClassWithMultipleMethods.class);
assertThat(javaClass.getField("usage").getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":0)"); // the byte code has no line number associated with a field
assertThat(javaClass.getConstructor().getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":3)"); // auto-generated constructor seems to get line of class definition
assertThat(javaClass.getStaticInitializer().get().getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":5)"); // auto-generated static initializer seems to get line of first static variable definition
assertThat(javaClass.getMethod("methodDefinedInLine7").getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":7)");
assertThat(javaClass.getMethod("methodWithBodyStartingInLine10").getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":10)");
assertThat(javaClass.getMethod("emptyMethodDefinedInLine15").getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":15)");
assertThat(javaClass.getMethod("emptyMethodEndingInLine19").getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":19)");

javaClass = importedClasses.get(ClassWithMultipleMethods.InnerClass.class);
assertThat(javaClass.getMethod("methodWithBodyStartingInLine24").getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":24)");

javaClass = importedClasses.get(ClassWithMultipleMethods.InnerClass.class.getName() + "$1");
assertThat(javaClass.getMethod("run").getSourceCodeLocation())
.hasToString("(" + sourceFileName + ":27)");
}

@Test
public void imports_methods_with_one_annotation_correctly() throws Exception {
JavaClass clazz = classesIn("testexamples/annotationmethodimport").get(ClassWithAnnotatedMethods.class);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.tngtech.archunit.core.importer.testexamples.methodimport;

public class ClassWithMultipleMethods {

static String usage = "ClassFileImporterTest's @Test imports_methods_with_correct_sourceCodeLocation";

int methodDefinedInLine7() { return 7; }

void methodWithBodyStartingInLine10() {
System.out.println(10);
System.out.println(11);
System.out.println(12);
}

void emptyMethodDefinedInLine15() { }

void emptyMethodEndingInLine19() {

}

public static class InnerClass {

void methodWithBodyStartingInLine24() {
new Runnable() {
@Override
public void run() {
System.out.println(27);
}
};
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -758,8 +758,8 @@ public void classes_should_have_only_private_constructor(ArchRule rule) {
assertThat(rule).checking(importClasses(ClassWithPrivateConstructors.class))
.hasNoViolation();
assertThat(rule).checking(importClasses(ClassWithPublicAndPrivateConstructor.class))
.hasOnlyViolations(String.format("Constructor <%s.<init>(%s)> is not private in (%s.java:0)",
ClassWithPublicAndPrivateConstructor.class.getName(), String.class.getName(), getClass().getSimpleName()));
.hasOnlyViolations(String.format("Constructor <%s.<init>(%s)> is not private in (%s.java:%d)",
ClassWithPublicAndPrivateConstructor.class.getName(), String.class.getName(), getClass().getSimpleName(), 1538));
}

@DataProvider
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ public void orShould_ORs_conditions(ArchRule rule) {
RightOne.class.getName(), RightTwo.class.getName()));
assertThat(report.getDetails()).containsOnly(
String.format("%s and %s",
isNotDeclaredInMessage(WrongOne.class, CONSTRUCTOR_NAME, RightOne.class),
isNotDeclaredInMessage(WrongOne.class, CONSTRUCTOR_NAME, RightTwo.class)),
isNotDeclaredInMessage(WrongOne.class, CONSTRUCTOR_NAME, RightOne.class, 111),
isNotDeclaredInMessage(WrongOne.class, CONSTRUCTOR_NAME, RightTwo.class, 111)),
String.format("%s and %s",
isNotDeclaredInMessage(WrongOne.class, "wrongMethod1", RightOne.class),
isNotDeclaredInMessage(WrongOne.class, "wrongMethod1", RightTwo.class)));
isNotDeclaredInMessage(WrongOne.class, "wrongMethod1", RightOne.class, 113),
isNotDeclaredInMessage(WrongOne.class, "wrongMethod1", RightTwo.class, 113)));
}

@DataProvider
Expand All @@ -75,24 +75,24 @@ public void andShould_ANDs_conditions(ArchRule rule) {
"members should not be declared in %s and should not be declared in %s",
WrongOne.class.getName(), WrongTwo.class.getName()));
assertThat(report.getDetails()).containsOnly(
isDeclaredInMessage(WrongOne.class, CONSTRUCTOR_NAME),
isDeclaredInMessage(WrongOne.class, "wrongMethod1"),
isDeclaredInMessage(WrongTwo.class, CONSTRUCTOR_NAME),
isDeclaredInMessage(WrongTwo.class, "wrongMethod2"));
isDeclaredInMessage(WrongOne.class, CONSTRUCTOR_NAME, 111),
isDeclaredInMessage(WrongOne.class, "wrongMethod1", 113),
isDeclaredInMessage(WrongTwo.class, CONSTRUCTOR_NAME, 117),
isDeclaredInMessage(WrongTwo.class, "wrongMethod2", 119));
}

private String isDeclaredInMessage(Class<?> clazz, String methodName) {
return message(clazz, methodName, "", clazz);
private String isDeclaredInMessage(Class<?> clazz, String methodName, int lineNumber) {
return message(clazz, methodName, "", clazz, lineNumber);
}

private String isNotDeclaredInMessage(Class<?> clazz, String methodName, Class<?> expectedTarget) {
return message(clazz, methodName, "not ", expectedTarget);
private String isNotDeclaredInMessage(Class<?> clazz, String methodName, Class<?> expectedTarget, int lineNumber) {
return message(clazz, methodName, "not ", expectedTarget, lineNumber);
}

private String message(Class<?> clazz, String methodName, String optionalNot, Class<?> expectedTarget) {
return String.format("%s <%s.%s()> is %sdeclared in %s in (%s.java:0)",
private String message(Class<?> clazz, String methodName, String optionalNot, Class<?> expectedTarget, int lineNumber) {
return String.format("%s <%s.%s()> is %sdeclared in %s in (%s.java:%d)",
CONSTRUCTOR_NAME.equals(methodName) ? "Constructor" : "Method",
clazz.getName(), methodName, optionalNot, expectedTarget.getName(), getClass().getSimpleName());
clazz.getName(), methodName, optionalNot, expectedTarget.getName(), getClass().getSimpleName(), lineNumber);
}

@SuppressWarnings("unused")
Expand Down

0 comments on commit 72b5ad6

Please sign in to comment.