new check: ConstructorsDeclarationGrouping to check the order of overloaded constructors

[Zopsss]

I have downloaded the latest cli from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

How it works Now:
Checkstyle have a check called OverloadMethodsDeclarationOrder which checks the order of overloaded methods, meaning that it checks if the overloaded methods are grouped together or not, if there is another method between overloaded methods then it'll give you an error. Currently, in checkstyle there is no module to check the order of overloaded constructors in a class, enum, abstract class, records. This new module will check that the overloaded constructors are grouped together or not. If there is another method or anything between the constructors, it will give an error.

How I want check to work

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="ConstructorsDeclarationGrouping"/>
    </module>
</module>

Test.java

$ cat Test.java 
public class Test {
    public Test(){}

    public Test(int a){}

    public void testing() {}

    public Test(int a, int b) {}

}

Expected output
The module should check the order of overloaded constructors and give error if the constructors are not in order. It should give an error something like this:

$ java -jar checkstyle-10.13.0-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] C:\checkstyle testing\Test.java:8:3: All overloaded constructors should be placed next to each other. Placing other methods in between overloaded constructors is a violation. Previous overloaded constructor located at line ''4''.[OverloadMethodsDeclarationOrder]
Audit done.
Checkstyle ends with 1 errors.

---

Some other examples showing how the check should work
Example 1: This is fine

public class Example1 {
    Example1 () { }

    Example1 (int a) { }

    Example1 (int a, int b) { }

    void testing () { }
}

Example 2: This is fine as we do not required to have overloaded constructors in a specific order like in ascending order based on the number of parameters or something similar to that. We only care about that if the overloaded constructors are grouped together or not, we don't check their order based on number of parameters or anything.

public class Example2 {
    Example2 (int a, String str) { }

    Example2 () { }

    Example2 (int a) { }
}

Example 3: This will give error, constructors are not in order

enum Example3 {
  TEST1, TEST2, TEST3;

  Example3() { }

  Example3(String a) { }

  void testing() { } 

  Example3(int a) { } // violation, all overloaded constructors should be grouped together 
}

Example 4: This is fine

record Example4(int x, int y) {
  Example4() {
    this(0, 0);
  }

  Example4(int x, int y) {
    this.x = x;
    this.y = y;
  }

  Example4(int x) {
    this(x, 0);
  }

  Example4(int x, int y, int z) {
    this(x, y + z);
  }
}

Example 6: This will give error, constructors are not in an order

record Example6(int x, int y) {

  Example6(int x) {
    this(x, 0);
  }

  Example5() {
    this(0, 0);
  }

  void testing() { } 

  Example6(int x, int y, int z) { // violation, all overloaded constructors should be grouped together
    this(x, y + z);
  }
}

---

Solution

The check should be able to verify that all overloaded constructors are grouped together in a class, enum, records, etc. The overloaded constructors should be strictly grouped together, if there is a method, variables, etc between them then it will give an error.

[Zopsss]

The issue #14273 will be continuation of this issue, updating the google_checks.xml based on the new functionality.

If we want to add new functionality to OverloadMethodsDeclarationOrder check, this needs to be done in the scope of an entirely new issue. It has nothing to do with the google style guide. After this work is completed, we can revisit this issue to update google_checks.xml, related documentation, and tests to prove that we support it.
As @nrmancuso suggested, I created this issue so we can discuss/work on adding the new functionality to this check, after the new functionality has will be added, we can start working on the issue #14273 for updating the google_checks.xml.

I have already made a PR for adding the new functionality, I just need the maintainers review on that. I have referenced that PR here. I would like to have @romani @nrmancuso opinion on this.

[nrmancuso]

This feels like it should be its own check, OverloadedConstructorOrder, since there are a few nuances with constructors such as compact constructors in records. Each check should only care about its target (constructor or method) and that they are grouped and ordered correctly, and NOT the relationship to the other target. We have https://checkstyle.sourceforge.io/checks/coding/declarationorder.html#DeclarationOrder for that.
@rnveach @romani ?

[rnveach]

@Zopsss @nrmancuso Please make sure the URLs you are using are https://checkstyle.org/ and not sourceforge. The only thing sourceforge supports that our main doesn't still is version history which I think should be fixed.

OverloadedConstructorOrder

I am fine with this, but...

a few nuances with constructors such as compact constructors in records

