migrate SuppressWarningsHolder to use property macro

[romani]

there is problem to migrate SuppressWarningsHolder to property macro for aliasList for public void setAliasList(String... aliasList) {

migration:

diff --git a/config/checkstyle-non-main-files-suppressions.xml b/config/checkstyle-non-main-files-suppressions.xml
index c3743fb..c5e0cd8 100644
--- a/config/checkstyle-non-main-files-suppressions.xml
+++ b/config/checkstyle-non-main-files-suppressions.xml
@@ -220,8 +220,6 @@
             files="src[\\/]xdocs[\\/]checks[\\/]annotation[\\/]packageannotation.xml.template"/>
   <!-- Properties macro suppressions until https://github.com/checkstyle/checkstyle/issues/13693 -->
   <suppress id="propertiesMacroMustExist"
-         files="src[\\/]xdocs[\\/]checks[\\/]annotation[\\/]suppresswarningsholder.xml.template"/>
-  <suppress id="propertiesMacroMustExist"
          files="src[\\/]xdocs[\\/]checks[\\/]blocks[\\/]avoidnestedblocks.xml.template"/>
   <suppress id="propertiesMacroMustExist"
          files="src[\\/]xdocs[\\/]checks[\\/]blocks[\\/]emptyblock.xml.template"/>
diff --git a/src/xdocs/checks/annotation/suppresswarningsholder.xml.template b/src/xdocs/checks/annotation/suppresswarningsholder.xml.template
index 8abced6..532bafa 100644
--- a/src/xdocs/checks/annotation/suppresswarningsholder.xml.template
+++ b/src/xdocs/checks/annotation/suppresswarningsholder.xml.template
@@ -26,29 +26,10 @@
 
       <subsection name="Properties" id="Properties">
         <div class="wrapper">
-          <table>
-            <tr>
-              <th>name</th>
-              <th>description</th>
-              <th>type</th>
-              <th>default value</th>
-              <th>since</th>
-            </tr>
-            <tr>
-              <td>aliasList</td>
-              <td>
-                Specify aliases for check names that can be used in code within
-                <code>SuppressWarnings</code>.
-              </td>
-              <td>
-                <a href="../../property_types.html#String.5B.5D">String[]</a>
-                in a format of comma separated attribute=value entries.
-                The attribute is the fully qualified name of the Check and value is its alias.
-              </td>
-              <td><code>null</code></td>
-              <td>5.7</td>
-            </tr>
-          </table>
+          <macro name="properties">
+            <param name="modulePath"
+                   value="src/main/java/com/puppycrawl/tools/checkstyle/checks/SuppressWarningsHolder.java"/>
+          </macro>
         </div>
       </subsection>
 

failure:

[ERROR] com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest -- Time elapsed: 0.674 s <<< ERROR!
org.apache.maven.doxia.parser.ParseException: Macro execution failed: Could not find field aliasList in class SuppressWarningsHolder
	at org.apache.maven.doxia.parser.AbstractXmlParser.parse(AbstractXmlParser.java:165)
	at org.apache.maven.doxia.parser.XhtmlBaseParser.parse(XhtmlBaseParser.java:92)
	at org.apache.maven.doxia.module.xdoc.XdocParser.parse(XdocParser.java:113)
	at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.parse(XdocsTemplateParser.java:76)
	at org.apache.maven.doxia.parser.AbstractParser.parse(AbstractParser.java:207)
	at com.puppycrawl.tools.checkstyle.internal.utils.XdocGenerator.generateXdocContent(XdocGenerator.java:66)
	at com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest.generateXdocContent(XdocsPagesTest.java:238)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptLifecycleMethod(TimeoutExtension.java:128)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptBeforeAllMethod(TimeoutExtension.java:70)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllMethods$13(ClassBasedTestDescriptor.java:411)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeBeforeAllMethods(ClassBasedTestDescriptor.java:409)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:215)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:84)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:148)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
	at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
	at org.apache.maven.surefire.junitplatform.LazyLauncher.execute(LazyLauncher.java:56)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.execute(JUnitPlatformProvider.java:184)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:148)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:122)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: org.apache.maven.doxia.macro.MacroExecutionException: Could not find field aliasList in class SuppressWarningsHolder
	at com.puppycrawl.tools.checkstyle.site.SiteUtil.getFieldClass(SiteUtil.java:1005)
	at com.puppycrawl.tools.checkstyle.site.SiteUtil.getType(SiteUtil.java:763)
	at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertyTypeCell(PropertiesMacro.java:276)
	at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertyRow(PropertiesMacro.java:190)
	at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writeTablePropertiesRows(PropertiesMacro.java:164)
	at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertiesTable(PropertiesMacro.java:109)
	at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.execute(PropertiesMacro.java:81)
	at org.apache.maven.doxia.parser.AbstractParser.executeMacro(AbstractParser.java:145)
	at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.processMacroEnd(XdocsTemplateParser.java:187)
	at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.handleEndTag(XdocsTemplateParser.java:116)
	at org.apache.maven.doxia.parser.AbstractXmlParser.parseXml(AbstractXmlParser.java:238)
	at org.apache.maven.doxia.parser.AbstractXmlParser.parse(AbstractXmlParser.java:156)
	... 67 more

