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

IllegalType: illegal types in overridden methods should not be reported #6612

Closed
Vampire opened this issue Mar 26, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@Vampire
Copy link
Contributor

commented Mar 26, 2019

/var/tmp $ javac Foo.java

/var/tmp $ cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
    <module name="TreeWalker">
        <module name="IllegalType">
            <property name="illegalClassNames" value="StringBuffer"/>
        </module>
    </module>
</module>

/var/tmp $ cat Bar.java
interface Bar {
    void bar(StringBuffer buffer);
    StringBuffer bar();
    Object baz();
}

/var/tmp $ cat Foo.java
class Foo implements Bar {
    public void foo(StringBuffer buffer) { // line 2
    }
    public StringBuffer foo() { // line 4
        return null;
    }
    @Override
    public void bar(StringBuffer buffer) { // line 8
    }
    @Override
    public StringBuffer bar() { // line 11
        return null;
    }
    @Override
    public StringBuffer baz() { // line 15
        return null;
    }
}

/var/tmp $ java -Duser.language=en -Duser.country=US -jar checkstyle-8.18-all.jar -c config.xml Foo.java
Starting audit...
[ERROR] D:\Sourcecode\other\discord-logger\tmp\Foo.java:2:21: Usage of type 'StringBuffer' is not allowed. [IllegalType]
[ERROR] D:\Sourcecode\other\discord-logger\tmp\Foo.java:4:12: Usage of type 'StringBuffer' is not allowed. [IllegalType]
[ERROR] D:\Sourcecode\other\discord-logger\tmp\Foo.java:8:21: Usage of type 'StringBuffer' is not allowed. [IllegalType]
[ERROR] D:\Sourcecode\other\discord-logger\tmp\Foo.java:11:12: Usage of type 'StringBuffer' is not allowed. [IllegalType]
[ERROR] D:\Sourcecode\other\discord-logger\tmp\Foo.java:15:12: Usage of type 'StringBuffer' is not allowed. [IllegalType]
Audit done.
Checkstyle ends with 5 errors.

The 8:21 finding should not be there.
If you override a method, there is no chance to change the parameter types in any way or you overload the method instead.


Additionally demanded by project maintainers:
11:12 and 15:12 should not be reported either. Even though original return type is Object and can be changed in the overridden class as seen in the example. It will be hard to decide where to raise violation and where not to. It is better to have some false negatives rather than false positives.
I'm not sure it is possible to write good suppression for such cases.
All overridden methods should be ignored from validation.


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

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Please include Bar so we can get a full picture even if checkstyle can't.

you can e. g. override a method that returns Object and declare the overridden method to return StringBuffer

I think we can't make this assumption especially if the parent is in another class or jar that checkstyle can't read. We can't determine the parent's return type so we don't know if it is Object or actually StringBuffer. IMO, It would be rare that someone used Object in the parent return type instead of a physical type. If your wrong and they used StringBuffer there would be no way to remove the violation and we would be right back here.

We should ignore everything about the method. Return type, parameters, and throws clause.

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Please include Bar so we can get a full picture even if checkstyle can't.

Sorry, my bad, added.

you can e. g. override a method that returns Object and declare the overridden method to return StringBuffer

I think we can't make this assumption especially if the parent is in another class or jar that checkstyle can't read. We can't determine the parent's return type so we don't know if it is Object or actually StringBuffer. IMO, It would be rare that someone used Object in the parent return type instead of a physical type. If your wrong and they used StringBuffer there would be no way to remove the violation and we would be right back here.

We should ignore everything about the method. Return type, parameters, and throws clause.

My point is, that you can ignore the violation by suppression, but you cannot un-ignore it as user.
Object was just an example.
Let's instead assume the super method returns List and the overridden method returns ArrayList.
If the super method actually enforces this type, the finding can easily be suppressed.
And for thrown exceptions, you can actually anytime choose to not declare the throws or throw something else that is compatible but may or may not violate the rule.
For parameters you have no choice, if there is a violation it is always a false-positive as you have to have the exact same parameter types.

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@romani Please decide.

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

@romani

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

For return types it is different though, as you can e. g. override a method that returns Object and declare the overridden method to return StringBuffer or whatever, so there must still be a finding reported.

It is better to ignore all on override method, it will be hard to decide where to raise violation and where do not. It is better to have false negative rather than false positives.
I not sure it is possible to write good suppression for such cases.

@Vampire, does it make sense ?

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Well, your decision.
I still think it is better to have some false-positives that can be suppressed by the user than having false-negtives that cannot be de-suppressed.
I would suggest adding an option on whether to check return types, but afair you want to avoid adding options to rules.

Vampire added a commit to Vampire/checkstyle that referenced this issue Apr 8, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

I updated issue with discussion. I am marking this approved.

@rnveach rnveach added the approved label Apr 8, 2019

@rnveach rnveach changed the title illegal types in parameters of overridden methods should not be reported IllegalType: illegal types in overridden methods should not be reported Apr 8, 2019

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

it is rare parent class would use such a loose restriction o_O what?
If you refer to Object, that was just one of many examples.
Parent could define CharSequence and child could return StringBuffer illegally.
Parent could define List<String> and child could return Vector illegally.
Also, if you edit a text by me with information you know I disagree, please mark it accordingly.

Vampire added a commit to Vampire/checkstyle that referenced this issue Apr 10, 2019

Issue checkstyle#6612: do not complain about illegal types as paramet…
…ers or return types of methods with @override annotation

Vampire added a commit to Vampire/checkstyle that referenced this issue Apr 11, 2019

Issue checkstyle#6612: do not complain about illegal types as paramet…
…ers or return types of methods with @override annotation

@romani romani closed this in #6613 Apr 27, 2019

romani added a commit that referenced this issue Apr 27, 2019

@romani romani added this to the 8.20 milestone Apr 27, 2019

@romani romani added the bug label Apr 27, 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.