Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #13758: Add pattern array converter for auto bean #14740

Merged
merged 1 commit into from May 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -645,21 +645,6 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/AbstractClassCouplingCheck.java</fileName>
<specifier>methodref.receiver.bound</specifier>
<message>Incompatible receiver type</message>
<lineContent>.forEach(excludeClassesRegexps::add);</lineContent>
<details>
found : @GuardedBy List&lt;@GuardedBy Pattern&gt;
required: @GuardSatisfied List&lt;@GuardedBy Pattern&gt;
Consequence: method
@GuardedBy List&lt;@GuardedBy Pattern&gt;
is not a valid method reference for method in @GuardedBy List&lt;@GuardedBy Pattern&gt;
@GuardedBy boolean add(@GuardSatisfied List&lt;@GuardedBy Pattern&gt; this, @GuardedBy Pattern p0)
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/naming/AccessModifierOption.java</fileName>
<specifier>method.guarantee.violated</specifier>
Expand Down
2 changes: 2 additions & 0 deletions config/linkcheck-suppressions.txt
Expand Up @@ -35,6 +35,7 @@
<a href="AbstractNode.html#getDeclaredNamespaces(net.sf.saxon.om.NamespaceBinding%5B%5D)">AbstractNode.html#getDeclaredNamespaces(net.sf.saxon.om.NamespaceBinding%5B%5D)</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.OutputStreamOptions.html#%3Cinit%3E()">#%3Cinit%3E()</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.PatternConverter.html#%3Cinit%3E()">#%3Cinit%3E()</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.PatternArrayConverter.html#%3Cinit%3E()">#%3Cinit%3E()</a>: doesn't exist.
romani marked this conversation as resolved.
Show resolved Hide resolved
<a href="apidocs/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.RelaxedAccessModifierArrayConverter.html#%3Cinit%3E()">#%3Cinit%3E()</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.RelaxedStringArrayConverter.html#%3Cinit%3E()">#%3Cinit%3E()</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.ScopeConverter.html#%3Cinit%3E()">#%3Cinit%3E()</a>: doesn't exist.
Expand Down Expand Up @@ -801,6 +802,7 @@
<a href="apidocs/com/puppycrawl/tools/checkstyle/xpath/iterators/ReverseListIterator.html#%3Cinit%3E(java.util.Collection)">#%3Cinit%3E(java.util.Collection)</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/site/XdocsTemplateSink.CustomPrintWriter.html#%3Cinit%3E(java.io.Writer)">#%3Cinit%3E(java.io.Writer)</a>: doesn't exist.
<a href="com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.OutputStreamOptions.html#%3Cinit%3E()">com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.OutputStreamOptions.html#%3Cinit%3E()</a>: doesn't exist.
<a href="com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.PatternArrayConverter.html#%3Cinit%3E()">com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.PatternArrayConverter.html#%3Cinit%3E()</a>: doesn't exist.
<a href="com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.PatternConverter.html#%3Cinit%3E()">com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.PatternConverter.html#%3Cinit%3E()</a>: doesn't exist.
<a href="com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.RelaxedAccessModifierArrayConverter.html#%3Cinit%3E()">com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.RelaxedAccessModifierArrayConverter.html#%3Cinit%3E()</a>: doesn't exist.
<a href="com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.RelaxedStringArrayConverter.html#%3Cinit%3E()">com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.RelaxedStringArrayConverter.html#%3Cinit%3E()</a>: doesn't exist.
Expand Down
3 changes: 2 additions & 1 deletion config/pmd.xml
Expand Up @@ -316,7 +316,8 @@
<!-- Main has an annotated field for each command line option. This is by design. -->
<property name="violationSuppressXPath"
value="//ClassOrInterfaceDeclaration[@SimpleName='Checker']
| //ClassOrInterfaceDeclaration[@SimpleName='Main']"/>
| //ClassOrInterfaceDeclaration[@SimpleName='Main']
| //ClassOrInterfaceDeclaration[@SimpleName='ImportOrderCheck']"/>
Copy link
Contributor Author

