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

config: Remove redundant and incorrect rules from import control configuration #3736

Closed
jochenvdv opened this Issue Jan 18, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@jochenvdv
Contributor

jochenvdv commented Jan 18, 2017

The import control configuration has become a bit out of sync and has to be updated in several places. I will list the issues here as I go through the configuration and find more.

Please comment on how to handle these individual issues.

  • java.lang.reflect is redundantly allowed: it is listed non-locally in the top package but also under the checks.indentation package. Should java.lang.reflect be allowed anywhere? I think not, so I suggest we make it local-only in the top package (it is used in PackageObjectFactory) and add other local-only allowance only where needed.

  • java.nio.charset.Charset is redundantly allowed in checks.header: java.niois already globally allowed in the top package. I assume we want to allow java.nio everywhere and can simply remove the redundant allow?

  • Thanks to #3724 we can remove allowances for imports only used in test code. This will be done in #3724 for Guava, but other unnecessary rules might also be left over. I will find them all and remove them if possible.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 21, 2017

Member

java.lang.reflect is redundantly allowed:

approved. no violations with

$ git diff
diff --git a/config/import-control.xml b/config/import-control.xml
index 2289984..60506bf 100644
--- a/config/import-control.xml
+++ b/config/import-control.xml
@@ -18,7 +18,7 @@
   <allow pkg="org.apache.commons.logging"/>
   <allow pkg="org.xml.sax"/>
   <allow pkg="com.puppycrawl.tools.checkstyle"/>
-  <allow pkg="java.lang.reflect"/>
+  <allow pkg="java.lang.reflect" local-only="true"/>
 
   <!-- The local ones -->
   <allow class="java.security.MessageDigest" local-only="true"/>
@@ -39,6 +39,7 @@
   <subpackage name="utils">
     <allow class="com.google.common.base.CharMatcher" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.lang.reflect" local-only="true"/>
   </subpackage>
 
   <subpackage name="ant">
@@ -63,6 +64,7 @@
     <allow class="com.google.common.io.Closeables" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableCollection" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.lang.reflect" local-only="true"/>
   </subpackage>
 
   <subpackage name="checks">

java.nio.charset.Charset is redundantly allowed

global level should be lowered, not all need this imports, fix is

$ git diff
diff --git a/config/import-control.xml b/config/import-control.xml
index 2289984..6fff2b4 100644
--- a/config/import-control.xml
+++ b/config/import-control.xml
@@ -11,7 +11,7 @@
   <allow pkg="com.puppycrawl.tools.checkstyle.checks"/>
   <allow pkg="java.io"/>
   <allow pkg="java.net"/>
-  <allow pkg="java.nio"/>
+  <allow pkg="java.nio" local-only="true"/>
   <allow pkg="java.util"/>
   <allow pkg="javax.xml.parsers"/>
   <allow pkg="org.apache.commons.beanutils"/>
@@ -39,6 +39,7 @@
   <subpackage name="utils">
     <allow class="com.google.common.base.CharMatcher" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.nio" local-only="true"/>
   </subpackage>
 
   <subpackage name="ant">
@@ -63,6 +64,7 @@
     <allow class="com.google.common.io.Closeables" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableCollection" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.nio" local-only="true"/>
   </subpackage>
 
   <subpackage name="checks">
@@ -75,6 +77,7 @@
     <allow class="com.google.common.collect.Multiset" local-only="true"/>
     <allow class="com.google.common.collect.Multiset.Entry" local-only="true"/>
     <allow class="com.google.common.collect.SetMultimap" local-only="true"/>
+    <allow pkg="java.nio" local-only="true"/>
     <allow pkg="java.math"/>
 
     <subpackage name="indentation">
@@ -94,6 +97,7 @@
   <subpackage name="doclets">
     <allow pkg="com.sun.javadoc"/>
     <disallow pkg="com\.puppycrawl\.tools\.checkstyle\.(checks|ant|filters|gui)" regex="true"/>
+    <allow pkg="java.nio" local-only="true"/>
   </subpackage>
 
   <subpackage name="filters">
Member

romani commented Jan 21, 2017

java.lang.reflect is redundantly allowed:

approved. no violations with

