Issue #6301: ArrayTypeStyle support for method definitions

[esilkensen]

The goal of this PR is to address Issue #6301 by adding support for checking the ArrayTypeStyle of method definitions.

The following is valid Java syntax:

Object emptyArray()[] {
    ...
}

With this PR, method definitions with array return types like the above would be reported as violations regardless of the value of javaStyle. My initial thought was that since functions can't have an array return type in C, it could make sense for this check to always enforce "Java-style" method return types.

That said, I think it'd be an easy enough code change to only report these method return type violations when javaStyle is true - or similarly to add a new parameter (or TokenTypes.METHOD_DEF as an optional token?) to configure whether to check method return types.

What do you think?

Regression reports:

• http://esilkensen.github.io/checkstyle-tester/6301/true (javaStyle="true" )
• http://esilkensen.github.io/checkstyle-tester/6301/false (javaStyle="false")

[rnveach]

IMO, user should be able to trigger array style on/off for methods just like they can with variables. This follows the original logic of the check to enforce the way the user likes it written.
Example: Should be able to trigger methods to be written with the [] on the method name and variables with the [] on the type.

@romani You agree?

http://checkstyle.sourceforge.net/reports/google-java-style-20170228.html#s4.8.3-arrays

4.8.3.2 No C-style array declarations
The square brackets form a part of the type, not the variable: String[] args, not String args[].

Google doesn't specifically mention []s for methods, but I think it is safe to assume by the section title, they won't want the brackets on the method name.

[romani]

Should be able to trigger methods to be written with the [] on the method name and variables with the [] on the type.

I do not think so, I have never seen a human who likes that C style. Java got it only to allow easily convert C code to Java.

Please confirm that you are ok to proceed with proposed solution.

Google doesn't specifically mention []s for methods, but I think it is safe to assume by the section title,

Yes, you are right. On methods it looks even more veird.

[romani]

While discussion is not finished

Minor:

[rnveach]

I am fine to continue but I still think it should be configurable.

[romani]

Let's wait when users come up with request ( separate issue), and they will explain reasons of such style.

[rnveach]

Documentation at http://checkstyle.sourceforge.net/config_misc.html#ArrayTypeStyle , top of ArrayTypeStyleCheck, and http://checkstyle.sourceforge.net/checks.html needs to be updated with description of check and a example.

[esilkensen]

Thank you both for the comments - I have added some additional test cases and documentation.

[romani]

Behavior need to be changed:

[romani]

I think we need to be consistent and allow that weird location of [] when property is set to false.

In my previous comment I meant that I do not know who likes String strings[];, even 20 year ago, it was not good way to declare array.

[esilkensen]

Sounds good. So should we say: method return types are only checked when javaStyle is true? Then if that property is false, either style is OK?

[rnveach]

@romani Please confirm.
@esilkensen If logic is changing, documentation also needs to be updated again.

[esilkensen]

@rnveach I tried to update the documentation to reflect the current logic with my last commit. Is there a spot in particular you think it should be changed?

Anyway, thank you both for your patience - I am sorry for the churn, I just want to implement the logic that you would like to see for this check.

• Initial version of the PR: method return type always checked (and must be "Java style")
• Current version of the PR: method return type only checked when javaStyle is true

I was interpreting your first comments @romani as agreeing with that first version, then interpreted your most recent comment as saying we should have the second version.

Any approach is ok with me (or maybe a third one with a separate property configuring the method return check specifically?) - just let me know what you would like and I will make the changes.

[romani]

varieble/field type and method type are always checked.
if javaStyle="true" (default), then "[]" should be on type, only.
if javaStyle="false" then "[]" can be at other place.

Please extend documentation with examples to clearly show user what behavior will be. Examples works better than some explanation of logic.

[esilkensen]

Sorry, one more clarification:

varieble/field type and method type are always checked.
if javaStyle="true" (default), then "[]" should be on type, only.
if javaStyle="false" then "[]" can be at other place.

With the current logic in the PR: if javaStyle="false", then "[]" can be at the other place (for method return types), but it doesn't have to be.

Do you want it to be: if javaStyle="false" then "[]" should be at other place, only?

The reason I thought it might make sense to not make char[] toCharArray() a violation when javaStyle="false" is that it isn't really "C style" since it is not possible to actually return a [] type in C/C++ -- I don't know where this char toCharArray()[] syntax came from, but it doesn't seem like C/C++.

I thought it might be likely that a user would want to enforce int nums[]; style while still writing char[] toCharArray() for methods - but, I wonder how many users actually enable this check with javaStyle="false" in the first place - maybe it's no big deal either way.

Anyway, I am happy to make the change so that javaStyle="false" means char[] toCharArray() is a violation, and will do that now.

[romani]

Just rechecked, as a long time passed from C time...
tested at https://www.onlinegdb.com/online_c_compiler

#include <stdio.h>

int fun()[] 
{
    static int arr[100];    
    return arr; 
} 

int main()
{
    int ptr[] = fun(arr); 
    return 0;
}

error:

main.c:4:5: error: ‘fun’ declared as function returning an array
 int fun()[] 
     ^
main.c: In function ‘fun’:
main.c:7:12: warning: return makes integer from pointer without a cast [-Wint-conversion]
     return arr; 
            ^
main.c: In function ‘main’:
main.c:12:21: error: ‘arr’ undeclared (first use in this function)
     int ptr[] = fun(arr); 
                     ^
main.c:12:21: note: each undeclared identifier is reported only once for each function it appears in

You are right.
it is not possible(compilation error) to return array as value from function.

https://stackoverflow.com/a/14297191
https://stackoverflow.com/questions/3473438/return-array-in-a-function

So it is ok to forbid char toCharArray()[] when javaStyle="false", but we need to explain this in documentation, as it is very tiny nuance.

@rnveach , please confirm that you are ok with such behavior.

[rnveach]

@esilkensen @romani I am ok. It sounds like we are reverting back to the first instance of this PR where all []s on method name are forbidden.

[esilkensen]

Thanks @rnveach , @romani, I have updated the code and documentation with an extra note - let me know any further changes you would like to see. (Also, next time I will ask questions/clarify behavior in the issue first, if unsure about the implementation, hopefully for a smoother PR!)

[romani]

items to improve:

[romani]

assigned to @rnveach , to let him finish review first. I will be last reviewer.

[romani]

code is ok, please provide regression diff report for both modes of Check.

[esilkensen]

Ran the regression report twice with the latest code:

• http://esilkensen.github.io/checkstyle-tester/6301/true (javaStyle="true" )
• http://esilkensen.github.io/checkstyle-tester/6301/false (javaStyle="false")

[romani]

Reports looks good, I even surprised that we found bunch of such cases.

I am ok to merge.

[rnveach]

just minor rewording and re-arranging.

[rnveach]

I am good with regression.

[esilkensen]

@rnveach I have updated docs following your latest comments, let me know if anything needs to be changed. Thanks!

[rnveach]

See #6428 (comment) . Once that is done I think I will be good to merge.

[esilkensen]

Ah that's right, thanks for catching that! Fixed now.

[rnveach]

Sorry, 1 last update then I am done.