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 #14424: Add option to skip validation based on list of annotations #14701

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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 @@ -19,10 +19,14 @@

package com.puppycrawl.tools.checkstyle.checks.design;

import java.util.Collections;
import java.util.Set;

import com.puppycrawl.tools.checkstyle.StatelessCheck;
import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.utils.AnnotationUtil;

/**
* <p>
Expand Down Expand Up @@ -51,6 +55,19 @@
* }
* }
* </pre>
* <ul>
* <li>
* Property {@code ignoreAnnotatedBy} - Ignore classes annotated
* with the specified annotation(s). Annotation names provided in this property
* must exactly match the annotation names on the classes. If the target class has annotations
* specified with their fully qualified names (including package), the annotations in this
* property should also be specified with their fully qualified names. Similarly, if the target
* class has annotations specified with their simple names, this property should contain the
* annotations with the same simple names.
* Type is {@code java.lang.String[]}.
* Default value is {@code ""}.
* </li>
* </ul>
* <p>
* Parent is {@code com.puppycrawl.tools.checkstyle.TreeWalker}
* </p>
Expand All @@ -74,6 +91,33 @@ public class HideUtilityClassConstructorCheck extends AbstractCheck {
*/
public static final String MSG_KEY = "hide.utility.class";

/**
* Ignore classes annotated with the specified annotation(s). Annotation names
* provided in this property must exactly match the annotation names on the classes.
* If the target class has annotations specified with their fully qualified names
* (including package), the annotations in this property should also be specified with
* their fully qualified names. Similarly, if the target class has annotations specified
* with their simple names, this property should contain the annotations with the same
* simple names.
*/
private Set<String> ignoreAnnotatedBy = Collections.emptySet();

/**
* Setter to ignore classes annotated with the specified annotation(s). Annotation names
* provided in this property must exactly match the annotation names on the classes.
* If the target class has annotations specified with their fully qualified names
* (including package), the annotations in this property should also be specified with
* their fully qualified names. Similarly, if the target class has annotations specified
* with their simple names, this property should contain the annotations with the same
* simple names.
*
* @param annotationNames specified annotation(s)
* @since 10.17.0
*/
public void setIgnoreAnnotatedBy(String... annotationNames) {
ignoreAnnotatedBy = Set.of(annotationNames);
}