$ git diff
diff --git a/config/import-control.xml b/config/import-control.xml
index 2289984..60506bf 100644
--- a/config/import-control.xml
+++ b/config/import-control.xml
@@ -18,7 +18,7 @@
   <allow pkg="org.apache.commons.logging"/>
   <allow pkg="org.xml.sax"/>
   <allow pkg="com.puppycrawl.tools.checkstyle"/>
-  <allow pkg="java.lang.reflect"/>
+  <allow pkg="java.lang.reflect" local-only="true"/>
 
   <!-- The local ones -->
   <allow class="java.security.MessageDigest" local-only="true"/>
@@ -39,6 +39,7 @@
   <subpackage name="utils">
     <allow class="com.google.common.base.CharMatcher" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.lang.reflect" local-only="true"/>
   </subpackage>
 
   <subpackage name="ant">
@@ -63,6 +64,7 @@
     <allow class="com.google.common.io.Closeables" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableCollection" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.lang.reflect" local-only="true"/>
   </subpackage>
 
   <subpackage name="checks">

java.nio.charset.Charset is redundantly allowed

global level should be lowered, not all need this imports, fix is

$ git diff
diff --git a/config/import-control.xml b/config/import-control.xml
index 2289984..6fff2b4 100644
--- a/config/import-control.xml
+++ b/config/import-control.xml
@@ -11,7 +11,7 @@
   <allow pkg="com.puppycrawl.tools.checkstyle.checks"/>
   <allow pkg="java.io"/>
   <allow pkg="java.net"/>
-  <allow pkg="java.nio"/>
+  <allow pkg="java.nio" local-only="true"/>
   <allow pkg="java.util"/>
   <allow pkg="javax.xml.parsers"/>
   <allow pkg="org.apache.commons.beanutils"/>
@@ -39,6 +39,7 @@
   <subpackage name="utils">
     <allow class="com.google.common.base.CharMatcher" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.nio" local-only="true"/>
   </subpackage>
 
   <subpackage name="ant">
@@ -63,6 +64,7 @@
     <allow class="com.google.common.io.Closeables" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableCollection" local-only="true"/>
     <allow class="com.google.common.collect.ImmutableMap" local-only="true"/>
+    <allow pkg="java.nio" local-only="true"/>
   </subpackage>
 
   <subpackage name="checks">
@@ -75,6 +77,7 @@
     <allow class="com.google.common.collect.Multiset" local-only="true"/>
     <allow class="com.google.common.collect.Multiset.Entry" local-only="true"/>
     <allow class="com.google.common.collect.SetMultimap" local-only="true"/>
+    <allow pkg="java.nio" local-only="true"/>
     <allow pkg="java.math"/>
 
     <subpackage name="indentation">
@@ -94,6 +97,7 @@
   <subpackage name="doclets">
     <allow pkg="com.sun.javadoc"/>
     <disallow pkg="com\.puppycrawl\.tools\.checkstyle\.(checks|ant|filters|gui)" regex="true"/>
+    <allow pkg="java.nio" local-only="true"/>
   </subpackage>
 
   <subpackage name="filters">
@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Jan 23, 2017

Contributor

@romani

We need to make some rules about the import control file and clean it up.
Sometimes packages are allowed, sometimes specific classes.
Sometimes local, sometimes not. We need to be specific and consistent.

I am willing to go over all the rules and make them consistent, but I am not completely familiar with how they should be done.
I propose that I make a PR with some fixes listed in this issue, and that you review it and point out other things that should be changed (too prevent too much questions back and forth from me). I can also review my own PR and comment on lines that I think could be changed.

Contributor

jochenvdv commented Jan 23, 2017

@romani

We need to make some rules about the import control file and clean it up.
Sometimes packages are allowed, sometimes specific classes.
Sometimes local, sometimes not. We need to be specific and consistent.

I am willing to go over all the rules and make them consistent, but I am not completely familiar with how they should be done.
I propose that I make a PR with some fixes listed in this issue, and that you review it and point out other things that should be changed (too prevent too much questions back and forth from me). I can also review my own PR and comment on lines that I think could be changed.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 23, 2017

Member

@jochenvdv ,

I would love to clean up this, you can start work by any means, I will share my thoughts and guide you.
If supposed update is big, it is might be better to discuss it before processing to PR.

