Use first line number from LineNumberTable for JavaCodeUnit's sourceCodeLocation

[hankem]

So far, the sourceCodeLocation of JavaMembers 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 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. Arch Unit configuration with lombok [Error] #337)

With this pull request, line numbers are recorded for JavaCodeUnits (JavaMethods, JavaConstructors, JavaStaticInitializers) if the class file has a LineNumberTable.
(The reported line number 0 for JavaFields is unchanged, as it cannot be inferred from byte code.)

[ghost]

Congratulations 🎉. DeepCode analyzed your code in 1.714 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

---

[codecholeric]

Nice, thanks a lot 😃
I still feel a little uneasy about the "heuristic over the thumb" but I agree that it is probably more useful than a constant 0 😉
It's particularly nice that you get this number also for void-methods with empty body 😄
This makes it at least pretty consistent between methods and constructors I guess.
Too bad there is no way to determine it for fields 😞
But regarding the heuristic, is there any way to distinguish this empty case when the line number is actually exact (at least as far as I could tell?) and the case where it is the first line of the body? Because if yes I would wonder if a best guess - 1 wouldn't be even better in that case, even though it might sometimes still be a little off? (e.g. if there is a comment in the first line)

[hankem]

I don't think there's a way to get what you'd consider exact... Consider this example:

17    void emptyMethodEndingInLine19() {
18
19    }

The auto-generated return (the only statement in the byte code) gets placed in line 19, presumably at the }.

If we had instead used

17    void emptyMethodEndingInLine19() {
18        return;
19    }

it would of course be in line 18... 🤷‍♂️

I would anyways refrain from playing any "line number – 1" trick, but just stick to the data that is actually there (to me, the only canonical options seem to be either the first or the last line number of the statements in the byte code), because

• we don't know, as you already mention, whether "the first line of a statement – 1" really hits the method header declaration, due to, e.g.,
  • the method body starting with a comment (which can be arbitrarily long)
  • the method body starting with one or several empty lines
  • a code style demanding a method such as

    public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) {

to be written as

    public void visit(
            int version,
            int access,
            String name,
            String signature,
            String superName,
            String[] interfaces
    ) {

• simple, well defined behavior can IMO more easily be explained/understood
  (I'm sure that people will ask what the line number means...)
• we don't pretend that we could solve a problem that we know we cannot. 😉

[codecholeric]

Okay, you have convinced me 😉 I agree, we can't solve it perfectly, and an educated guess close to the declaration is still a lot nicer than line number 0 which will definitely make you search for the declaration (in any non-trivial class). And yes, keeping it simple and directly providing the info from the bytecode seems to be the way to go. Too bad we have no estimate whatsoever for fields ☹️

[codecholeric]

lgtm 😄, I have one small comment. Also I feel we should squash these two commits. Because adapting the tests feels to me like it should be part of the same commit. WDYT?

[hankem]

I feel we should squash these two commits.

Fair enough. (I just kept them separate as I wasn't sure whether we should make the existing failure report tests somehow ignore the line number of methods [which actually didn't work out easily] rather than encode the line number in the tests... 😉)