@Lmh-java Lmh-java Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppress this because PWD complains that ImportOrderCheck has too many class fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with this.

</properties>
</rule>
<rule ref="category/java/design.xml/TooManyMethods">
Expand Down
1 change: 1 addition & 0 deletions pom.xml
Expand Up @@ -3675,6 +3675,7 @@
<targetClasses>
<param>com.puppycrawl.tools.checkstyle.checks.imports.*</param>
</targetClasses>
<avoidCallsTo>com.puppycrawl.tools.checkstyle.utils.UnmodifiableCollectionUtil</avoidCallsTo>
<targetTests>
<param>com.puppycrawl.tools.checkstyle.checks.imports.*</param>
</targetTests>
Expand Down
Expand Up @@ -170,6 +170,7 @@ private static void registerIntegralTypes(ConvertUtilsBean cub) {
*/
private static void registerCustomTypes(ConvertUtilsBean cub) {
cub.register(new PatternConverter(), Pattern.class);
cub.register(new PatternArrayConverter(), Pattern[].class);
cub.register(new SeverityLevelConverter(), SeverityLevel.class);
cub.register(new ScopeConverter(), Scope.class);
cub.register(new UriConverter(), URI.class);
Expand Down Expand Up @@ -313,6 +314,25 @@ public Object convert(Class type, Object value) {

}

/** A converter that converts a comma-separated string into an array of patterns. */
private static final class PatternArrayConverter implements Converter {

@SuppressWarnings("unchecked")
@Override
public Object convert(Class type, Object value) {
final StringTokenizer tokenizer = new StringTokenizer(
value.toString(), COMMA_SEPARATOR);
final List<Pattern> result = new ArrayList<>();

while (tokenizer.hasMoreTokens()) {
final String token = tokenizer.nextToken();
result.add(CommonUtil.createPattern(token.trim()));
}

return result.toArray(new Pattern[0]);
}
}

/** A converter that converts strings to severity level. */
private static final class SeverityLevelConverter implements Converter {

Expand Down
Expand Up @@ -28,6 +28,8 @@
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.FullIdent;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.utils.CommonUtil;
import com.puppycrawl.tools.checkstyle.utils.UnmodifiableCollectionUtil;

/**
* <p>
Expand Down Expand Up @@ -74,7 +76,7 @@
* (e.g. {@code /regexp/}). All type imports, which does not match any group, falls into an
* additional group, located at the end.
* Thus, the empty list of type groups (the default value) means one group for all type imports.
* Type is {@code java.util.regex.Pattern[]}.
* Type is {@code java.lang.String[]}.
* Default value is {@code ""}.
* </li>
* <li>
Expand Down Expand Up @@ -118,7 +120,7 @@
* an additional group, located at the end. Thus, the empty list of static groups (the default
* value) means one group for all static imports. This property has effect only when the property
* {@code option} is set to {@code top} or {@code bottom}.
* Type is {@code java.util.regex.Pattern[]}.
* Type is {@code java.lang.String[]}.
* Default value is {@code ""}.
* </li>
* <li>
Expand Down Expand Up @@ -186,7 +188,7 @@ public class ImportOrderCheck
* located at the end. Thus, the empty list of type groups (the default value) means one group
* for all type imports.
*/
private Pattern[] groups = EMPTY_PATTERN_ARRAY;
private String[] groups = CommonUtil.EMPTY_STRING_ARRAY;

/**
* Specify list of <b>static</b> import groups. Every group identified either by a common prefix
Expand All @@ -196,7 +198,7 @@ public class ImportOrderCheck
* static imports. This property has effect only when the property {@code option} is set to
* {@code top} or {@code bottom}.
*/
private Pattern[] staticGroups = EMPTY_PATTERN_ARRAY;
private String[] staticGroups = CommonUtil.EMPTY_STRING_ARRAY;

/**
* Control whether type import groups should be separated by, at least, one blank
Expand Down Expand Up @@ -262,6 +264,16 @@ public class ImportOrderCheck
*/
private ImportOrderOption option = ImportOrderOption.UNDER;

/**
* Complied array of patterns for property {@code groups}.
*/
private Pattern[] groupsReg = EMPTY_PATTERN_ARRAY;

/**
* Complied array of patterns for property {@code staticGroups}.
*/
private Pattern[] staticGroupsReg = EMPTY_PATTERN_ARRAY;

Comment on lines +267 to +276
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add these two more fields to store the compiled regex expressions. I have to expose String[] to users, but we also need to store the complied regex (we don't want to compile every time we want to use the regex). I am curious whether there are better solutions to this, since this change brings us more suppressions of all kinds.

Copy link
Member

@romani romani Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To next reviewers:
to avoid dealing with long discussion on what is actually a type that user should care, I am ok with this update.
I hope in some future we will agree on type system and simplify this Check

/**
* Setter to specify policy on the relative order between type imports and static imports.
*
Expand All @@ -284,7 +296,8 @@ public void setOption(String optionStr) {
* @since 3.2
*/
public void setGroups(String... packageGroups) {
groups = compilePatterns(packageGroups);
groups = UnmodifiableCollectionUtil.copyOfArray(packageGroups, packageGroups.length);
groupsReg = compilePatterns(packageGroups);
}

/**
Expand All @@ -299,7 +312,8 @@ public void setGroups(String... packageGroups) {
* @since 8.12
*/
public void setStaticGroups(String... packageGroups) {
staticGroups = compilePatterns(packageGroups);
staticGroups = UnmodifiableCollectionUtil.copyOfArray(packageGroups, packageGroups.length);
staticGroupsReg = compilePatterns(packageGroups);
}

/**
Expand Down Expand Up @@ -662,10 +676,10 @@ private static String getImportContainer(String qualifiedImportName) {
private int getGroupNumber(boolean isStatic, String name) {
final Pattern[] patterns;
if (isStatic) {
patterns = staticGroups;
patterns = staticGroupsReg;
}
else {
patterns = groups;
patterns = groupsReg;
}

int number = getGroupNumber(patterns, name);
Expand Down
Expand Up @@ -156,10 +156,8 @@ public final void setExcludedClasses(String... excludedClasses) {
*
* @param from array representing regular expressions of classes to ignore.
*/
public void setExcludeClassesRegexps(String... from) {
Arrays.stream(from)
.map(CommonUtil::createPattern)
.forEach(excludeClassesRegexps::add);
public void setExcludeClassesRegexps(Pattern... from) {
excludeClassesRegexps.addAll(Arrays.asList(from));
}

/**
Expand Down
Expand Up @@ -40,7 +40,7 @@
&lt;a href="https://en.wikipedia.org/wiki/ASCII#Order"&gt;ASCII sort order&lt;/a&gt;.
It affects both type imports and static imports.</description>
</property>
<property default-value="" name="groups" type="java.util.regex.Pattern[]">
<property default-value="" name="groups" type="java.lang.String[]">
Copy link
Member

@rnveach rnveach Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has it been discussed what impact this has on 3rd party tools like eclipse-cs or sonar-cs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be declared breaking compatibility, I hope eclipscs will not be actually affected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are not going to do any analysis or warning?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let move forward, and just declare breaking changes.

<description>Specify list of &lt;b&gt;type import&lt;/b&gt; groups. Every group identified
either by a common prefix string, or by a regular expression enclosed in forward slashes
(e.g. {@code /regexp/}). All type imports, which does not match any group, falls into an
Expand Down Expand Up @@ -75,9 +75,7 @@
<description>Control whether
&lt;b&gt;static imports&lt;/b&gt; located at &lt;b&gt;top&lt;/b&gt; or &lt;b&gt;bottom&lt;/b&gt; are sorted within the group.</description>
</property>
<property default-value=""
name="staticGroups"
type="java.util.regex.Pattern[]">
<property default-value="" name="staticGroups" type="java.lang.String[]">
<description>Specify list of &lt;b&gt;static&lt;/b&gt; import groups. Every group
identified either by a common prefix string, or by a regular expression enclosed in forward
slashes (e.g. {@code /regexp/}). All static imports, which does not match any group, fall into
Expand Down
Expand Up @@ -23,7 +23,9 @@

import java.net.URI;
import java.util.Arrays;
import java.util.List;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import org.apache.commons.beanutils.ConversionException;
import org.apache.commons.beanutils.ConvertUtilsBean;
Expand Down Expand Up @@ -278,6 +280,60 @@ public void testBeanConvertersUri3() {
}
}

@Test
public void testBeanConverterPatternArray() throws Exception {
final ConverterBean bean = new ConverterBean();
final DefaultConfiguration config = new DefaultConfiguration("bean");
final String patternString = "^a*$ , ^b*$ , ^c*$ ";
final List<String> expectedPatternStrings = Arrays.asList("^a*$", "^b*$", "^c*$");
config.addProperty("patterns", patternString);
bean.configure(config);

final List<String> actualPatternStrings = Arrays.stream(bean.patterns)
.map(Pattern::pattern)
.collect(Collectors.toUnmodifiableList());

assertWithMessage("invalid size of result")
.that(bean.patterns)
.hasLength(3);
assertWithMessage("invalid result")
.that(actualPatternStrings)
.containsExactlyElementsIn(expectedPatternStrings);
}

@Test
public void testBeanConverterPatternArraySingleElement() throws Exception {
final ConverterBean bean = new ConverterBean();
final DefaultConfiguration config = new DefaultConfiguration("bean");
final String patternString = "^a*$";
final List<String> expectedPatternStrings = List.of("^a*$");
config.addProperty("patterns", patternString);
bean.configure(config);

final List<String> actualPatternStrings = Arrays.stream(bean.patterns)
.map(Pattern::pattern)
.collect(Collectors.toUnmodifiableList());

assertWithMessage("invalid size of result")
.that(bean.patterns)
.hasLength(1);
assertWithMessage("invalid result")
.that(actualPatternStrings)
.containsExactlyElementsIn(expectedPatternStrings);
}

@Test
public void testBeanConverterPatternArrayEmptyString() throws Exception {
final ConverterBean bean = new ConverterBean();
final DefaultConfiguration config = new DefaultConfiguration("bean");
config.addProperty("patterns", "");
bean.configure(config);
romani marked this conversation as resolved.
Show resolved Hide resolved

assertWithMessage("invalid size of result")
.that(bean.patterns)
.hasLength(0);
}

private static final class ConvertUtilsBeanStub extends ConvertUtilsBean {

private int registerCount;
Expand Down Expand Up @@ -338,6 +394,7 @@ public static class ConverterBean extends AbstractAutomaticBean {
private Scope scope;
private URI uri;
private AccessModifierOption[] accessModifiers;
private Pattern[] patterns;

/**
* Setter for strings.
Expand Down Expand Up @@ -394,6 +451,15 @@ public void setAccessModifiers(AccessModifierOption... accessModifiers) {
accessModifiers.length);
}

/**
* Setter for patterns.
*
* @param patterns patterns
*/
public void setPatterns(Pattern... patterns) {
this.patterns = Arrays.copyOf(patterns, patterns.length);
}

@Override
protected void finishLocalSetup() {
// no code
Expand Down
Expand Up @@ -377,4 +377,18 @@ public void testClearStatePackageName() throws Exception {
.isTrue();
}

/**
* Test [ClassName]::new expression. Such expression will be processed as a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this have to do with the pattern array converter? All the properties in the file are default which means it doesn't go against the converter, right?

Copy link
Contributor Author

@Lmh-java Lmh-java Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has nothing to do with the converter. Please see #14740 (comment).

This is a part of the workaround for ClassFanOutCheck until #14787

TLDR;
I have to keep the following line, otherwise the CI will fail.

excludeClassesRegexps.add(CommonUtil.createPattern("^$"));

Added this test case to kill Pitest mutation for this line. After #14787 is solved, this test case will report violations.

* normal declaration of a new instance. We need to make sure this does not
*
* @throws Exception when code tested throws exception
*/
@Test
public void testLambdaNew() throws Exception {
final String[] expected = {};
// no violation until #14787
verifyWithInlineConfigParser(
getPath("InputClassFanOutComplexityLambdaNew.java"), expected);
}

}
@@ -0,0 +1,36 @@
/*
ClassFanOutComplexity
max = 0
excludedClasses = (default)ArrayIndexOutOfBoundsException, ArrayList, Boolean, Byte, \
Character, Class, Collection, Deprecated, Deque, Double, DoubleStream, \
EnumSet, Exception, Float, FunctionalInterface, HashMap, HashSet, \
IllegalArgumentException, IllegalStateException, IndexOutOfBoundsException, \
IntStream, Integer, LinkedHashMap, LinkedHashSet, LinkedList, List, Long, \
LongStream, Map, NullPointerException, Object, Optional, OptionalDouble, \
OptionalInt, OptionalLong, Override, Queue, RuntimeException, SafeVarargs, \
SecurityException, Set, Short, SortedMap, SortedSet, Stream, String, \
StringBuffer, StringBuilder, SuppressWarnings, Throwable, TreeMap, TreeSet, \
UnsupportedOperationException, Void, boolean, byte, char, double, float, \
int, long, short, var, void
excludedPackages = (default)


*/

package com.puppycrawl.tools.checkstyle.checks.metrics.classfanoutcomplexity;

import java.util.List;
import java.util.Map;
import java.util.ArrayList;
import java.io.File;

public class InputClassFanOutComplexityLambdaNew {
public void testMethod(Map<String, List<String>> entry) {
List<File> files = new ArrayList<>();
String string = "";
entry.values().stream()
.flatMap(List::stream)
.map(File::new)
.forEach(files::add);
}
}
4 changes: 2 additions & 2 deletions src/xdocs/checks/imports/importorder.xml
Expand Up @@ -53,7 +53,7 @@
<tr>
<td>groups</td>
<td>Specify list of <b>type import</b> groups. Every group identified either by a common prefix string, or by a regular expression enclosed in forward slashes (e.g. <code>/regexp/</code>). All type imports, which does not match any group, falls into an additional group, located at the end. Thus, the empty list of type groups (the default value) means one group for all type imports.</td>
<td><a href="../../property_types.html#Pattern.5B.5D">Pattern[]</a></td>
<td><a href="../../property_types.html#String.5B.5D">String[]</a></td>
<td><code>{}</code></td>
<td>3.2</td>
</tr>
Expand Down Expand Up @@ -95,7 +95,7 @@
<tr>
<td>staticGroups</td>
<td>Specify list of <b>static</b> import groups. Every group identified either by a common prefix string, or by a regular expression enclosed in forward slashes (e.g. <code>/regexp/</code>). All static imports, which does not match any group, fall into an additional group, located at the end. Thus, the empty list of static groups (the default value) means one group for all static imports. This property has effect only when the property <code>option</code> is set to <code>top</code> or <code>bottom</code>.</td>
<td><a href="../../property_types.html#Pattern.5B.5D">Pattern[]</a></td>
<td><a href="../../property_types.html#String.5B.5D">String[]</a></td>
<td><code>{}</code></td>
<td>8.12</td>
</tr>
Expand Down