Java LoC metric #694

dburriss · 2021-10-26T05:42:30Z

This PR is for adding the LoC metric discussed in issue #359.

codecov-commenter · 2021-10-26T13:48:54Z

Codecov Report

Merging #694 (0a7b404) into master (08c61f4) will increase coverage by 0.00%.
The diff coverage is 73.75%.

@@           Coverage Diff           @@
##           master     #694   +/-   ##
=======================================
  Coverage   41.07%   41.07%           
=======================================
  Files          51       51           
  Lines        7016     7096   +80     
  Branches      988     1011   +23     
=======================================
+ Hits         2882     2915   +33     
- Misses       3463     3481   +18     
- Partials      671      700   +29

Impacted Files	Coverage Δ
src/metrics/loc.rs	`78.45% <70.83%> (-1.11%)`	⬇️
src/getter.rs	`76.05% <100.00%> (-5.26%)`	⬇️
src/node.rs	`71.15% <0.00%> (-15.39%)`	⬇️
src/spaces.rs	`71.51% <0.00%> (-1.22%)`	⬇️
src/metrics/nargs.rs	`74.71% <0.00%> (-0.77%)`	⬇️
src/langs.rs	`100.00% <0.00%> (+10.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08c61f4...0a7b404. Read the comment docs.

marco-c · 2022-01-10T11:03:22Z

@dburriss still interested in finishing this up?

dburriss · 2022-01-10T11:05:07Z

Yes. Just not a lot of time at the moment. Hopefully be able to come back to this in a week or 2.

dburriss · 2022-02-11T06:50:05Z

@marco-c ok based on the discussion in the issue I rolled back the incorrect route I went down with while logical lines.
I think implementation wise this is ready for a review.

Some notes/questions:

the different style of for loops adds a little complexity to the implementation. Maybe there is a more elegant way to handle this?
I wrote a fair number of tests as I was iteratively figuring out what is going on. Maybe some of these can be removed but when they break they do allow to narrow down exactly where. I worry a bit about navigating this file if a few more languages get added.

dburriss · 2022-02-12T09:13:17Z

@marco-c @Luni-4 this is up-to-date with master and ready for review.

calixteman · 2022-02-14T08:58:30Z

src/getter.rs

+        let typ = node.object().kind_id();
+        match typ.into() {
+            MethodDeclaration | LambdaExpression => SpaceKind::Function,
+            Class | ClassDeclaration => SpaceKind::Class,


Did you intentionally miss interface:
https://github.com/tree-sitter/tree-sitter-java/blob/a24ae7d16de3517bff243a87d087d0b4877a65c5/grammar.js#L632
?
If yes, please add a comment to explain why.
In looking at the above link I can see that there is a module concept (I don't know what is: I didn't write Java code for years), so same question.

Good catch. Since this is my first time working in Rust, this repo, and tree-sitter, rather than copying other implementations I just added grammar to make tests pass.
I will take a scan through to see if any other things I missed.
It has been a decade or so since I have looked at Java... I do not remember ever using module :D I learned something today.

For the SLOC off by 1 issue the only match I need is Program => SpaceKind::Unit. The space_kind for the rest can be handled in the implementations that need them. Otherwise no test coverage. Does that make sense to you @calixteman ?

What's the SLOC off by 1 issue ?

Well, I left this note for myself while documenting my changes. And no I don't generally make my notes as if I am talking to myself but I had initially thought to add to the documentation as I went.

As you implement tests, you may find yourself running into an off by 1 error for SLOC count.
For most code we expect the SpaceKind to be SpaceKind::Unit but the default is SpaceKind::Unknown.
To solve this you need to implement the Getter trait in src/getter.rs. Specifically, the get_space_kind function.

I think it is because any java snippet of code gets a Program leaf. I guess UNKNOWN gets counted and UNIT does not? Or maybe the other way around? It was a few months ago that I wrote it...

@calixteman does this one make sense? I imagine the rest will be added for other metrics as needed. Possibly needed for other metrics counting functions? I have not looked at anything but LoC and the Java Cyclomatic PR.

src/metrics/loc.rs

dburriss · 2022-03-02T07:04:50Z

@calixteman A quick summary of the latest changes in commit "Remove expressions from count".

I removed ALL Expression matches
IMPORTANT: with the removal of BinaryExpression UpdateExpression means the numbers for a for would drop from 3 to 2 unless explicitly handled. I will call this explicit handling out in a comment on the relevant code.
updated the expectations in the tests from the expression changes. The major change in count is around ternary as is expected.
added some tests and changed a few others to cover try, switch, do..while, throw, break, and continue.

A reminder: I will remove the debug stuff once we are happy with the implementation.

src/metrics/loc.rs

dburriss · 2022-03-02T19:05:57Z

Ok in this latest push I have incorporated the changes from the comments from @Luni-4 (hopefully @calixteman you agree with the arguments)

for statement is 1 LLOC and so expressions inside are ignored
commented with links to the Oracle articles on why we only count statements, including the one about for statements
removed the debug lines etc.
rebased the cyclomatic complexity and other changes from the past week or 2 on master

Hopefully, it is ready to go.

dburriss · 2022-03-09T15:51:19Z

@calixteman would you have a chance to review this before the weekend? That way I can make any requested changes over the weekend or move on to another.

marco-c · 2022-03-09T16:05:54Z

@dburriss Calixte is off this week, perhaps you could start looking at C#?

dburriss · 2022-03-09T16:09:01Z

Ah ok, good to know. Thanks @marco-c
Yes, I think I will start on C# cyclomatic complexity.

dburriss · 2022-03-17T21:31:22Z

A ping to see if we can get this merged?

marco-c · 2022-03-18T09:30:31Z

Calixte was off this week too unfortunately, he's back today :)
BTW, you could join our Matrix room on https://chat.mozilla.org/#/room/#rust-code-analysis:mozilla.org.

dburriss · 2022-03-19T08:19:56Z

Rebased in recent merges and fixed errors from recent Stats changes.

calixteman · 2022-03-19T17:55:07Z

src/getter.rs

+
+        let typ = node.object().kind_id();
+        match typ.into() {
+            Program => SpaceKind::Unit,


My definition of space is: something which can have its own metrics.
So for example, you can have a class A with an inner class B each of them could be a space: this way we'll have some metrics for both of them:

B has its own metrics

A has its own metrics merged with the B ones
And at the end in the final output you'll have the granularity the user wants.
So not having more spaces here means that we don't have any granularity.

Interesting. I have taken a shot at it based on the other implementations and that explanation. Other than that not having Program => SpaceKind::Unit causes the line count to be off (javascript and typescript have this match too), I am uncertain how to actually test this in the context of LoC?

@calixteman I think this covers what can contain other code in Java. As I said in the above comment, I am uncertain how to test this in the context of LoC.

ClassDeclaration => SpaceKind::Class, MethodDeclaration | LambdaExpression => SpaceKind::Function, PackageDeclaration | ModuleDeclaration => SpaceKind::Namespace, Program => SpaceKind::Unit

If you want to see space in actions you could just run cargo run -p rust-code-analysis-cli -- -m -p foo.java and have a look on the output, with foo.java:

class Outer { String x; class Inner { String y; int bar() { return 12; } } }

If you return unknown for any kind of space, you'll get some unknown in the output.
Maybe the "off by 1" issue is related to:
https://github.com/mozilla/rust-code-analysis/blob/master/src/metrics/loc.rs#L40

Ah cool! Makes complete sense. And yes that line of code was what led me to the fix of Unit for Program for the "off by 1" issue. It was in November last year when I started on this (and learning Rust) so the memories are a little hazy.

Adding a lambda expression and a package to the java file didn't seem to add anything to the structure. Not sure if I should leave them in, conceptually it seems correct to me as is. The code on this comment is not showing as outdated but I do think the kind is working as you wanted. See image:

calixteman · 2022-03-19T17:57:07Z

Sorry for the long delay: I've been to be off in the last weeks (life....).
Overall it looks good, just need to address the space thing issue and then I guess it'll be mergeable.

get_space_kind was needed to fix an off by one issue with ploc

calixteman · 2022-03-22T19:39:52Z

src/getter.rs

+
+        let typ = node.object().kind_id();
+        match typ.into() {
+            ClassDeclaration => SpaceKind::Class,


Maybe you could add InterfaceDeclaration here too.
Maybe it's worthing adding a SpaceKind::Interface because an interface contributes to loc for example but it expected to have other metrics equal to 0 since it just some functions declarations.
So having interface in the output will help the user the understand this numbers and maybe (probably ?) skip this data since they aren't really useful (but maybe they're).

I mapped InterfaceDeclaration as a SpaceKind::Class and it didn't show up at all in the hierarchy.

With AND without gave:

unit: foo.java

function: print

class: A6
-- function: print
-- function: main

for the following code file:

package com.java; interface printable{ void print(); } class A6 implements printable{ public void print(){System.out.println("Hello");} public static void main(String args[]){ A6 obj = new A6(); obj.print(); } }

Adding a SpaceKind::Interface could be interesting if it indeed showed up in the hierarchy. I would expect:

unit: foo.java

interface: printable
-- function: print

class: A6
-- function: print
-- function: main

Although as you said, not sure how many metrics interfaces actually contribute. Evaluating with "Lay of least surprise" I do think the SpaceKind::Interface makes sense but that should be a separate PR I would think?

calixteman

It looks good to me.
Thanks a lot for doing that.

dburriss marked this pull request as ready for review February 12, 2022 09:06

dburriss force-pushed the dburriss/feature/issue-359/java-loc branch from 7a7e7ac to 869afd9 Compare February 12, 2022 09:08

Luni-4 requested a review from calixteman February 14, 2022 08:45

calixteman reviewed Feb 14, 2022

View reviewed changes

src/metrics/loc.rs Outdated Show resolved Hide resolved

calixteman reviewed Feb 14, 2022

View reviewed changes

src/metrics/loc.rs Outdated Show resolved Hide resolved

dburriss commented Feb 22, 2022

View reviewed changes

src/metrics/loc.rs Outdated Show resolved Hide resolved

dburriss commented Mar 2, 2022

View reviewed changes

src/metrics/loc.rs Outdated Show resolved Hide resolved

dburriss force-pushed the dburriss/feature/issue-359/java-loc branch from 39c5da8 to 0a35fbc Compare March 2, 2022 18:53

dburriss force-pushed the dburriss/feature/issue-359/java-loc branch from 0a35fbc to 1f169ca Compare March 4, 2022 15:41

dburriss requested review from calixteman and Luni-4 March 4, 2022 15:42

dburriss force-pushed the dburriss/feature/issue-359/java-loc branch from 1f169ca to d6daf70 Compare March 19, 2022 07:35

dburriss mentioned this pull request Mar 19, 2022

Add support for C-Sharp #789

Open

calixteman reviewed Mar 19, 2022

View reviewed changes

WIP of Java LoC metric

c98bc45

dburriss added 17 commits March 22, 2022 06:23

Enables 2 passing simple tests

22190ba

Adds more individual loc tests

d14a724

Initial impl of JavaCode Getter trait

b988696

get_space_kind was needed to fix an off by one issue with ploc

Test cleanup

3ce6450

Fix test

f28b458

Fixes handling of for loops

2d11dbf

Adds while

36cf32d

Remove println

2108244

Ran fmt

ca1b35c

Reduce to handle only SLOC off by 1

70a04ed

Adds ternary lloc to test

aa42b7c

Adds expression grammar lloc

7d4f9f1

Removes expressions from count

9c030c2

Do not count expressions in for loop

c8112e1

Removes debugging lines

c90d092

Fixes for the recent Stats changes

8bac42f

Defines space kinds

c765293

dburriss force-pushed the dburriss/feature/issue-359/java-loc branch from e64f371 to c765293 Compare March 22, 2022 05:24

dburriss requested a review from calixteman March 22, 2022 19:09

calixteman reviewed Mar 22, 2022

View reviewed changes

Adds interface space

0a7b404

dburriss requested a review from calixteman March 23, 2022 05:56

calixteman approved these changes Mar 23, 2022

View reviewed changes

calixteman merged commit bcb3ca8 into mozilla:master Mar 23, 2022

marco-c mentioned this pull request Mar 23, 2022

Implement metrics for Java code #359

Closed

marco-c linked an issue Mar 23, 2022 that may be closed by this pull request

Implement metrics for Java code #359

Closed

dburriss mentioned this pull request Mar 24, 2022

Add an Interface SpaceKind #809

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java LoC metric #694

Java LoC metric #694

dburriss commented Oct 26, 2021

codecov-commenter commented Oct 26, 2021 •

edited

Loading

marco-c commented Jan 10, 2022

dburriss commented Jan 10, 2022

dburriss commented Feb 11, 2022

dburriss commented Feb 12, 2022

calixteman Feb 14, 2022

dburriss Feb 15, 2022

dburriss Feb 22, 2022

calixteman Feb 24, 2022

dburriss Feb 28, 2022

dburriss Mar 1, 2022

dburriss commented Mar 2, 2022 •

edited

Loading

dburriss commented Mar 2, 2022 •

edited

Loading

dburriss commented Mar 9, 2022

marco-c commented Mar 9, 2022

dburriss commented Mar 9, 2022

dburriss commented Mar 17, 2022

marco-c commented Mar 18, 2022

dburriss commented Mar 19, 2022

calixteman Mar 19, 2022

dburriss Mar 20, 2022

dburriss Mar 22, 2022

calixteman Mar 22, 2022

dburriss Mar 22, 2022

calixteman commented Mar 19, 2022

calixteman Mar 22, 2022

dburriss Mar 22, 2022 •

edited

Loading

calixteman left a comment

Java LoC metric #694

Java LoC metric #694

Conversation

dburriss commented Oct 26, 2021

codecov-commenter commented Oct 26, 2021 • edited Loading

Codecov Report

marco-c commented Jan 10, 2022

dburriss commented Jan 10, 2022

dburriss commented Feb 11, 2022

dburriss commented Feb 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dburriss commented Mar 2, 2022 • edited Loading

dburriss commented Mar 2, 2022 • edited Loading

dburriss commented Mar 9, 2022

marco-c commented Mar 9, 2022

dburriss commented Mar 9, 2022

dburriss commented Mar 17, 2022

marco-c commented Mar 18, 2022

dburriss commented Mar 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Mar 19, 2022

Choose a reason for hiding this comment

dburriss Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

calixteman left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 26, 2021 •

edited

Loading

dburriss commented Mar 2, 2022 •

edited

Loading

dburriss commented Mar 2, 2022 •

edited

Loading

dburriss Mar 22, 2022 •

edited

Loading