ImportOrder: replace Pattern[] with String[] type for properties staticGroups and groups

[romani]

from #13754 (comment)

We think our documentation at https://checkstyle.org/property_types.html#Pattern.5B.5D is either wrong or misleading.

Found looking at https://checkstyle.org/checks/metrics/classfanoutcomplexity.html#Properties

The string representation is parsed as a set of forward slash ('/') separated patterns.

When glancing at this, i thought it meant each pattern is separated by a /.

But looking at the code at

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/AbstractClassCouplingCheck.java

Line 160 in 5a0f923

public void setExcludeClassesRegexps(String... from) {

, it takes input as a string array, which is separated by the comma and not the /.

Even createPattern method that it calls does nothing with /, so I don't understand where the / is coming from.

https://github.com/search?q=repo%3Acheckstyle%2Fcheckstyle+%22Pattern.5B.5D%22+path%3A%2F%5Esrc%5C%2Fxdocs%5C%2Fchecks%5C%2F%2F&type=code
We have 6 checks with this type.

It looks like documentation is coming from

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java

Line 759 in 5a0f923

else if (pkg.startsWith(FORWARD_SLASH)) {

, which is a serious problem now as we have a documented type being parsed differently in different checks, meaning we can't even unify documentation without first changing production code.

---

We need review all Patern[] usage and probably move ImportOrderCheck to have special type.
how we can support list of Pattern, when "," can be used in regexp.

---

agreement on how to fix we archived at #14764 (comment)

---

Migration Notes:

ImportOrder is update to replace Pattern[] with String[] type for properties staticGroups and groups.
user who use xml config are not affected, but plugins who might have type validation during Check configuration (before Check execution) might be affected. Validation type in metadata is different from 10.17.0 for this Check.

[rnveach]

global types = types that we recognize through our process of

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java

Lines 110 to 111 in 5a0f923

	registerIntegralTypes(cub);
	registerCustomTypes(cub);

.

If I make a 3rd party check, I expect every type listed on that page ( https://checkstyle.org/property_types.html ) to work as described if I were to use them in a property in said 3rd party check. Otherwise, it would confusing on what we do and do not support.

[Lmh-java]

I will give a try on this issue.

[Lmh-java]

@rnveach @romani It looks like the group and staticGroup property of ImportOrderCheck support both plain prefixes of classes and regex at the same time for matching import names. Even if the user passes in plain prefixes of classes without using regex, we will automatically convert it to a regex expression.

Below is the code segment of ImportOrderCheck#compilePatterns(String):

if (WILDCARD_GROUP_NAME.equals(pkg)) {
    // matches any package
    grp = Pattern.compile("");
}
else if (pkg.startsWith(FORWARD_SLASH)) {
    if (!pkg.endsWith(FORWARD_SLASH)) {
        throw new IllegalArgumentException("Invalid group: " + pkg);
    }
    pkg = pkg.substring(1, pkg.length() - 1);
    grp = Pattern.compile(pkg);
}
else {
    final StringBuilder pkgBuilder = new StringBuilder(pkg);
    if (!pkg.endsWith(".")) {
        pkgBuilder.append('.');
    }
    grp = Pattern.compile("^" + Pattern.quote(pkgBuilder.toString()));
}

This could be very confusing. Instead enforcing users to use / to wrap regex expression, would it be better to directly enforce users to use regex expressions?

Update: alternatively, maybe it's better to split this group property into groupRegex for pure regex type Pattern[] and groupPrefix for pure String[]. Same to staticGroup property.

[Lmh-java]

It looks like the group and staticGroup property of ImportOrderCheck support both plain prefixes of classes and regex at the same time for matching import names. Even if the user passes in plain prefixes of classes without using regex, we will automatically convert it to a regex expression.

Below is the code segment of ImportOrderCheck#compilePatterns(String):

if (WILDCARD_GROUP_NAME.equals(pkg)) {
    // matches any package
    grp = Pattern.compile("");
}
else if (pkg.startsWith(FORWARD_SLASH)) {
    if (!pkg.endsWith(FORWARD_SLASH)) {
        throw new IllegalArgumentException("Invalid group: " + pkg);
    }
    pkg = pkg.substring(1, pkg.length() - 1);
    grp = Pattern.compile(pkg);
}
else {
    final StringBuilder pkgBuilder = new StringBuilder(pkg);
    if (!pkg.endsWith(".")) {
        pkgBuilder.append('.');
    }
    grp = Pattern.compile("^" + Pattern.quote(pkgBuilder.toString()));
}

This could be very confusing. Instead enforcing users to use / to wrap regex expression and mix regex with plain texts, would it be better to directly enforce users to use regex expressions directly?

If we can enforce this, we can get rid of the delim / and unify every Pattern[] property to use , or other specific delim we can decide later on.

[Lmh-java]

If this proposal works, I will:

1. Send a PR to modify ImportOrderCheck to one of the above.
2. Send multiple PRs to enforce a specific delimiter in Pattern[] property, adjust relevant checks if needed, and alter corresponding documentation.

[romani]

I see that type at ImportOrderCheck is correct.
Description pattern[] should be updated to remove reference to /.
Doc of ImportOrderCheck.groups should be updated if somebody see a way, but this it is ok.

@Lmh-java, please create new PR with only what I referencing in this post, I am voting to do only this update, not a new global type as it has problems in design, as I mentioned in your PR.

[Lmh-java]

@romani Thanks for your response! Sure, I will create a new PR with only documentation changes.

For discussion:
For the delimiter of Pattern[], is it possible for us to not use a single character as a delimiter, instead, we enforce users to use expressions like ['regex1', 'regex2', 'regex3'] for a property of Pattern[] type, so we can clearly identify different regex expressions for the global type. (We quote every single regex expression, and use comma to split different quotations. If users still want to match ' or , with regex, they can still use it.

[romani]

we enforce users to use expressions like

It might be too late. Users will be frustrated. We need to respect user backward compatibility, and use it only when absolutely required.

[nrmancuso]

I have read through all the linked PRs, comments, etc. and I am struggling to understand why we need Pattern[] at all.

Is there some conceptual difference between this and a String[] until we compile the expression? This is an implementation detail that we don't need to expose. Users provide an array of strings to a check module, and they should not care what we do with it after that.

[Lmh-java]

Is there some conceptual difference between this and a String[] until we compile the expression?

@nrmancuso, Basically, this property works by converting String[] to Pattern[].

My reasoning for this is that we have some checks that use this Pattern[], and all they were using String[] to take in the property and converted them to Pattern[] in the check. This can be simplified if we directly take in properties as Pattern[]. Also for checkstyle developers, they do not need to do this conversion anymore. We have a global convertor that does this for them.

This property might also remind checkstyle users about the nature of some properties. If the type of a property is Pattern[], then they know it is a list of regex. Otherwise, we might need to use English words to claify the usage of a String[] every time. Since we have both String and Pattern, we might also want Pattern[] with String[].

Please feel free to correct me :).

[nrmancuso]

Please feel free to correct me :).

No need for correction in any case, it is a matter of opinion.

This property might also remind checkstyle users about the nature of some properties.

Yes, this is good, I agree.

[nrmancuso]

@romani please update the issue description to reflect what is left to do here.

[romani]

I do not remember what is left, look like we fixed all.

#14857 is postponed for future.