Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong documentation for InnerAssignment #6488

Closed
Vampire opened this issue Feb 27, 2019 · 12 comments

Comments

Projects
None yet
3 participants
@Vampire
Copy link
Contributor

commented Feb 27, 2019

Documentation shows how to configure the check for specific tokens, but possible tokens are not documented.
Looking at the code suggests that all supported tokens are required, so is the setting possible at all?
If so, please add valid tokens to documentation.
If not, why doesn't the analysis fail or show an error if you use the setting?

https://checkstyle.org/config_coding.html#InnerAssignment

@rnveach

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Basically it looks like the document is wrong right now as those tokens can't be configured. Display of customizable tokens is part of a xdoc validation.

Xdoc example has been there for 14 years: https://github.com/checkstyle/checkstyle/blame/8bf6df6e7a5910f7666ee690505520517f0e37b2/src/xdocs/config_coding.xml#L980

It looks like behavior of check changed with 6.10.

6:10 output:

$ cat TestClass.java
public class TestClass {
    void method() {
if ((a += b) == 2) {}
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.2//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_2.dtd">

<module name="Checker">

    <module name="TreeWalker">
<module name="InnerAssignment">
  <property name="tokens" value="DIV_ASSIGN"/>
</module>
    </module>
</module>

$ java -jar checkstyle-6.10-all.jar -c TestConfig.xml TestClass.java
Starting audit...
TestClass.java:3:8: Inner assignments should be avoided.
Audit done.
Checkstyle ends with 1 errors.

6.9 output:

$ java -jar checkstyle-6.9-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

This change is in the scope of this issue, d56a2a3#diff-03d545f1964bfdfae7e54dfd979c5d9f , which seems like it wasn't mean to change the functionality of the check.

@romani Please share your thoughts on this issue.

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

And an invalid property is not meant to rise some error?

@rnveach

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@Vampire It is technically not an invalid property.

Invalid means that the property does not exist. The property tokens always exists if the check extends AbstractCheck, which it does in this case.
An error will arise if a specific token is added that is not in the acceptable list, but these tokens are still in the acceptable list.
No error will be given if a token is added that is in the required list as it is acceptable and required. In this case all the tokens are deemed required after 6.9 .
Since all tokens are required and none are customizable, the xdoc only shows token properties that are customizable. This is why the xdoc doesn't show tokens as a property as it is deemed all these tokens are required for the check to do it's business and would impact the check's functionality if they could somehow be removed. There is no point in showing what you can't change.

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Sure, I see that it does make no sense to document tokens if they are all required anyway.
I just wondered that it does not give an error if it makes no sense to set it that way.
To the user it looks like he was able to set the list of tokens, while it does not have any effect actually.

@rnveach

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

it does not give an error if it makes no sense to set it that way

You may not see a sense in it. We have deemed one case where it would. See #6385 .

The xdoc tests could be stricter about properties being used in examples to warn us about situations like this where a property is used that is not in the list of customizable properties.

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Honestly I don't see where that issue applies.
As far as I understood it just says to configure all tokens explicitly where the list is configurable.
But still a check could be done whether the config done is effectless, or where required tokens are configured explicitly or where some required tokens are configured but not all.

@romani

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Yes, example in xdoc need to be updated to show how Check work now.

I do not think it is reasonable to configure tokens.

If Check has non configurable list of tokens(required token list is equal to allowed list), user still should be able to define list of tokens in config, but if list if not full, Check should throw some sort of exception.

@romani

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

seems like it wasn't mean to change the functionality of the check.

It is a bug.

@romani romani added the approved label Mar 3, 2019

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

seems like it wasn't mean to change the functionality of the check.

It is a bug.

Current behavior or previous behavior?
What exactly do you reference with "it"?

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@romani ping

@romani

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Sorry for confusion.
Reviewed issue from the beginning.

if ((a += b) == 2) {} is inner assignment, so violation in 6.10 is good, no violation in previous version 6.9 was not good.
So problem in Check behavior.

But documentation example of it should be fixed. Fact that Check might throw exception of bad configuration, is better to move to separate issue ( looks like it is already there #2492)

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 30, 2019

rnveach added a commit that referenced this issue Mar 31, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Fix was merged

@rnveach rnveach added this to the 8.19 milestone Mar 31, 2019

Vantuz added a commit to Vantuz/checkstyle that referenced this issue Apr 3, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 27, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.