One more issue that will help is ability to enforce import control for certain files not whole package , we already have issue on this somewhere. It will help to make a fence around Classes that are unusual and allow certain imports only in them. Example: allowance of reflection in our code, it is allowed only in certain classes. But not enforced :( .

Member

romani commented Jan 23, 2017

@jochenvdv ,

I would love to clean up this, you can start work by any means, I will share my thoughts and guide you.
If supposed update is big, it is might be better to discuss it before processing to PR.

One more issue that will help is ability to enforce import control for certain files not whole package , we already have issue on this somewhere. It will help to make a fence around Classes that are unusual and allow certain imports only in them. Example: allowance of reflection in our code, it is allowed only in certain classes. But not enforced :( .

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Jan 23, 2017

Contributor

@romani I think you mean issue #3492? I agree, I will wait with the cleanup of the config until that issue is implemented, else we will have to redo it too many times.

Contributor

jochenvdv commented Jan 23, 2017

@romani I think you mean issue #3492? I agree, I will wait with the cleanup of the config until that issue is implemented, else we will have to redo it too many times.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 23, 2017

Member

with issue #3492 we could finally enforce very strict model of import usages.
Some cleanup could be done independently. This clan up will generate more use cases for #3492 .
This issue could be done even now.

Member

romani commented Jan 23, 2017

with issue #3492 we could finally enforce very strict model of import usages.
Some cleanup could be done independently. This clan up will generate more use cases for #3492 .
This issue could be done even now.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Jan 25, 2017

Contributor

Ok, I will list the issues here before I make a PR. Will take a while though since I am busy.

Contributor

jochenvdv commented Jan 25, 2017

Ok, I will list the issues here before I make a PR. Will take a while though since I am busy.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Feb 3, 2017

Contributor

Some more issues, most of them are rules that I think can be made more restrictive:

  • line 81: checks package allowed in checks package 😵, let's remove this useless rule
  • line 61:java.text allowed in api package, maybe this should be limited to the MessageFormat class (only one used) and made local-only?
  • line 60: java.beans is only used in AutomaticBean (only java.beans.PropertyDescriptor), I suggest to only allow this class locally
  • line 9: org.antlr.v4.runtime is only required in checkstyle and api packages (in one class each), maybe this allowance can be made more specific instead of global?
  • line 82: Definitions only used once locally, can be made local-only
  • line 80: java.math only used in checks.metrics but allowed in all checks, let's limit it to metrics?
  • line 117: only WeakReference is used, we can limit it to this class only
  • line 70-71: Utils class no longer exists?

Biggest issue:

line 20: this rule makes many other allowances of checkstyle subpackages unnecessary (e.g line 62 - 73 is redundant) since it simply allows everything from checkstyle. We should probably change this to only allow utils globally and add specific rules for the other imports that are required (most are checkstyle root package, some gui)

For example, with line 20 changed to only allow com.puppycrawl.tools.checkstyle.utils only the following violations are left:

[echo] Checkstyle started: 03/02/2017 08:20:08 PM
[checkstyle] Running Checkstyle 7.6-SNAPSHOT on 929 files
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java:25:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseErrorMessage. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java:26:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseStatus. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:47:1: Disallowed import - com.puppycrawl.tools.checkstyle.Checker. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:48:1: Disallowed import - com.puppycrawl.tools.checkstyle.ConfigurationLoader. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:49:1: Disallowed import - com.puppycrawl.tools.checkstyle.DefaultLogger. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:50:1: Disallowed import - com.puppycrawl.tools.checkstyle.ModuleFactory. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:51:1: Disallowed import - com.puppycrawl.tools.checkstyle.PackageObjectFactory. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:52:1: Disallowed import - com.puppycrawl.tools.checkstyle.PropertiesExpander. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:53:1: Disallowed import - com.puppycrawl.tools.checkstyle.XMLLogger. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/api/ExternalResourceHolder.java:24:1: Disallowed import - com.puppycrawl.tools.checkstyle.Checker. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java:29:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java:30:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseErrorMessage. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java:31:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseStatus. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/MainFrame.java:44:1: Disallowed import - com.puppycrawl.tools.checkstyle.gui.MainFrameModel.ParseMode. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/MainFrameModel.java:30:1: Disallowed import - com.puppycrawl.tools.checkstyle.TreeWalker. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/ParseTreeTableModel.java:29:1: Disallowed import - com.puppycrawl.tools.checkstyle.gui.MainFrameModel.ParseMode. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/ParseTreeTablePModel.java:27:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/ParseTreeTablePModel.java:31:1: Disallowed import - com.puppycrawl.tools.checkstyle.gui.MainFrameModel.ParseMode. [ImportControl]
     [echo] Checkstyle finished: 03/02/2017 08:20:33 PM

It seems that there is a problem with nested classes, they raise a violation even if they come from the same package. Is this a bug?

Contributor

jochenvdv commented Feb 3, 2017

Some more issues, most of them are rules that I think can be made more restrictive:

  • line 81: checks package allowed in checks package 😵, let's remove this useless rule
  • line 61:java.text allowed in api package, maybe this should be limited to the MessageFormat class (only one used) and made local-only?
  • line 60: java.beans is only used in AutomaticBean (only java.beans.PropertyDescriptor), I suggest to only allow this class locally
  • line 9: org.antlr.v4.runtime is only required in checkstyle and api packages (in one class each), maybe this allowance can be made more specific instead of global?
  • line 82: Definitions only used once locally, can be made local-only
  • line 80: java.math only used in checks.metrics but allowed in all checks, let's limit it to metrics?
  • line 117: only WeakReference is used, we can limit it to this class only
  • line 70-71: Utils class no longer exists?

Biggest issue:

line 20: this rule makes many other allowances of checkstyle subpackages unnecessary (e.g line 62 - 73 is redundant) since it simply allows everything from checkstyle. We should probably change this to only allow utils globally and add specific rules for the other imports that are required (most are checkstyle root package, some gui)

For example, with line 20 changed to only allow com.puppycrawl.tools.checkstyle.utils only the following violations are left:

[echo] Checkstyle started: 03/02/2017 08:20:08 PM
[checkstyle] Running Checkstyle 7.6-SNAPSHOT on 929 files
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java:25:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseErrorMessage. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java:26:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseStatus. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:47:1: Disallowed import - com.puppycrawl.tools.checkstyle.Checker. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:48:1: Disallowed import - com.puppycrawl.tools.checkstyle.ConfigurationLoader. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:49:1: Disallowed import - com.puppycrawl.tools.checkstyle.DefaultLogger. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:50:1: Disallowed import - com.puppycrawl.tools.checkstyle.ModuleFactory. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:51:1: Disallowed import - com.puppycrawl.tools.checkstyle.PackageObjectFactory. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:52:1: Disallowed import - com.puppycrawl.tools.checkstyle.PropertiesExpander. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java:53:1: Disallowed import - com.puppycrawl.tools.checkstyle.XMLLogger. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/api/ExternalResourceHolder.java:24:1: Disallowed import - com.puppycrawl.tools.checkstyle.Checker. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java:29:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java:30:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseErrorMessage. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java:31:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser.ParseStatus. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/MainFrame.java:44:1: Disallowed import - com.puppycrawl.tools.checkstyle.gui.MainFrameModel.ParseMode. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/MainFrameModel.java:30:1: Disallowed import - com.puppycrawl.tools.checkstyle.TreeWalker. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/ParseTreeTableModel.java:29:1: Disallowed import - com.puppycrawl.tools.checkstyle.gui.MainFrameModel.ParseMode. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/ParseTreeTablePModel.java:27:1: Disallowed import - com.puppycrawl.tools.checkstyle.JavadocDetailNodeParser. [ImportControl]
[checkstyle] [ERROR] /home/jochenvdv/src/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/gui/ParseTreeTablePModel.java:31:1: Disallowed import - com.puppycrawl.tools.checkstyle.gui.MainFrameModel.ParseMode. [ImportControl]
     [echo] Checkstyle finished: 03/02/2017 08:20:33 PM

It seems that there is a problem with nested classes, they raise a violation even if they come from the same package. Is this a bug?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 8, 2017

Member

@jochenvdv , sorry for delay.

line 81: checks package allowed in checks package 😵, let's remove this useless rule

:) , ok to remove.

line 61:java.text allowed in api package, maybe this should be limited to the MessageFormat class (only one used) and made local-only?

in api in mandatory to be as strict as possible.

line 60: java.beans is only used in AutomaticBean (only java.beans.PropertyDescriptor), I suggest to only allow this class locally

yes.

line 9: org.antlr.v4.runtime is only required in checkstyle and api packages (in one class each), maybe this allowance can be made more specific instead of global?

has to be limited as much as possible. should not be present in API, but we will address this in checkstyle8.

line 82: Definitions only used once locally, can be made local-only

please do

line 80: java.math only used in checks.metrics but allowed in all checks, let's limit it to metrics?

lets keep it, math package has some good utils that might be useful. I do not see design harm from usage of this package. Do you see problems ?

line 117: only WeakReference is used, we can limit it to this class only

yes

line 70-71: Utils class no longer exists?

yes, please remove. I removed Utils from API a while ago, this is old stuff.

It seems that there is a problem with nested classes, they raise a violation even if they come from the same package. Is this a bug?

yes, please report it.

Member

romani commented Feb 8, 2017

@jochenvdv , sorry for delay.

line 81: checks package allowed in checks package 😵, let's remove this useless rule

:) , ok to remove.

