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

Update all AbstractChecks to log DetailAST #4830

Closed
MEZk opened this Issue Jul 26, 2017 · 25 comments

Comments

4 participants
@MEZk
Copy link
Contributor

MEZk commented Jul 26, 2017

All AbstractChecks should use log(DetailAST ast, String key, Object... args) method to log DetailAST.

lineNo, columnNo, and tokenType should be retrieved from DetailAST and passed to LocalizedMessage.

This issue is blocked by: #4419

@timurt

This comment has been minimized.

Copy link
Collaborator

timurt commented Jul 30, 2017

@MEZk @rnveach
I want to create additional method: log(int lineNo, int tokenType, String key, Object... args). Parameter types are the same with existing method log(int lineNo, int colNo, String key, Object... args)
Should I change order of parameters or change name of the function for example logAstLine(int lineNo, int tokenType, String key, Object... args)

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Jul 30, 2017

I want to create additional method: log(int lineNo, int tokenType, String key, Object... args).

No, you should not do this. It will increase the number of parameters.

@timurt

This comment has been minimized.

Copy link
Collaborator

timurt commented Jul 30, 2017

@MEZk
But how I should deal with log(int line, String key, Object... args) calls, since it has no parameter for tokenType?

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Jul 30, 2017

@timurt

All AbstractChecks should use log(DetailAST ast, String key, Object... args) method to log DetailAST.

@timurt

This comment has been minimized.

Copy link
Collaborator

timurt commented Jul 30, 2017

@MEZk
Here is example, where AvoidEscapedUnicodeCharactersCheck which is extension of AbstractCheck uses log(ast.getLineNo(), MSG_KEY), should it be rewritten to log(ast, MSG_KEY) or logAstLine(ast.getLineNo(), ast.getType(), MSG_KEY)? I think the second one is correct.

Another example is this, which passes changed columnNo to log method, I think it should be extended to

log(ast.getLineNo(),
                ast.getColumnNo() + ast.getText().length() - 1,
                ast.getType(),
                MSG_KEY)

Therefore, we should create additional log method with signature log(int lineNo, int colNo, int tokenType, String key, Object... args)

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Jul 30, 2017

Here is example, where AvoidEscapedUnicodeCharactersCheck which is extension of AbstractCheck uses log(ast.getLineNo(), MSG_KEY), should it be rewritten to log(ast, MSG_KEY)

Yes. Example of usage: https://github.com/MEZk/checkstyle/blob/ce21086e087661553f3a774c38362327ee88996a/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/LeftCurlyCheck.java#L325-L325

Another example is this, which passes changed columnNo to log method, I think it should be extended to

No, all information can be retrieved from DetailAST directly.

Therefore, we should create additional log method with signature log(int lineNo, int colNo, int tokenType, String key, Object... args)

No, the required method already exists. See the issue description.

@timurt

This comment has been minimized.

Copy link
Collaborator

timurt commented Aug 1, 2017

@MEZk @rnveach

Yes. Example of usage:

rewriting log calls (AvoidEscapedUnicodeCharactersCheck) gives errors like this
Expected :InputAvoidEscapedUnicodeCharacters.java:104: Unicode escape(s) usage should be avoided.
Actual :InputAvoidEscapedUnicodeCharacters.java:104:38: Unicode escape(s) usage should be avoided.

I think instead of storing tokenType in LocalizedMessage type we should store DetailAst object or add new fields like realLineNo, realColumnNo because columnNo is often being changed. Sometimes columnNo is set to zero, increased by 1 or by some value, decreased and etc, even inside log(int lineNo, int colNo, String key, Object... args) we can see some arithmetic operation of column number

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Aug 1, 2017

@timurt
These checks logs both lineNo and columnNo. They can be updated to log DetailAST. I see no problems there:

AbstractClassNameCheck
AbstractNameCheck
VisibilityModifierCheck
ThrowsCountCheck
StringLiteralEqualityCheck
SimplifyBooleanReturnCheck
SimplifyBooleanExpressionCheck
SeparatorWrapCheck
ReturnCountCheck
RedundantModifierCheck
RedundantImportCheck
ParameterNumberCheck
ParameterAssignmentCheck
OperatorWrapCheck
MutableExceptionCheck
ModifierOrderCheck
ModifiedControlVariableCheck
MethodNameCheck
MethodLengthCheck
MagicNumberCheck
JavaNCSSCheck
InnerTypeLastCheck
InnerAssignmentCheck
IllegalTokenTextCheck
IllegalTokenCheck
IllegalInstantiationCheck
IllegalImportCheck
HideUtilityClassConstructorCheck
FinalParametersCheck
FinalLocalVariableCheck
ExecutableStatementCountCheck
EqualsHashCodeCheck
EqualsAvoidNullCheck
EmptyStatementCheck
EmptyBlockCheck
DesignForExtensionCheck
DescendantTokenCheck
CovariantEqualsCheck
BooleanExpressionComplexityCheck
AvoidNestedBlocksCheck
AvoidInlineConditionalsCheck
ArrayTypeStyleCheck
AnonInnerLengthCheck
AnnotationUseStyleCheck
AbstractSuperCheck

These checks causes problems, as they log modified, or shifted column or line number, or AST is not available to log due to some wrapper, etc:

WhitespaceAroundCheck
WhitespaceAfterCheck
UpperEllCheck
UnusedImportsCheck
SuppressWarningsCheck
SingleSpaceSeparatorCheck
PackageNameCheck
NoWhitespaceBeforeCheck
NoWhitespaceAfterCheck
MultipleStringLiteralsCheck
JavadocTypeCheck
JavadocStyleCheck
JavadocMethodCheck
IllegalTypeCheck
GenericWhitespaceCheck
EmptyForIteratorPadCheck
EmptyForInitializerPadCheck
AbstractParenPadCheck
AbstractClassCouplingCheck

This check is deprecated, but also causes problems during log.

AbstractTypeAwareCheck
  1. Please, update the checks to log DetailAST. Send separate PR. Provide regression reports for all the updated checks.
  2. Investigate the reason why we cannot log DetailAST for the problematic checks. You have to describe the problem for each check in this issue.
  3. Write a list of checks which logs only line or column number.
@timurt

This comment has been minimized.

Copy link
Collaborator

timurt commented Aug 2, 2017

@rnveach @MEZk
Main problem I have tried to explain is LocalizedMessage often stores non-original column number.
If you look at the method log(int lineNo, int colNo, String key, Object... args), you can see that variable col passed to LocalizedMessage was initialized using CommonUtils.lengthExpandedTabs method based on lineNo, colNo and tabWidth variables.

Example. Here is my implementation of log method based on DetailAst, it is almost the same as original one, additionally I store tokenType

public final void log(DetailAST ast, String key, Object... args) {
        final int col = 1 + CommonUtils.lengthExpandedTabs(
                getLines()[ast.getLineNo() - 1], ast.getColumnNo(), tabWidth);
        messages.add(
                new LocalizedMessage(
                        ast.getLineNo(),
                        col,
                        ast.getType(),
                        getMessageBundle(),
                        key,
                        args,
                        getSeverityLevel(),
                        getId(),
                        getClass(),
                        getCustomMessages().get(key)));
    }

The following test will fail

@Test
    public void testLog() {
        final AbstractCheck check = new DummyAbstractCheck();
        final FileContents fileContents = new FileContents(
                new FileText(new File("filename"), Arrays.asList("  public class Main {    ", "  int a;  ", " } ")));
        check.setFileContents(fileContents);
        final DetailAST root = new DetailAST();

        root.setType(TokenTypes.METHOD_DEF);
        root.setLineNo(2);
        root.setColumnNo(2);

        check.log(root, "key", "args");
        Assert.assertEquals(2, check.getMessages().first().getColumnNo());
    }

As I understood, minimal possible column number stored inside log messages is 1, while actual column number are 0-based, so we always have +1

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Aug 2, 2017

By the way we change columnNo in DetailAST intentionally. We cannot just change it to

columnNo = tok.getColumn();

because some checks logic depends on column and line number. For example LeftCurlyCheck.

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Aug 12, 2017

@romani @rnveach

