Exclude from a report private empty constructors that do not have arguments

[Godin]

After reflecting for some time, I believe that we can unconditionally safely ignore private empty constructors that do not have arguments. Even despite the fact that they relate to a source code and not generated by compiler. Note that this will completely exclude from report unused classes that have only such constructor, but anyway revealing of those is not a job of code coverage tool.

This will also improve report for JaCoCo itself.

[marchof]

@Godin This filter is definitely a quick win! I have two minor comments to improve the matcher API.

[marchof]

If nextIs() would have an boolean return value, we could write this as:

return nextIs(Opcodes.Return)

Could also be used in other existing filters.

[Godin]

@marchof then there will be temptation to use it with && everywhere, but that is exactly what we were trying to avoid.

[marchof]

@Godin It's internal API, I would try to keep the filter as readable as possible.

[Godin]

@marchof Then in case of preceding if statement containing return of a boolean, IDE start to suggest to combine them, making code unreadable. Also even if I don't see this in IntelliJ, static analyzers might warn about unused return value in other places.

[marchof]

The trick with ASTORE is to define a named argument. This looks surprising as there is no ASTORE involved in private empty constructors.

For better documentation I would therefore prefer to encapsulate this within AbstractMatcher and provide a method for this:

void defineLocal(int idx, String name)

[Godin]

@marchof I removed usage of nextIsVar - it does not work for a first instruction in absence of preceding label.

[marchof]

@Godin Ok, I see. I would still prefer to implement semantical matching premitives in AbstractMatcher, so we can write e.g.

	private boolean match(final MethodNode methodNode,
			final String superClassName) {
		cursor = methodNode.instructions.getFirst();
		skipNonOpcodes();
		nextIsVar(Opcodes.ALOAD, 0);
		nextIsMethod(Opcodes.INVOKESPECIAL, superClassName, "<init>",
				"()V");
		return nextIs(Opcodes.RETURN);
	}

[Updated]

[Godin]

@marchof I added skipNonOpcodes, but nextIsVar anyway can't be used for a very first instruction, because after skipNonOpcodes we need to check current instruction, while semantic of next... methods is to move to a next instruction. Change of semantic to check of current instruction - first of all will affect a lot of existing code relying on it, but anyway will lead to the same problem, a kind of, but at the end of method. IMO the only way - is to insert an artificial label at the beginning of all methods, but IMO this is an overkill compared just to a condition in if here, which is IMO perfectly readable.

Replacement of another if on method nextIsMethod in abstract class IMO also an overkill, because its only usage will be here, and there won't be code-sharing with other places where we already check MethodInsnNode (see references on it in TryWithResourcesJavacFilter and TryWithResourcesEcjFilter), and again IMO condition is perfectly readable.

[marchof]

@Godin Thanks for the explations. This would require to keep a state in AbstractMatcher and replace the direct access to cursor with simething like start(AbstractInsnNode).

But I agree that refactoring the matcher API soes not provide enough value for now.

[marchof]

@Godin I wonder whether we should add another negative test for the case where the private non-arg constructor is not empty in source code:

private static class PrivateNonEmptyNoArg {
	private PrivateNonEmptyNoArg() {
		nop(); // $line-privateNonEmptyNoArg$
	}
}

[Godin]

Done

[marchof]

@Godin Thx! 👍

[marchof]

@Godin From my point of view this can be merged. Please do so if you don't have any open issues.

[MonsieurBon]

Do you have an estimate about when this could be released?

[marchof]

@MonsieurBon Hopefully this year.

You can use our snapshot builds in the meantime and help us testing!

[saadlu]

@marchof

Snapshot build is fine with me, but I can't seem to test this.
This is what I have done

1. Checkout the latest master
   As of now, this is the master HEAD (your commit I assume :))

   commit 8f0460960967e15e46c9f2bf766c0bfe9e18e6b8
   Author: Marc R. Hoffmann <hoffmann@mountainminds.com>
   Date:   Thu Nov 2 22:07:24 2017 +0100
   
   Remove obsolete phase binding (#617)
   
   Since version 0.6.2 the "report" goal is bound to phase "verify".
   

2. built locally (with mvn clean install)

3. Using a gradle project to test out the local built jacoco

   apply plugin: 'jacoco'
   jacoco {
      toolVersion = "0.7.10-SNAPSHOT"
   }
   

   testing code:

   public class Empty {
   
       private Empty(){}
   
       public static String say(){
           return "say";
       }
    }
   
   

   with

   public class EmptyTest {
      @Test
      public void say() {
        assertThat(Empty.say(), is("say"));
      }
   
   }
   

4. Using sonar to generate me the report

5. Sonar says only 50% covered. Complaining that the private constructor is not convered

what am I doing wrong?

[Godin]

@saadlu see #513 (comment)