Can we explain what these nuances are? I am not seeing anything.

While I am generally fine with OverloadedConstructorOrder, I am not sure I see how it will be nothing more than a copy/paste of the existing module.

[Zopsss]

OverloadedConstructorOrder

I am fine with this, but...

a few nuances with constructors such as compact constructors in records

Can we explain what these nuances are? I am not seeing anything.

While I am generally fine with OverloadedConstructorOrder, I am not sure I see how it will be nothing more than a copy/paste of the existing module.

@rnveach
I was also thinking the same that if we decide to make it's own check for constructors, it will be mostly copy/pasting the current check. But as @nrmancuso is saying, if there are some nuances with constructors, we can investigate that to decide if it is solvable without creating a new check or not. I have made a PR which extends the current check for supporting constructors, I have linked it above, you can check that out and see how we can extend this check. Either way I would be happy to contribute with whatever decision we take.

[nrmancuso]

Why we should create a new check:

1. Constructors are not methods, they should not be checked by OverloadMethodsDeclarationOrder
2. Logic of checking constructors is different, since they do not have a return type.
3. Some of the tokens included in OverloadMethodsDeclarationOrder do not have constructors.
4. Constructor checking and method checking should be completely separate, i.e. we should only check that each group is ordered correctly and that sequencing is not broken. I feel that checking for both in the same check could encourage some sort of coupling that shouldn't exist.
5. Expanding on item (4), properties added to the check may not apply to all tokens, such as preference for compact constructor order.

Regardless of if we extend OverloadMethodsDeclarationOrder or create OverloadConstructorDeclarationOrder, we need to clearly define the behavior of the check when checking compact constructors:

public class Test {
    record R1(int x, int y) {

        R1() {
            this(0, 0);
        }

        R1(int x) {
            this(x, 0);
        }

        R1(int x, int y) {
            this.x = x;
            this.y = y;
        }

        R1(int x, int y, int z) {
            this(x, y + z);
        }
    }

    record R2(int x, int y) {

        R2() {
            this(0, 0);
        }

        R2(int x) {
            this(x, 0);
        }

        R2 {
            // Compact constructor is equivalent to R2(int x, int y).
            // Should this be here? Could it be argued that compact
            // constructors should be declared first?
        }

        R2(int x, int y, int z) {
            this(x, y + z);
        }
    }
}

Additional reading: https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.8

[nrmancuso]

@nrmancuso thanks for the detailed explanation, you have strong points... @rnveach can you pls confirm on how we should approach this issue? then I will start working on this issue

We will not be able to approve this issue until we have determined how we should check compact constructors. @Zopsss please share your vision on this.

[Zopsss]

@nrmancuso thanks for the detailed explanation, you have strong points... @rnveach can you pls confirm on how we should approach this issue? then I will start working on this issue

We will not be able to approve this issue until we have determined how we should check compact constructors. @Zopsss please share your vision on this.

@nrmancuso I agree with all the 5 points you mentioned but...

        R2 {
            // Compact constructor is equivalent to R2(int x, int y).
            // Should this be here? Could it be argued that compact
            // constructors should be declared first?
        }

for the compact constructor example you mentioned, I researched about it but I did not find any rules saying that we should keep the compact constructor at first before any other constructors. But I guess we should keep that rule in our check, that rule seems pretty obvious to keep as it helps us to initialize the instance variables of record class based on our requirements or conditions so it should be the first constructors so other contributor's can see how the record variables are initialized. I agree to keep that rule, now we only need @romani's approval on this issue.

[nrmancuso]

@Zopsss if you are in agreement about creating a new check, please update the issue description and title to match.

[nrmancuso]

@Zopsss we need to extend issue description to show good and bad code, with classes and records to show users clearly what the behavior should be and make sure we have agreement during implementation

[Zopsss]

@Zopsss we need to extend issue description to show good and bad code, with classes and records to show users clearly what the behavior should be and make sure we have agreement during implementation

done

[Zopsss]

@romani can you approve this issue?

[Zopsss]

@romani

[rnveach]

@Zopsss No one likes the constant pinging and I would consider it rude. Not all of us live in GH to respond to every message every day. This is not the only issue admins are looking at. I always come on to a backlog of 20-30 notifications a day, and most aren't for me. It is more proper etiquette to wait some time for a reply, before thinking this is being missed and ask for a follow up. If you believe you are waiting on only approval at this point, then you can ask (if you can't do it) the issue be assigned to the person you are waiting on to ensure they see the assignment. Otherwise, I recommend you look at other issues or PRs to spend your free time on if you wish.

