Issue #5899: Add InterfaceImpliedModifierCheck #5974
Conversation
|
CI failures are related to TC Violations (IDEA) and travis violations (Eclipse and spellchecker). |
| @@ -654,6 +654,8 @@ private static void fillChecksFromModifierPackage() { | |||
| BASE_PACKAGE + ".checks.modifier.ModifierOrderCheck"); | |||
| NAME_TO_FULL_MODULE_NAME.put("RedundantModifierCheck", | |||
| BASE_PACKAGE + ".checks.modifier.RedundantModifierCheck"); | |||
| NAME_TO_FULL_MODULE_NAME.put("InterfaceMemberImpliedModifierCheck", | |||
| BASE_PACKAGE + ".checks.modifier.InterfaceMemberImpliedModifierCheck"); | |||
There was a problem hiding this comment.
please order the checks alphabetically.
| import com.puppycrawl.tools.checkstyle.api.TokenTypes; | ||
|
|
||
| /** | ||
| * Checks modifiers on interface members, ensuring that certain modifiers are |
There was a problem hiding this comment.
Add your check name to the list and fix all violations the test produce. It will align your Javadoc based on what is supplied in the XDoc.
Let us know if you run into any issues or have questions.
| @@ -1,3 +1,4 @@ | |||
| annotation.order=El modificador de anotación ''{0}'' no precede a los modificadores normales. | |||
| mod.order=Modificador ''{0}'' desordenado según las sugerencias de la JLS. | |||
| redundantModifier=Modificador ''{0}'' redundante. | |||
| interface.implied.modifier=El modificador impl\u00edcito debe ser expl\u00edcito: ''{0}'' | |||
There was a problem hiding this comment.
I don't think we should have these \u in the property files. Is there a reason you can't enter the actual characters into the file? Do the same for the other property files.
There was a problem hiding this comment.
unicode symbols as is should be used.
@jodastephen , If you start to change your commit, please user Google translator to have non escaped symbols. In worst case we merge ugly translation, and other update it to better later on.
| // private methods on interfaces are Java 9 only, but checkstyle is Java 8 | ||
| // this test exists solely to provide coverage of private methods | ||
| final DetailAST modPrivate = new DetailAST(); | ||
| modPrivate.setType(TokenTypes.LITERAL_PRIVATE); |
There was a problem hiding this comment.
You should be able create a valid Java 9 file and place it in non-compilable folder and put a marker on it that it should be compiled by Java 9.
Example: https://github.com/checkstyle/checkstyle/blob/master/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammars/java9/InputJava9TryWithResources.java#L1
| @Override | ||
| public void visitToken(DetailAST ast) { | ||
| if (isInterfaceMember(ast)) { | ||
| if (ast.getType() == TokenTypes.METHOD_DEF) { |
| @@ -1,3 +1,4 @@ | |||
| annotation.order=Annotation-Modifier ''{0}'' sollte vor den anderen Modifiern stehen. | |||
| mod.order=Modifier ''{0}'' weicht von der empfohlenen Modifier-Reihenfolge der Java Language Specification ab. | |||
| redundantModifier=Überflüssiger Modifier ''{0}''. | |||
| interface.implied.modifier=Impliziter Modifikator sollte explizit sein: ''{0}'' | |||
There was a problem hiding this comment.
@Bananeweizen , can you recheck this German translation.
| @@ -1,3 +1,4 @@ | |||
| annotation.order=Le modificateur d''annotation ''{0}'' ne précède pas les autres modificateurs. | |||
| mod.order=Le mot-clef ''{0}'' n''apparaît pas dans l''ordre préconisé par les JLS. | |||
| redundantModifier=Mot-clef ''{0}'' redondant. | |||
| interface.implied.modifier=Le modificateur implicite devrait \u00eatre explicite: ''{0}'' | |||
There was a problem hiding this comment.
@Jajawah , is it good translation ?
| @@ -1,3 +1,4 @@ | |||
| annotation.order=''{0}'' 注釈修飾子は、非注釈修飾子に先行していません。 | |||
| mod.order=''{0}'' 修飾子が JLS 提案の順序に沿いません。 | |||
| redundantModifier=冗長な ''{0}'' 修飾子です。 | |||
| interface.implied.modifier=\u6697\u793A\u3055\u308C\u305F\u4FEE\u98FE\u5B50\u306F\u660E\u793A\u7684\u306B\u3059\u308B\u5FC5\u8981\u304C\u3042\u308A\u307E\u3059: ''{0}'' | |||
There was a problem hiding this comment.
@Nobuyuki-Inaba, please help us to make good Japanese translation.
There was a problem hiding this comment.
sure. I'd appreciate if you could give me a few days to check it.
I found that existing messages are not unicode-escaped but the added message is unicode-escaped. If not unicode-escaped message allowed to use, I think not unicode-escaped should be used because not unicode-escaped message makes maintenance easy.
There was a problem hiding this comment.
We need not unicode-escaped, just to be easy to read by others. Thanks a lot in advance.
There was a problem hiding this comment.
The original message is OK but the below message sounds more natural in Japanese.
暗黙的な ''{0}'' 修飾子は明示的に指定すべきです。
| @@ -1,3 +1,4 @@ | |||
| annotation.order=A anotação ''{0}'' não deveria preceder modificadores que não são anotações. | |||
| mod.order=O modificador ''{0}'' está fora da ordem sugerida pela JLS. | |||
| redundantModifier=O modificador ''{0}'' é redundante. | |||
| interface.implied.modifier=O modificador impl\u00edcito deve ser expl\u00edcito: ''{0}'' | |||
There was a problem hiding this comment.
@victorwss, please help us good translation to Portuguese
| @@ -5,3 +5,4 @@ annotation.order = ''{0}'' anotasyon niteleyicisi, anotasyon olmayan niteleyicil | |||
| mod.order = ''{0}'' niteleyicisi, Java tarafından önerilen sırada değil. | |||
|
|
|||
| redundantModifier = Gereksiz ''{0}'' niteleyicisi. | |||
| interface.implied.modifier=Z\u0131mni de\u011Fi\u015Ftirici aç\u0131k olmal\u0131d\u0131r: ''{0}'' | |||
There was a problem hiding this comment.
@precoder, it would be awesome is you can suggest Turkish translation.
There was a problem hiding this comment.
After reading the discussion link at the top of issue, I would suggest something like:
interface.implied.modifier=Varsayılan niteleyici: ''{0}'' açıkça belirtilmelidir.
| @@ -1,3 +1,4 @@ | |||
| annotation.order=注解 ''{0}'' 前不应有非注解修饰符。 | |||
| mod.order=''{0}'' 修饰符顺序违反 JLS 建议. | |||
| redundantModifier=多余修饰符： ''{0}''。 | |||
| interface.implied.modifier=\u9690\u542B\u7684\u4FEE\u9970\u7B26\u5E94\u8BE5\u662F\u660E\u786E\u7684: ''{0}'' | |||
There was a problem hiding this comment.
@Apache9, please help us with Chinese translation.
There was a problem hiding this comment.
Sure. Will take a look later today.
There was a problem hiding this comment.
@jodastephen , thanks a lot for implementation.
I really impressed that passed pitest on first try :).
Passing CI is good, but not enough, we need prove that there are no exceptions on real life ugly projects.
Please read README of https://github.com/checkstyle/contribution/tree/master/checkstyle-tester
Please launch launch.groovy on all projects that are referenced in property file(just uncomment all). There should be no exceptions. Unfortunately there will be too much violations to share in web, so please select 1-3 projects you like(openjdkX is good candidate) and run report generation on them only and share it on github.io for us to review.
If you have any concerns or questions do not hesitate to ask us.
items to improve:
|
|
||
| void method(); | ||
|
|
||
| // cannot test private methods due to Java 8 baseline |
There was a problem hiding this comment.
we have java9 section in resources - https://github.com/checkstyle/checkstyle/tree/master/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammars/java9
just put a special comment to let CI verify that it actually compilable in java9.
| @@ -1,3 +1,4 @@ | |||
| annotation.order=El modificador de anotación ''{0}'' no precede a los modificadores normales. | |||
| mod.order=Modificador ''{0}'' desordenado según las sugerencias de la JLS. | |||
| redundantModifier=Modificador ''{0}'' redundante. | |||
| interface.implied.modifier=El modificador impl\u00edcito debe ser expl\u00edcito: ''{0}'' | |||
There was a problem hiding this comment.
unicode symbols as is should be used.
@jodastephen , If you start to change your commit, please user Google translator to have non escaped symbols. In worst case we merge ugly translation, and other update it to better later on.
src/xdocs/config_modifier.xml
Outdated
| <p> | ||
| Methods on interfaces are <code>public</code> by default, however from Java 9 | ||
| they can also be <code>private</code>. This check provides the ability to enforce | ||
| that <code>public</code> is not implicitly added by the compiler. |
There was a problem hiding this comment.
I would rather void phrases that we we enforce smth on compiler.
May it be better smth "enforce public be explicitly defined by human and not implicitly added by the compiler"
and in other places.
| public interface AddressFactory { | ||
| // check enforces code contains "public static final" | ||
| public static final String UNKNOWN = "Unknown"; | ||
|
|
There was a problem hiding this comment.
please update examples to have trailing comment "// violation" - it help users a lot to understand how Check works and what to expect from it.
// check enforces code contains "public static final"
public static final String UNKNOWN = "Unknown";
static final String UNKNOWN2 = "Unknown"; // violation
src/xdocs/config_modifier.xml
Outdated
| // check enforces code contains "public abstract" | ||
| public abstract Address createAddress(String addressLine, String city); | ||
|
|
||
| // check enforces code contains "public default" or "private" |
There was a problem hiding this comment.
it is better to explain that we force "public" or "private".
"abstract" and "default" are enforced by compiler, not by checkstyle.
|
Some issues addressed while I work towards the rest (Java 9 proving a problem). Key one is that the messages are now not unicode escapes. |
It is better to reply "done" on each point we raised, to clearly track what is done and what is not. It will help us a lot. |
| @@ -1,3 +1,4 @@ | |||
| annotation.order=A anotação ''{0}'' não deveria preceder modificadores que não são anotações. | |||
| interface.implied.modifier=O modificador implícito deve ser explícito: ''{0}'' | |||
There was a problem hiding this comment.
I think that O modificador implícito deveria ser explícito: ''{0}'' is a slightly better translation.
|
@romani The best translation to Portugueses would be O modificador implícito deveria ser explícito: ''{0}'' - However, the given translation is OK. Here, "deve" means "must", while "deveria" means "should". BTW, I am not sure if \u-escaping those strings is a good idea. That makes non-ascii characters illegible and unrecognizable for humans. For far east languages, that makes the entire property files unintelligible and people would need to use some external tool to make any sense of them. Nowadays, all those properties are already encoded without escaping, so picking one or other and encoding them in that way is not the right approach. We should use an all or nothing approach with that. Digging further into this issue, unfortunately, the The proper solution would be to use the I searched through Checkstyle's code and there is a lot of places which uses |
Please move this to separate issue. |
There was a problem hiding this comment.
@jodastephen , I know this is terrible bureaucracy, but please reply "done" for each point. You did emotion markers. But I do not receive notification on them so I could miss your changes for some time.
Regression testing is still required.
items to improve:
| case TokenTypes.ENUM_DEF: | ||
| default: | ||
| processNestedType(ast); | ||
| break; |
There was a problem hiding this comment.
we more like exceptions on default (smth unknown), example:
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/UncommentedMainCheck.java#L102
https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/UncommentedMainCheckTest.java#L114
| } | ||
| return parentTypeDef != null | ||
| && parentTypeDef.getType() == TokenTypes.INTERFACE_DEF; | ||
| } |
There was a problem hiding this comment.
this getParents and checking for nulls looks suspicious.
Some gut filling tell me that we should use - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/utils/ScopeUtil.java#L100
and be safe
Please also add to UT input cases like, and most likely in such case it is not public class, probably decompilation can say for sure:
$ cat Test.java
interface Test
{
default void test() {
class A {}
}
}
$ javac Test.java
$
There was a problem hiding this comment.
All members of a package scoped interface are still public (output from javap):
Compiled from "InputInterfaceMemberImpliedModifierPackageScopeInterface.java"
interface com.puppycrawl.tools.checkstyle.checks.modifier.interfacememberimpliedmodifier.InputInterfaceMemberImpliedModifierPackageScopeInterface {
public static final int fieldPublicStaticFinal;
public static final int fieldPublicStatic;
public static final int fieldPublicFinal;
public static final int fieldPublic;
public static final int fieldStaticFinal;
public static final int fieldStatic;
public static final int fieldFinal;
public static final int field;
public static void methodPublicStatic();
public static void methodStatic();
public void methodPublicDefault();
public void methodDefault();
public abstract void methodPublicAbstract();
public abstract void methodAbstract();
public abstract void methodPublic();
public abstract void method();
}
Compiled from "InputInterfaceMemberImpliedModifierPackageScopeInterface.java"
public interface com.puppycrawl.tools.checkstyle.checks.modifier.interfacememberimpliedmodifier.InputInterfaceMemberImpliedModifierPackageScopeInterface$NestedInterface {
}
| if (violateImpliedAbstractMethod | ||
| && modifiers.findFirstToken(TokenTypes.LITERAL_STATIC) == null | ||
| && modifiers.findFirstToken(TokenTypes.LITERAL_DEFAULT) == null | ||
| && modifiers.findFirstToken(TokenTypes.LITERAL_PRIVATE) == null |
There was a problem hiding this comment.
please move PRIVATE to the top of this group, to not mix visibility and "kind".
| @@ -1,3 +1,4 @@ | |||
| annotation.order=''{0}'' 注釈修飾子は、非注釈修飾子に先行していません。 | |||
| interface.implied.modifier=暗示された修飾子は明示的にする必要があります: ''{0}'' | |||
There was a problem hiding this comment.
please add 。 , just to be consistent
| @@ -1,3 +1,4 @@ | |||
| annotation.order=Annotation-Modifier ''{0}'' sollte vor den anderen Modifiern stehen. | |||
| interface.implied.modifier=Impliziter Modifikator sollte explizit sein: ''{0}'' | |||
There was a problem hiding this comment.
please put period as in English.
| /** | ||
| * Tests Java 9 private method on interface. | ||
| */ | ||
| public class Java9PrivateMethodOnInterfaceTest extends AbstractModuleTestSupport { |
There was a problem hiding this comment.
please explain this intention to have separate test.
We usually never do like this, grammar tests are for checking AST structure only, not running any Checks.
probably we misleaded you.
You need to keep all UTs in one file.
create folder "modifiers/interfaceimpliedmodifier" at https://github.com/checkstyle/checkstyle/tree/master/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/checks
put InputJava9PrivateMethodOnInterface.java in new folder. Ones we migrate to jdk9 compilation, we will move all such inputs to regular location.
Example of test on non-compilable input - https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheckTest.java#L62
There was a problem hiding this comment.
This is not a grammar test, so it shouldn't be here.
|
|
||
| void method(); | ||
|
|
||
| // cannot test private methods due to Java 8 baseline |
src/xdocs/config_modifier.xml
Outdated
| <td>violateImpliedPublicField</td> | ||
| <td> | ||
| Control whether to enforce that <code>public</code> is not being implied by the | ||
| compiler on interface fields (true).</td> |
There was a problem hiding this comment.
is not being implied by the compiler on interface fields
Please void such phrases over compiler, lets phrase it as it is pure style demand.
Control whether to enforce that <code>public</code> is being explicitly defined or smth similar.
Please fix in other properties.
src/xdocs/config_modifier.xml
Outdated
| <p> To configure the check: </p> | ||
| <source> | ||
| <module name="InterfaceMemberImpliedModifier"/> | ||
| </source> |
There was a problem hiding this comment.
please add here small snipped of code to show what behavior will be (with trailing comments "//violation")
| <property name="violateImpliedPublicNested" value="false"/> | ||
| <property name="violateImpliedStaticNested" value="false"/> | ||
| </module> | ||
| </source> |
There was a problem hiding this comment.
please add here small snipped of code to show what behavior will be (with trailing comments "//violation").
Unfortunately not that much good cases in our documentation to show you example, but user constantly complain about this. We try at least in new Checks enforce this good practice.
Good example: http://checkstyle.sourceforge.net/config_annotation.html#AnnotationLocation , there is human description, config, code example.
There was a problem hiding this comment.
@jodastephen , I did not found case in inner class in method that I mentioned in comments.
item to resolve:
| objBlock.addChild(init); | ||
| final DetailAST iface = new DetailAST(); | ||
| iface.setType(TokenTypes.INTERFACE_DEF); | ||
| iface.addChild(objBlock); |
There was a problem hiding this comment.
https://travis-ci.org/checkstyle/checkstyle/jobs/398313438#L672
please name it in better way "iface" ==> "interfaceAst"
CIs must pass.
|
@jodastephen See preferred japanese translation by speaker at #5974 (comment) |
|
|
||
| /** | ||
| * <p> | ||
| * Checks for implicit modifiers in interfaces. |
There was a problem hiding this comment.
I think it is more precise:
"Checks for implicit modifiers on interface members and nested types."
if you agree please change here in all other plases.
| * <ul> | ||
| * <li> | ||
| * Property {@code violateImpliedPublicField} - Control whether to enforce that {@code public} | ||
| * is explicitly coded and not implicitly added by the compiler on interface fields (true). |
There was a problem hiding this comment.
(true).
I am not sure what does this mean. If that is leftover from smth in past please remove (and in all other properties)
is explicitly coded
is explicitly coded on field
There was a problem hiding this comment.
@jodastephen , I still see "(true)" .
Please keep replying "done" or some disagreement or .... on each review point. Please trust my experience it helps a lot.
| * </li> | ||
| * <li> | ||
| * Property {@code violateImpliedStaticField} - Control whether to enforce that {@code static} | ||
| * is explicitly coded and not implicitly added by the compiler on interface fields (true). |
There was a problem hiding this comment.
is explicitly coded ==> is explicitly coded on <smth> .
Please fix in all cases.
do not forget about xdoc.
unfortunately generation of xdoc by javadoc is not finished yet.
| * </li> | ||
| * </ul> | ||
| * <p> | ||
| * This example checks that methods, fields and nested types are explicitly specified |
There was a problem hiding this comment.
checks that methods ...
checks that all implicit modifiers on methods ...
|
|
||
| /** | ||
| * Control whether to enforce that {@code public} is explicitly coded and | ||
| * not implicitly added by the compiler on interface fields (true). |
There was a problem hiding this comment.
explicitly coded on field.
please fix all other places.
| } | ||
|
|
||
| public default boolean methodWithLocalClass(String input) { | ||
| class LocalClass { |
There was a problem hiding this comment.
from #5974 (review)
Passing CI is good, but not enough, we need prove that there are no exceptions on real life ugly projects.
Please read README of https://github.com/checkstyle/contribution/tree/master/checkstyle-tester
Please launch launch.groovy on all projects that are referenced in property file(just uncomment all). There should be no exceptions. Unfortunately there will be too much violations to share in web, so please select 1-3 projects you like(openjdkX is good candidate) and run report generation on them only and share it on github.io for us to review.
If you have any concerns or questions do not hesitate to ask us.
|
Since we released new version, any versions saying 8.11 in this PR needs to change to 8.12 . |
|
All comments addressed. But the regression suite doesn't seem to work: I don't believe this is my changes - its not getting as far as running checkstyle. Also, the checkstyle-tester project does not have instructions for running |
|
to fix appveyor or travis, it is required to rebase on latest master, I just pushed fix to master. Reason #5985 |
I do not see your command to help, I am not sure what you launched.
please follow: do not forget to build SNAPSHOT from your branch, uncomment projects in projects-to-test-on.properties and use your Check in my_check.xml |
|
To get 31 projects passed. It then failed in parsing. FWIW, I use Windows, mvn 3.5.0, Java 1.8.0_152: Again, I don't think this is my code at fault. The regression process is very slow, so I don't have time now to run the script excluding this project. Maybe another day... |
please share a problem on old version. I can not reproduce problem. It still works fine for me.
This almost always mean problems with java parsing or non-compilable source. you run into existing problem in our parser - #3238 (comment) Please run again. |
did you run into the same issue as have today in our CIs - checkstyle/contribution#316 , if yes, please grab latest from our contributions repo. I pushed fixes, |
|
@jodastephen , please update Chinese translation as suggested at #5974 (comment) |
|
Launch groovy file passed without exceptions. Let me know if there is something else you need from me. |
|
@jodastephen , thanks a lot. While we do final reviews, please run launch.groovy one more time on all projects in properties BUT with severity "ignore", it will generate empty report if no exceptions found. No need to share such report. Just confirm in comment that there are no exceptions. Please also add your new Check to https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/checks-nonjavadoc-error.xml by PR, we will merge such PR after release, and your Check will be in automatic regression execution. |
|
@rnveach , please do final review, please pay attention to documentation , as it was main problem for us from the beginning. reports looks good, I did random recheck for about 30 cases (for static, final, public), all looks good.
this is false positive. |
Thanks |
| * specified even though they are actually redundant. | ||
| * </p> | ||
| * <p> | ||
| * Methods on interfaces are {@code public} by default, however from Java 9 they can also be |
There was a problem hiding this comment.
Methods on interfaces => Methods in interfaces
These things are in the interface, not on top of them.
later on you use within. that is acceptable too for all these.
| * coded and not implicitly added by the compiler. | ||
| * </p> | ||
| * <p> | ||
| * From Java 8, there are three types of methods on interfaces - static methods marked with |
There was a problem hiding this comment.
methods on interfaces => methods in interfaces
| * explicitly coded and not implicitly added by the compiler. | ||
| * </p> | ||
| * <p> | ||
| * Fields on interfaces are always {@code public static final} and as such the compiler does not |
There was a problem hiding this comment.
Fields on interfaces => Fields in interfaces
| * } | ||
| * </pre> | ||
| * <p> | ||
| * Rationale for these checks: Methods, fields and nested types are treated differently |
There was a problem hiding this comment.
these checks? plural? You never specify more than this check, so I think the rationale should only pertain to this check.
| * default methods are package-scoped on classes, but public on interfaces. However, from | ||
| * Java 8 onwards, interfaces have changed to be much more like abstract classes. | ||
| * Interfaces now have static and instance methods with code. Developers should not have to | ||
| * remember which modifiers are required and which are implicit. This check allows the simpler |
There was a problem hiding this comment.
which are implicit => which are implied
implicit is an adjective and you are using it as a verb in this scenario.
| * </ul> | ||
| * <p> | ||
| * This example checks that all implicit modifiers on methods, fields and nested | ||
| * types are explicitly specified on interfaces. |
There was a problem hiding this comment.
specified on interfaces => specified inside interfaces
| } | ||
|
|
||
| /** | ||
| * Check method on interface. |
There was a problem hiding this comment.
Check method on interface => Checks method in interface
| } | ||
|
|
||
| /** | ||
| * Check field on interface. |
There was a problem hiding this comment.
Check field on interface => Checks field in interface
|
|
||
| /** | ||
| * Check field on interface. | ||
| * @param ast method AST |
| } | ||
|
|
||
| /** | ||
| * Check nested types on interface. |
There was a problem hiding this comment.
Check nested types on interface => Checks nested type in interface
|
@romani Feel free to look over this one last time before merging. |
|
@jodastephen , thanks a lot for close collaboration, 9 days for PR acceptable for new module, it is pretty quick result. |
|
Thanks a lot to all translation participants, we appreciate your help and hope you will enjoy new validations in native languages. |
This PR implements #5899 with one extra property -
violateImpliedPublicNested- which was missing from the discussion. All tests/checks pass locally. The translations are automated. Thanks for your consideration!