[romani]

@stoyanK7 , can you help us to fix this issue?

[rnveach]

Property Setter:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/SuppressWarningsHolder.java

Line 157 in aab6311

public void setAliasList(String... aliasList) {

No field.

XDocs Overrides:

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java

Lines 147 to 148 in aab6311

	// static field (all upper case)
	"SuppressWarningsHolder.aliasList",

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java

Lines 1019 to 1021 in aab6311

	if ("SuppressWarningsHolder".equals(sectionName) && "aliasList".equals(propertyName)) {
	expectedTypeName = "String[] in a format of comma separated attribute=value entries. "
	+ "The attribute is the fully qualified name of the Check and value is its alias.";

[kalpadiptyaroy]

@romani
Hi, I am interested in taking up this issue. But prior to making code changes I would like to know more about this property macro. Can I get some relevant documentation suggestions for understanding macro?

[rnveach]

Google doxia and macro to look at its documentation. It is not something we made and I don't know the location for it. The PropertiesMacro is ours, and you can find it in our repository under that name.

[relentless-pursuit]

I was looking into the PropertiesMacro file. And, can this be a way to proceed? The aim could be to extend the functionality to handle properties that do not have a corresponding Field, the ones who could be accessible via setter methods like aliasList, without changing the existing functionality (backwards compatibility) i.e. having a provision for field or setter-method level retrival. Or, the solution is simple than i think it to be?

[romani]

@relentless-pursuit , please check comments on similar issue #13867 (comment)

There is some limitations and hardcoded mapping parent classes. Solution somewhere there.

[rnveach]

This check has no parent classes. Property is in the same class.

[romani]

In this case we need to extend logic to check for setters if no field is found

[kalpadiptyaroy]

In this case we need to extend logic to check for setters in no field is found

@romani
Are you indicating an override of doxia's parse function and hack it to extend the logic?

[romani]

Yes, but is it is not doxia parse method, it is general reflection method to find field/setter.
Doxia is used to just generate xdoc files.

[kalpadiptyaroy]

private static void registerAlias(String sourceName, String checkAlias) {
        CHECK_ALIAS_MAP.put(sourceName, checkAlias);
    }

public void setAliasList(String... aliasList) {
        for (String sourceAlias : aliasList) {
            final int index = sourceAlias.indexOf('=');
            if (index > 0) {
                registerAlias(sourceAlias.substring(0, index), sourceAlias
                    .substring(index + 1));
            }
            else if (!sourceAlias.isEmpty()) {
                throw new IllegalArgumentException(
                    "'=' expected in alias list item: " + sourceAlias);
            }
        }
    }

I have studied this function. I think the solution to fix this error is by re-naming the function.

Why should we re-name?

Its pretty logical that there is no real aliasList field in this class. Whereas, the setAliasList function is actually spliting a series of Strings and putting it in a hashmap. So, if we re-name it something as registerAliasFromAliasList OR putAliasList. Then it should make more sense and also resolve the error.

@romani - let me know if you are ok with this idea?

[rnveach]

Renaming a property setter to anything else will break the check as well as everyone's configurations.

See https://checkstyle.org/writingchecks.html#Defining_Check_Properties
Check properties: https://checkstyle.org/checks/annotation/suppresswarningsholder.html#Properties

[kalpadiptyaroy]

Renaming a property setter to anything else will break the check as well as everyone's configurations.

Understood.

Now clarify one thing - What should be returned for the non-existent field aliasList? when it is passed to:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java

Lines 278 to 288 in a6a339f

	public static Object getFieldValue(Field field, Object instance)
	throws MacroExecutionException {
	try {
	// required for package/private classes
	field.trySetAccessible();
	return field.get(instance);
	}
	catch (IllegalAccessException ex) {
	throw new MacroExecutionException("Couldn't get field value", ex);
	}
	}

Since aliasList does not exists so naturally the field variable would be null. So, what should we return ideally for such fields?

[rnveach]

IMO, most likely there shouldn't be a change in that method as it should be kept simple and to one point. I would like at XdocsPagesTest, as it does the validation correctly, and see what is missing from the macro.

[kalpadiptyaroy]

See the non-existent fields are taken care of in this class (SiteUtil) only for getFieldClass method.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java

Lines 1001 to 1049 in a6a339f

	private static Class<?> getFieldClass(Field field, String propertyName,
	String moduleName, Object instance)
	throws MacroExecutionException {
	Class<?> result = null;
	if (field != null) {
	result = field.getType();
	}
	if (result == null) {
	if (!PROPERTIES_ALLOWED_GET_TYPES_FROM_METHOD
	.contains(moduleName + DOT + propertyName)) {
	throw new MacroExecutionException(
	"Could not find field " + propertyName + " in class " + moduleName);
	}
	try {
	final PropertyDescriptor descriptor = PropertyUtils.getPropertyDescriptor(instance,
	propertyName);
	result = descriptor.getPropertyType();
	}
	catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException exc) {
	throw new MacroExecutionException(exc.getMessage(), exc);
	}
	}
	if (field != null && (result == List.class || result == Set.class)) {
	final ParameterizedType type = (ParameterizedType) field.getGenericType();
	final Class<?> parameterClass = (Class<?>) type.getActualTypeArguments()[0];
	if (parameterClass == Integer.class) {
	result = int[].class;
	}
	else if (parameterClass == String.class) {
	result = String[].class;
	}
	else if (parameterClass == Pattern.class) {
	result = Pattern[].class;
	}
	else {
	final String message = "Unknown parameterized type: "
	+ parameterClass.getSimpleName();
	throw new MacroExecutionException(message);
	}
	}
	else if (result == BitSet.class) {
	result = int[].class;
	}
	return result;
	}

That's why I was asking how to tackle that for getFieldValue method?

[relentless-pursuit]

What value be expected if it isn't a field rather a setter?
final Object value = getFieldValue(field, classInstance);

Inside, getFieldValue, field.get(instance); I am struggling to do that for public static String getAlias(String sourceName) or corresponding setter. Also, the getter doesn't follow a standard structure and takes an argument.

[rnveach]

I am struggling to do that for public static String getAlias(String sourceName)

Getters have no connection to properties and how Checkstyle works with them. They can technically be whatever they want, even be named wrongly, though I am not sure that is the case with your example.

Truthfully, our property process only looks at setters. Fields are just storage containers, and don't need to be there, or even named after the setter. We continue that since it is what makes sense for Java and bean classes.

What value be expected .. final Object value = getFieldValue

I don't pretend to know the macro's logic at a detailed level, which is why I can't provide any guidance as you are expecting exact instructions. I recommend instead to examine the full code and find what logically makes sense. Not everything is going to be cut and dry. If you are unable to identify what to fix, then I would probably recommend an easier issue.

getFieldValue to me looks like a simple reflection util to get the value of a field. It doesn't make sense to me to add logic to it that is for examining a setter, or determining if we should be looking at the field or the setter. There should probably be some logic above that which should be changed to say if the field doesn't exist, to examine the setter instead.

As I mentioned before, the macro should be compared to XdocsPagesTest. It was written first, and has the correct logic on identifying properties and their types. XdocsPagesTest was what should have been gutted and copied to the macro, but something is missing obviously.

PROPERTIES_ALLOWED_GET_TYPES_FROM_METHOD
Properties that can not be gathered from class instance.

Maybe look into this field and why it can't be used or isn't working correctly.

[relentless-pursuit]

I am struggling to do that for public static String getAlias(String sourceName)

Getters have no connection to properties and how Checkstyle works with them. They can technically be whatever they want, even be named wrongly, though I am not sure that is the case with your example.

What value be expected .. final Object value = getFieldValue

I don't pretend to know the macro's logic at a detailed level, which is why I can't provide any guidance as you are expecting exact instructions. I recommend instead to examine the full code and find what logically makes sense. Not everything is going to be cut and dry. If you are unable to identify what to fix, then I would probably recommend an easier issue.

getFieldValue to me looks like a simple reflection util to get the value of a field. It doesn't make sense to me to add logic to it that is for examining a setter, or determining if we should be looking at the field or the setter. There should probably be some logic above that which should be changed to say if the field doesn't exist, to examine the setter instead.

As I mentioned before, the macro should be compared to XdocsPagesTest. It was written first, and has the correct logic on identifying properties and their types. XdocsPagesTest was what should have been gutted and copied to the macro, but something is missing obviously.

PROPERTIES_ALLOWED_GET_TYPES_FROM_METHOD
Properties that can not be gathered from class instance.

Maybe look into this field and why it can't be used or isn't working correctly.

It is not being used because it has SuppressWarningsHolderCheck.aliasList. As per my understanding, it has to be SuppressWarningsHolder.aliasList. On doing that, i can access the writeMethod i.e. setter from propertyDescriptor. And, about getFieldValue(...), the exception is being thrown from there because field is null in this case. As you mentioned maybe some null check accompanied by redirecting to setter. However, what i don't know is how do i have a codebase getting similar results so far for a setter like it did for field. Currently, looking for it in my free time. But, if anybody else submits a PR, i can learn from them too. And, thanks for the response. Will go deeper into some of the insights offered.

[kalpadiptyaroy]

@relentless-pursuit Exactly this is my observation! On correcting the class name to SuppressWarningsHolderCheck the field variable is throwing Null Pointer Exception!

So we need to handle this NullPointerException...!

Raising a PR on this soon.

[relentless-pursuit]

I have raised a PR on this. Some test cases are failing, which can be corrected as we proceed with review. It would be great if i can have a review on the core logic where i used CHECK_ALIAS_MAP stores aliasList values.