line 61:java.text allowed in api package, maybe this should be limited to the MessageFormat class (only one used) and made local-only?

in api in mandatory to be as strict as possible.

line 60: java.beans is only used in AutomaticBean (only java.beans.PropertyDescriptor), I suggest to only allow this class locally

yes.

line 9: org.antlr.v4.runtime is only required in checkstyle and api packages (in one class each), maybe this allowance can be made more specific instead of global?

has to be limited as much as possible. should not be present in API, but we will address this in checkstyle8.

line 82: Definitions only used once locally, can be made local-only

please do

line 80: java.math only used in checks.metrics but allowed in all checks, let's limit it to metrics?

lets keep it, math package has some good utils that might be useful. I do not see design harm from usage of this package. Do you see problems ?

line 117: only WeakReference is used, we can limit it to this class only

yes

line 70-71: Utils class no longer exists?

yes, please remove. I removed Utils from API a while ago, this is old stuff.

It seems that there is a problem with nested classes, they raise a violation even if they come from the same package. Is this a bug?

yes, please report it.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Feb 9, 2017

Contributor

Please comment on this, I think you forgot. It is a problem irrelevant of that bug.

line 20: this rule makes many other allowances of checkstyle subpackages unnecessary (e.g line 62 - 73 is redundant) since it simply allows everything from checkstyle. We should probably change this to only allow utils globally and add specific rules for the other imports that are required (most are checkstyle root package, some gui)