[Zopsss]

@Zopsss No one likes the constant pinging and I would consider it rude. Not all of us live in GH to respond to every message every day. This is not the only issue admins are looking at. I always come on to a backlog of 20-30 notifications a day, and most aren't for me. It is more proper etiquette to wait some time for a reply, before thinking this is being missed and ask for a follow up. If you believe you are waiting on only approval at this point, then you can ask (if you can't do it) the issue be assigned to the person you are waiting on to ensure they see the assignment. Otherwise, I recommend you look at other issues or PRs to spend your free time on if you wish.

so sorry for that, I thought he missed my ping. Can you assign this issue to him? I don't have permission do it

[Zopsss]

hey @romani can you review this issue pls?

[romani]

constructors are not in order

What order of ctor we expect? From less parameters to more parameters? I do not think we do this for methods.

Did you mean we are going to enforce groupping of constructors and any declaration in between ctor declaration will be violation ?

Please make Inputs with violation message exact location and message, you will reuse this code after approval.

[Zopsss]

What order of ctor we expect? From less parameters to more parameters? I do not think we do this for methods.

Did you mean we are going to enforce groupping of constructors and any declaration in between ctor declaration will be violation ?

In OverloadMethodsDeclarationOrder we only check that if another methods comes between the overloaded methods or not, we do not care about any other thing like variables, enums, constructors, interfaces, class, etc... So if we decide to implement this check as OverloadMethodsDeclarationOrder check, we will only check that if any methods comes between overloaded constructors if yes then it will give errors. Below is an example file:

public class TestMethods {
	public void foo(int i) {}

	public void foo(String s) {}

	public void foo(int i, String s) {}

	public enum Hello{ // ok
		TEST, TEST2
	}

	public TestMethods(String a) {} // ok

	int a = 10; // ok

	interface Testinggg { // ok
	}

	static class Test2 { // ok
	}

        public void notFoo() {}  // violation

	public void foo(String s, int i) {}
}

config:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
  <module name="TreeWalker">
    <module name="OverloadMethodsDeclarationOrder"/>
  </module>
</module>

result:

$ java -jar ../checkstyle-10.13.0-all.jar -c config.xml TestMethods.java
Starting audit...
Audit done.

@romani

[Zopsss]

Additionally, in records we will also check that if it contains a compact constructor, if it is then it should be the first constructor in order, it cannot be in somewhere between other constructors as explained in #14415 (comment) ( last paragraph with code )

[nrmancuso]

I would expect the general behavior of this check to follow whatever MethodDeclarationOrder does for grouping/ordering. The documentation at https://checkstyle.org/checks/coding/overloadmethodsdeclarationorder.html#Example1-code gave me the impression that we ordered methods by number of parameters, ascending, and grouped by no fields/ other method declarations should come between a given sequence of overloaded methods.

Looks like we should also extend the documentation for MethodDeclarationOrder to make it's behavior clear.

[romani]

@Zopsss ,

Below is an example file:

please update example to show where violation is expected by comments, same as we do in our Inputs for test and Example for xdoc.

[Zopsss]

@Zopsss ,

Below is an example file:

please update example to show where violation is expected by comments, same as we do in our Inputs for test and Example for xdoc.

Updated the example showing what's allowed and what's not

[Zopsss]

I would expect the general behavior of this check to follow whatever MethodDeclarationOrder does for grouping/ordering. The documentation at https://checkstyle.org/checks/coding/overloadmethodsdeclarationorder.html#Example1-code gave me the impression that we ordered methods by number of parameters, ascending, and grouped by no fields/ other method declarations should come between a given sequence of overloaded methods.

Looks like we should also extend the documentation for MethodDeclarationOrder to make it's behavior clear.

Yeah I agree with you

[romani]

@Zopsss, please move all to issue description to have single place with all details and examples.

I will remove all comments as we finalize discussion, to avoid confusion from next readers of this issue.

Below is an example file:

It as only single ctor. Not clear why we have violations.

[Zopsss]

@Zopsss, please move all to issue description to have single place with all details and examples.

Done, I have updated the description as we discussed, added more examples and additional context.

Below is an example file:

It as only single ctor. Not clear why we have violations.

The example was showing how the OverloadedMethodsDeclarationOrder works, showing what is allowed and what is not. Our new check will work the same way like this check but for constructors. @romani

[romani]

Please update examples to put violations on constructors that are happening after nonctor. Check should violate only constructors.

[Zopsss]

Please update examples to put violations on constructors that are happening after nonctor. Check should violate only constructors.

Done

[Zopsss]

@romani ping

is this issue is ready to be approved? or it still needs some changes?

[romani]

Example5 { // violation, compact constructor should be first in order

Why violation?

---

Name of module become a confusion. As we do not care about order and just care about grouping. Let's rename Check
OverloadMethodsDeclarationGrouping ?

[Zopsss]

Example5 { // violation, compact constructor should be first in order

Why violation?

Me & @nrmancuso thinks that in records compact constructor should be first in the order. Please take a look at the example shown here #14415 (comment)
Here's the reason I think why we should give violation for compact constructors if they're not first in the order: #14415 (comment)

Name of module become a confusion. As we do not care about order and just care about grouping. Let's rename Check OverloadMethodsDeclarationGrouping ?

You mean OverloadConstructorsDeclarationGrouping right? And by grouping you mean that all the overloaded constructors should be grouped together right? if Yes, then I guess your point is right we should rename the module name. Initially this module was inspired by OverloadMethodsDeclarationOrder so we decided to name it after that module. @romani

[nrmancuso]

Me & @nrmancuso thinks that in records compact constructor should be first in the order.

I didn't think this, I just shared a possibility :)

Initially this module was inspired by OverloadMethodsDeclarationOrder so we decided to name it after that module.

This does seem reasonable initially, but if we aren't actually checking the order, we should consider the name change. Although, adding ordering probably wouldn't be that difficult if that is something that we wanted to do. Not sure about this one.

[romani]

You mean OverloadConstructorsDeclarationGrouping right?

Yes

And by grouping you mean that all the overloaded constructors should be grouped together right?

Yes

---

Ordering might be tricky, as user like logical ordering not a technical ordering. Logical: by reusage, by amount or arguments, as vs description, ...
We can meditate on this after grouping is done and enforced.

Let's update all examples to do grouping validation only.

[Zopsss]

Ordering might be tricky, as user like logical ordering not a technical ordering. Logical: by reusage, by amount or arguments, as vs description, ... We can meditate on this after grouping is done and enforced.

ohkayyy

Let's update all examples to do grouping validation only.

I have updated the examples, In Example 2 I mentioned that we don't check about the order of overloaded constructors expect for the compact constructor in record. Also updated other examples in which constructors are not in a specific order based on parameters. @romani

[romani]

Example 7: This is fine as we do not check for classes, interfaces, variables, etc between the overloaded constructors

Should be violation full. As all actors should be grouped together.

---

OverloadConstructorsDeclarationOrder

Please rename to ConstructorsDeclarationGroupping

Example5 { // violation, compact constructor should be first in order

Are you sure?
We targeting grouping. Ordering in group is more complicated, and need more parameters, I think ordering should be separate Check.

[Zopsss]

Should be violation full. As all actors should be grouped together.

Done! removed that example and the check will follow the rule you suggested.

Please rename to ConstructorsDeclarationGroupping

Done

Are you sure? We targeting grouping. Ordering in group is more complicated, and need more parameters, I think ordering should be separate Check.

This was just an idea by Mr. Nick, if you think that it should be in different check then I'm also okay with it. @romani

[Zopsss]

Can I start working on this issue now?

[romani]

No, until issue is labeled as approved, I do not recommend to start, as you will waste bunch time in reimplementation.

Example5: This will give error because compact constructor should be the first constructor in the order

We do not order ctors, we just group them.

[Zopsss]

We do not order ctors, we just group them.

sorry for the late reply... I've removed that example and updated the issue description as well @romani

Do we need anything else to change also?

[romani]

Please close this issue, and open new issue. It should not have "order" word at all!! I fixed few cases of that word, but we should eliminate it completely. So we will start review of design from scratch, without any history.

[Zopsss]

here is the new issue link: #14726

I updated the description and ensured that the new module does not checks the order of constructors, it only checks the grouping of them @romani