If we create LocalizedMessage inside log(DetailAST ast, String key, Object... args) without changing of column number, then the following Checks which use the log method will become broken:

DefaultCOmesLast
MultipleVariableDeclarations
UnnecessaryParentheses
JavadocVariable
ExplicitInitialization
NPathComplexity
CyclomaticComplexity
RightCurlyCheck
HiddenField
NestedTryDepth
NestedForDepth
OuterTypeNumber
FallThrough
RequireThis
IllegalThrows
NestedIfDepth
OneStatementPerLine
ImportControl
MethodParamPad
IllegalCatch

as column numbers in violations will be decreased by one. It's a massive regression. I don't know how we can review all regression reports.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Aug 14, 2017

It's a massive regression. I don't know how we can review all regression reports.

If violations are off by 1, we can either modify branch just for regression reports, or make a small script to modify regression's XML report to align numbers and make reports easier to view.

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Aug 27, 2017

@timurt
Did you start working on the issue?

@timurt

This comment has been minimized.

Copy link
Collaborator

timurt commented Apr 20, 2018

@MEZk @romani @rnveach
About this issue,

  1. Some checks pass modified column number to AbstractCheck.log(int lineNo, int colNo, String key, Object... args) method. For example EmptyForInitializerPadCheck or SingleSpaceSeparatorCheck.
  2. Some checks use HtmlTag, FullIdent, StringInfo or JavadocTag objects' line and column number. For example llegalTypeCheck or JavadocMethodCheck.
  3. Some checks pass only line number to AbstractCheck.log(int line, String key, Object... args) method. For example OuterTypeFilenameCheck or TrailingCommentCheck.

How to resolve

  1. We can add additional method AbstractCheck.log(DetailAST ast, int colNo, String key, Object... args) to resolve first problem. From ast we retrieve line number, real column number and token type, colNo parameter is modified column number for user messages.
  2. Can we add tokenType field to HtmlTag, FullIdent, StringInfo and JavadocTag classes? If no, we can add additional method AbstractCheck.log(int lineNo, int colNo, int tokenType, String key, Object... args) to handle such cases.
  3. The third problem can be resolved adding the following method AbstractCheck.logLine(DetailAST ast, String key, Object... args). We cannot use method AbstractCheck.log(DetailAST ast, String key, Object... args) because non-zero column number will be stored.

What do you think?

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Apr 20, 2018

@timurt Those are good ways to resolve the issue if we want to keep the violations the same, only reporting line number or reporting a generated line/column.

But if the column/line doesn't match the AST, users won't be able to generate a suppression through the CLI. For example, Type 1, say we print line/column as 1:10 and AST's line/column is 1:1. Chances are suppression generator will give us a different or no AST for 1:10 and thus the suppression generated won't work. Right? So fixing the log method used might not be the best path since it has to match the suppression generator.

We should look into why the checks were designed this way. Type 3 will be the easiest. Why are we only printing violations on line, and not line/column? Will there be any negative effects to switching it to log the line and column? If there are no good reasons, I think we should switch it to AbstractCheck.log(DetailAST ast, ... and take the hit with regression.

This is just my opinion.

Some checks use ... FullIdent ... objects' line and column number. For example llegalTypeCheck

llegalTypeCheck's FullIdent makes sure we use the start of the type for the line/column. Example: java.util.List starts with java, but last AST used points to .List. Same with Boolean[], starts with Boolean but last AST used points to [].

FullIdent could be modified to save the AST that starts the type so we can then retrieve the correct AST.

Some checks pass modified column number to AbstractCheck.log(int lineNo, int colNo, String key, Object... args) method. For example EmptyForInitializerPadCheck

Calculation is -1 from true column number so space violation can be pinpointed. The violation message is already ''{0}'' is not preceded with whitespace. so I don't see why we can't use the accurate line/column from the AST.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Apr 23, 2018

@timurt Until someone else chimes in with a better idea, please start with the simple cases where we can swap type 1 where we don't do any calculations or use special classes.
IE: log(ast.getLineNo(), ast.getColumnNo(), MSG_KEY....

Here are the following checks with atleast 1 log of this type that I saw: AbstractParenPadCheck, AnonInnerLengthCheck, BooleanExpressionComplexityCheck, ExecutableStatementCountCheck, JavaNCSSCheck, ParameterNumberCheck, ThrowsCountCheck, ReturnCountCheck, RedundantImportOrderCheck, DescendantTokenCheck, AbstractClassNameCheck, AbstractNameCheck, DesignForExtension, AbstractSuperCheck, EmptyBlockCheck, FinalLocalVariableCheck, FinalParameterCheck, etc....

If any checks are fully supported (all logs in check were converted), add them to the XDoc list.
Any that aren't added will be covered when we work on the rest.

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Apr 26, 2018

@timurt

Some checks pass only line number to AbstractCheck.log(int line, String key, Object... args) method. For example OuterTypeFilenameCheck or TrailingCommentCheck.

@timurt Until someone else chimes in with a better idea, please start with the simple cases where we can swap type 1 where we don't do any calculations or use special classes.

This is a good idea by @rnveach . Do not try to resolve problem 1 and 2, till we discuss them with @romani and @rnveach, and come up with the final solution. As @rnveach sad the checks which log only line number and do not modify line number can be easily converted to log ast. As we discussed previously with @romani , it is OK to print lineNo and columnNo in the violation message (ex. 1:1 ''{0}'' is not preceded with whitespace), but please, review each message of the modifying check, try to figure out if it become understandable for user, and if yes, change the check to log ast, if no, write the description of the problem to this topic. Do not rush, we need to keep all checks working.

@rnveach @romani
Maybe it will be better to have one PR for each of this checks to ease review:

AbstractParenPadCheck, AnonInnerLengthCheck, BooleanExpressionComplexityCheck, ExecutableStatementCountCheck, JavaNCSSCheck, ParameterNumberCheck, ThrowsCountCheck, ReturnCountCheck, RedundantImportOrderCheck, DescendantTokenCheck, AbstractClassNameCheck, AbstractNameCheck, DesignForExtension, AbstractSuperCheck, EmptyBlockCheck, FinalLocalVariableCheck, FinalParameterCheck, etc...

What do you think?

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Apr 26, 2018

Maybe it will be better to have one PR for each of this checks to ease review:

I am fine with multiple easy changes in 1 PR. Stuff like log(ast.getLineNo(), ast.getColumnNo(), MSG_KEY.... will just be a 1 line change for each Check and require no changes in tests. All tests should pass as they are now with the change.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Apr 26, 2018

@MEZk @timurt Since we are going to need some discussions on the different types, maybe we should split them into separate issues to make reading and management easier?
The extremely easy ones can be done in this issue or a separate issue from the other 3.

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented Apr 26, 2018

I'm closing these issue as I split it to 3 separate: #5757, #5758, #5759.

All further discussion should be continued in the scope of these 3 issues.

@MEZk MEZk closed this Apr 26, 2018

@MEZk MEZk moved this from To Do to Done in Flexible Suppression Model Apr 26, 2018

@romani

This comment has been minimized.

Copy link
Member

romani commented May 3, 2018

Some checks pass modified column number to

It was ok, when we did not care. Such Checks need to be updated to report violation on nearest AST element ( before or after). It most likely will require update in Check message, like "there are non single space after symbol ')' " and we should report violation on ')'.

Can we add tokenType field to HtmlTag, FullIdent, StringInfo and JavadocTag classes?

This classes are internal, javadoc related could be skipped for now, as we do not support xpath in javadoc. Yes, they could be updated to have required field to make logging of violation more precise. It would be better to update Checks to log violation not base on such objects, but let's see what will be easier.

Some checks pass only line number

Just use first or more related AST and report violation on it. Message need be rechecked and could be updated to be reasonable. We will see what to do in each particular case.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented May 3, 2018

@MEZk Shouldn't there be 4 issues as we split type 1 into easy and hard cases and closed this issue?

@MEZk

This comment has been minimized.

Copy link
Contributor

MEZk commented May 6, 2018

@rnveach
Done

@romani

This comment has been minimized.

Copy link
Member

romani commented May 8, 2018

@MEZk , should we close this issue ?

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented May 8, 2018

@romani This issue is closed.

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