Thanks for your input, I will fix these issues this weekend, as well as investigate/report that bug.

Contributor

jochenvdv commented Feb 9, 2017

Please comment on this, I think you forgot. It is a problem irrelevant of that bug.

line 20: this rule makes many other allowances of checkstyle subpackages unnecessary (e.g line 62 - 73 is redundant) since it simply allows everything from checkstyle. We should probably change this to only allow utils globally and add specific rules for the other imports that are required (most are checkstyle root package, some gui)

Thanks for your input, I will fix these issues this weekend, as well as investigate/report that bug.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 9, 2017

Member

we need to remove that global allowance. Even utils package usage in API should not be allowed but this should be separate issue with "checkstyle8" label.
Lets just remove it and put allowances if required to child packages.
Unfortunately global allow and specific disallows is hard to maintain, in our small projects lets avoid this.

Member

romani commented Feb 9, 2017

we need to remove that global allowance. Even utils package usage in API should not be allowed but this should be separate issue with "checkstyle8" label.
Lets just remove it and put allowances if required to child packages.
Unfortunately global allow and specific disallows is hard to maintain, in our small projects lets avoid this.

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Feb 10, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Feb 10, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Feb 10, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Feb 11, 2017

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Feb 11, 2017

Contributor

@romani I investigated the possible bug but it is expected behaviour, not a bug.

When inner classes are imported they must be explicitly allowed (possibly with regex), else only the top-level class is matched.

Contributor

jochenvdv commented Feb 11, 2017

@romani I investigated the possible bug but it is expected behaviour, not a bug.

When inner classes are imported they must be explicitly allowed (possibly with regex), else only the top-level class is matched.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 11, 2017

Member

OK, we need to update documentation as it is not clear, even for us, we need to add some "Attention" note.

Member

romani commented Feb 11, 2017

OK, we need to update documentation as it is not clear, even for us, we need to add some "Attention" note.

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Feb 11, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Feb 12, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Feb 12, 2017

rnveach added a commit that referenced this issue Feb 13, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

Fix is merged

Member

rnveach commented Feb 13, 2017

Fix is merged

@rnveach rnveach closed this Feb 13, 2017

@rnveach rnveach added this to the 7.6 milestone Feb 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment