[java] UnnecessaryConstructor vs. AtLeastOneConstructor

[StevanWhite]

Affects PMD Version: 7.2.0

Rules: UnnecessaryConstructor and AtLeastOneConstructor

Description:

Situation: a class whose elements are properly instantiated with a no-argument constructor.

If the code is left without a constructor, the compiler will provide one, but PMD complains: Each class should have at least one constructor

If a constructor is provided in the code, PMD complains: Avoid unnecessary constructors - the compiler will generate these for you

Sure, the programmer can insert @SuppressWarnings into (otherwise very simple) code, but this really shouldn't be necessary. Sure, the programmer can disable either warning globally... but in fact, both warnings are good to have.

But as it is, it leaves the programmer thinking: "Make up your mind!". And it wastes time.

I'm not sure what measure would be best to take.
Perhaps a property for the UnnecessaryConstructor rule, "allowSingleConstructor", or something like that, would help.

Exception Stacktrace:

# Copy-paste the stack trace here

Code Sample demonstrating the issue:

/** An overly-simple example */
class ExampleClass {
        // If this constructor is present, PMD complains.
        // If this constructor is removed, PMD complains.
        ExampleClass(){
        }
}

The situation arises in many practical cases. For instance, the documentation for javax.annotation.processing.Processor explicitly states "Each implementation of a Processor must provide a public no-argument constructor to be used by tools to instantiate the processor."

Steps to reproduce:

Please provide detailed steps for how we can reproduce the bug.

1. ... Run PMD on a file named ExampleClass.java containing the above code
2. ...Then delete the constructor, and try it again.

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other]

Running PMD through CLI

[StevanWhite]

I think what you are getting at may be the following: A class should define data members, and the data members ought to be initialized in constructors.

But there are exceptions to this.

For instance, an abstract class might implement caches for its descendants. Such a class might define private data members that are all set to null on instantiation, for which no constructor is required.

[jsotuyod]

Both these rules are part of the code-style category. Code style is, by definition, an arbitrary convention a given organization or project adheres to. PMD attempts to not be opinionated on such matters, but enable users to enforce whatever convention they want to implement, and that sometimes means providing conflictive / opposite rules, such as on this case.

You don't provide detail, but I'd assume you are using the whole codestyle category as is. That is a user error. PMD encourages users to create a ruleset picking the rules that are right for them. This is further encouraged as it's the very first item in our best practices list.

Having said that, could we make this harder to occur? maybe…

• we could warn on whole category additions… but to be honest, that is probably not an error on itself (ie: including the whole errorprone category may be sane). This seems very specific to codestyle…
• merging both rules together, with a property set to either forbid or require to ensure both can't coexist… but the rule naming would suffer, as the behavior could no longer be explicitly stated but be tied to the property.
• improve the docs? I'm unsure what else to add, other than ensuring no samples do this; or using stronger wording in some paragraphs…

[StevanWhite]

" I'd assume you are using the whole codestyle category as is. "

​That is an incorrect assumption. More caution in future interactions is advisable.

For some legitimate code, I have been unable to find a PMD configuration that makes both warnings stop. It may be that I missed something in the configuration, but I spent an inordinate amount of time on it, and that drove me to spend more of my time to alert you to the problem.

The reasonable coding scenario I provided, short of disabling usually-sound PMD rules, produces one warning or the other. This is a bad situation, which results in the loss of time for good professional programmers. But PMD should be an aid for us, not a confusion.

I hoped that you would be in a better position to analyze these problems than I am. Maybe my hopes were too high, so I'll have a go at it.

The question is: When is it really a mistake for a class not to have a constructor?

• a concrete class, with data members that need initialization, must have one, in order to be instantiated.

(And, it isn't a syntax error to fail to explicitly initialize a non-final member variable, hence the PMD warning.)

but,

• a class, which implements only interfaces, might only contain code and data that are initialized in-line. Such a class needs no constructor besides the default, as there is nothing to initialize.

And this is just where I ran across the problem. I can't think of another situation like it.

I propose that the check be modified to:

A class should always have a constructor, if it, or any of its ancestors, has data members that are not explicitly initialized in-line.

[oowekyala]

As Juan explained, code style rules may contradict each other and should be chosen carefully. If you want a workaround to keep using those incompatible rules together, you can use XPath suppression to suppress violations of AtLeastOneConstructor that match your criteria.

My opinion on this is that we shouldn't change the rule. Yes, it has problems, but it can be disabled. Your proposed reformulation for the rule is IMHO misguided, in essence, an XY problem. If you consider that a constructor is necessary if there are fields that are not initialized inline, that's presumably because you want those fields to be initialized in the constructor (I don't see how it would make a difference otherwise whether there is a constructor or not). But for that purpose, just checking that a constructor exists is not enough, you have to check that those fields are written to in the constructors. A rule that does that would essentially have nothing to do with AtLeastOneConstructor, and would not be in the codestyle category.

[adangel]

We could slightly soften the rule AtLeastOneConstructor by adding a property, that would make this rule to not require a default constructor. But that would be essentially the same as not using the rule at all. Because in Java each class has implicitly always a default constructor, unless a constructor is explicitly provided (which would satisfy AtLeastOneConstructor).

The way I see this is:

• either don't use the two rules together at all
• use suppressions
• use one rule for only one part of your project, e.g. you have a specific package, that should never use unnecessary constructors. And you have another specific package, where you require at least one constructor. This essentially means, you use two different code styles within your project. You can achieve this through filters. Later on, maybe you want to move one of these packages out into a separate project (e.g. a API module and a Impl module).

[StevanWhite]

I am just a developer.

As I said, I have spent a lot of time on this already, suppressing stuff, and turning rules on and off. I found no solution, short of disabling more than I wanted to. I wonder if you have actually tried it.

The problem remains. It wasted my time. It will waste other people's time.

I do not know what the ideal solution would be, within the context of your project.

To dismiss this problem, as just a stupid user's stupid mistakes, is very unprofessional.

I am confident there is a solution, however. Actually try some code examples. Then sleep on it.

[oowekyala]

We should likely mention explicitly in the documentation that these rules are incompatible. In the future, #4298 may provide a better way to warn about this mistake and prevent wasting time.

Please stay professional in your tone. We have provided you a number of solutions, in good faith, but your attitude is very off-putting to me.