@Override
public int[] getDefaultTokens() {
return getRequiredTokens();
Expand All @@ -92,7 +136,7 @@ public int[] getRequiredTokens() {
@Override
public void visitToken(DetailAST ast) {
// abstract class could not have private constructor
if (!isAbstract(ast)) {
if (!isAbstract(ast) && !shouldIgnoreClass(ast)) {
romani marked this conversation as resolved.
Show resolved Hide resolved
final boolean hasStaticModifier = isStatic(ast);

final Details details = new Details(ast);
Expand Down Expand Up @@ -142,6 +186,16 @@ private static boolean isStatic(DetailAST ast) {
.findFirstToken(TokenTypes.LITERAL_STATIC) != null;
}

/**
* Checks if class is annotated by specific annotation(s) to skip.
*
* @param ast class to check
* @return true if annotated by ignored annotations
*/
private boolean shouldIgnoreClass(DetailAST ast) {
return AnnotationUtil.containsAnnotation(ast, ignoreAnnotatedBy);
}

/**
* Details of class that are required for validation.
*/
Expand Down
Expand Up @@ -30,6 +30,17 @@
}
}
&lt;/pre&gt;</description>
<properties>
<property default-value="" name="ignoreAnnotatedBy" type="java.lang.String[]">
<description>Ignore classes annotated
with the specified annotation(s). Annotation names provided in this property
must exactly match the annotation names on the classes. If the target class has annotations
specified with their fully qualified names (including package), the annotations in this
property should also be specified with their fully qualified names. Similarly, if the target
class has annotations specified with their simple names, this property should contain the
annotations with the same simple names.</description>
</property>
</properties>
<message-keys>
<message-key key="hide.utility.class"/>
</message-keys>
Expand Down
Expand Up @@ -146,4 +146,26 @@ public void testGetAcceptableTokens() {
.isEqualTo(expected);
}

@Test
public void testIgnoreAnnotatedBy() throws Exception {
final String[] expected = {
"30:1: " + getCheckMessage(MSG_KEY),
};
verifyWithInlineConfigParser(
getPath("InputHideUtilityClassConstructorIgnoreAnnotationBy.java"),
expected
);
}

@Test
public void testIgnoreAnnotatedByFullQualifier() throws Exception {
final String[] expected = {
"9:1: " + getCheckMessage(MSG_KEY),
};
verifyWithInlineConfigParser(
getPath("InputHideUtilityClassConstructor"
+ "IgnoreAnnotationByFullyQualifiedName.java"),
expected
);
}
}
@@ -0,0 +1,46 @@
/*
HideUtilityClassConstructor
ignoreAnnotatedBy = Skip, SkipWithParam, SkipWithAnnotationAsParam
*/

package com.puppycrawl.tools.checkstyle.checks.design.hideutilityclassconstructor;

@Skip
public class InputHideUtilityClassConstructorIgnoreAnnotationBy {
public static void func() {}
}

@SkipWithParam(name = "tool1")
class ToolClass1 {
public static void func() {}
}

@SkipWithAnnotationAsParam(skip = @Skip)
class ToolClass2 {
public static void func() {}
}

@CommonAnnot
@Skip
class ToolClass3 {
public static void func() {}
}

@CommonAnnot // violation
class ToolClass4 {
public static void func() {}
}


@interface Skip {}

@interface SkipWithParam {
String name();
}

@interface SkipWithAnnotationAsParam {
Skip skip();
}

@interface CommonAnnot {}
@@ -0,0 +1,19 @@
/*
HideUtilityClassConstructor
ignoreAnnotatedBy = java.lang.Deprecated
*/

package com.puppycrawl.tools.checkstyle.checks.design.hideutilityclassconstructor;

@Deprecated // violation
public class InputHideUtilityClassConstructorIgnoreAnnotationByFullyQualifiedName {
public static void func() {}
}

@java.lang.Deprecated
class DeprecatedClass {
public static void func() {}
}

@interface Deprecated {}
Expand Up @@ -36,9 +36,25 @@ protected String getPackageLocation() {
public void testExample1() throws Exception {
final String[] expected = {
"12:1: " + getCheckMessage(MSG_KEY),
"37:1: " + getCheckMessage(MSG_KEY),
"38:1: " + getCheckMessage(MSG_KEY),
};

verifyWithInlineConfigParser(getPath("Example1.java"), expected);
}

@Test
public void testExample2() throws Exception {
final String[] expected = {};

verifyWithInlineConfigParser(getPath("Example2.java"), expected);
}

@Test
public void testExample3() throws Exception {
final String[] expected = {
"15:1: " + getCheckMessage(MSG_KEY),
};

verifyWithInlineConfigParser(getPath("Example3.java"), expected);
}
}
Expand Up @@ -9,7 +9,8 @@
package com.puppycrawl.tools.checkstyle.checks.design.hideutilityclassconstructor;

// xdoc section -- start
class Example1 { // violation
@java.lang.Deprecated // violation
class Example1 {

public Example1() {
}
Expand All @@ -18,23 +19,24 @@ public static void fun() {
}
}

class Foo { // OK
class Foo1 {

private Foo() {
private Foo1() {
}

static int n;
}

class Bar { // OK
class Bar1 {

protected Bar() {
protected Bar1() {
// prevents calls from subclass
throw new UnsupportedOperationException();
}
}

class UtilityClass { // violation
@SpringBootApplication // violation
class ApplicationClass1 {

static float f;
}
Expand Down
@@ -0,0 +1,47 @@
/*xml
<module name="Checker">
<module name="TreeWalker">
<module name="HideUtilityClassConstructor">
<property name="ignoreAnnotatedBy"
value="SpringBootApplication, java.lang.Deprecated" />
Copy link
Member

Choose a reason for hiding this comment

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

Annotation names provided in this property must exactly match the annotation names on the classes. If the target class has annotations specified with their fully qualified names (including package), the annotations in this property should also be specified with their fully qualified names. Similarly, if the target class has annotations specified with their simple names, this property should contain the annotations with the same simple names.

Show some of these issues in your example.

I assume Deprecated by itself won't work. Show that and put a comment to that effect.

Copy link
Member

Choose a reason for hiding this comment

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

Can you also provide me where we said we would be so strict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume Deprecated by itself won't work.

Only Deprecated will not work if it doesn't match the annotation used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you also provide me where we said we would be so strict?

We mentioned this at #14424 (comment). We agreed to do exact match first as #14553. We might do pattern matching later, but they need to solve the problem of #5434 first.

Copy link
Member

@romani romani May 3, 2024

Choose a reason for hiding this comment

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

yes, I am fine to be strict for first step, we can improve later if sombody start to complain.
We just need to document this.

Copy link
Contributor Author

@Lmh-java Lmh-java May 12, 2024

Choose a reason for hiding this comment

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

Annotation names provided in this property must exactly match the annotation names on the classes. If the target class has annotations specified with their fully qualified names (including package), the annotations in this property should also be specified with their fully qualified names. Similarly, if the target class has annotations specified with their simple names, this property should contain the annotations with the same simple names.

Show some of these issues in your example.

I assume Deprecated by itself won't work. Show that and put a comment to that effect.

done, please look at #14701 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I removed my comment here and added some more below.

</module>
</module>
</module>
*/

package com.puppycrawl.tools.checkstyle.checks.design.hideutilityclassconstructor;

// xdoc section -- start
@java.lang.Deprecated
class Example2 { // OK, skipped by annotation

public Example2() {
}

public static void fun() {
}
}

class Foo2 {

private Foo2() {
}

static int n;
}

class Bar2 {

protected Bar2() {
// prevents calls from subclass
throw new UnsupportedOperationException();
}
}

@SpringBootApplication
class ApplicationClass2 { // OK, skipped by annotation

static float f;
}
// xdoc section -- end
romani marked this conversation as resolved.
Show resolved Hide resolved
@interface SpringBootApplication {}
@@ -0,0 +1,46 @@
/*xml
<module name="Checker">
<module name="TreeWalker">
<module name="HideUtilityClassConstructor">
<property name="ignoreAnnotatedBy"
value="SpringBootApplication, Deprecated" />
</module>
</module>
</module>
*/

package com.puppycrawl.tools.checkstyle.checks.design.hideutilityclassconstructor;
Copy link
Member

Choose a reason for hiding this comment

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

I would much rather this be combined with example 2. It makes more sense to see side by side, exact match suppresses, non-exact match is a violation.


// xdoc section -- start
@java.lang.Deprecated // violation
Copy link
Member

Choose a reason for hiding this comment

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

add some verbiage that this isn't an exact match because of the package name.

class Example3 {

public Example3() {
}

public static void fun() {
}
}

class Foo3 {

private Foo3() {
}

static int n;
}

class Bar3 {

protected Bar3() {
// prevents calls from subclass
throw new UnsupportedOperationException();
}
}

@SpringBootApplication
class ApplicationClass3 { // OK, skipped by annotation
Copy link
Member

Choose a reason for hiding this comment

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

Same when this becomes a violation.


static float f;
}
// xdoc section -- end