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

EitherLogOrThrowCheck does not recognise lombock log annotations #784

Open
jansauer opened this issue Dec 3, 2019 · 3 comments
Open

EitherLogOrThrowCheck does not recognise lombock log annotations #784

jansauer opened this issue Dec 3, 2019 · 3 comments

Comments

@jansauer
Copy link

jansauer commented Dec 3, 2019

/var/tmp $ cat YOUR_FILE.java

import lombok.extern.slf4j.Slf4j;

@Slf4j
public class Test {
      
    public void demo() {
        try {
          throw new RuntimeException("test");
        } catch (RuntimeException exception) {
            log.error("test", exception);
            throw new RuntimeException("test", exception);
        }
    }
}

/var/tmp $ cat config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name = "Checker">
    <property name="charset" value="UTF-8"/>
    <property name="fileExtensions" value="java, properties, xml"/>
    <module name="TreeWalker">
        <module name="EitherLogOrThrowCheck"/>
    </module>
</module>

The rule violation in Test.java is not reported.
I expect that checkstyle doesn't recognise the logger as he gets declared as class field at compile time by lombok.

Interestingly adding the static final log field by hand to any class of the projects helps checkstle enough too then find EitherLogOrThrowCheck violations in the project.


PS: I really tried building a working cli example but was unable to do so.

@rnveach
Copy link
Contributor

rnveach commented Dec 3, 2019

Since lombok modifies the bytecode in the JVM, we have so far been against making special overrides just for it. We deal with the source code as it is.

See checkstyle/checkstyle#5470 .

@romani ping

@jansauer
Copy link
Author

jansauer commented Dec 3, 2019

@rnveach that confirms my assumption.

How about, instead of extending the check to recognise lombok / one liberty the EitherLogOrThrowCheck could have a parameter to tread all class fields with name X as loggers.

Also thx for the issue reference.

@romani
Copy link
Member

romani commented Dec 3, 2019

PS: I really tried building a working cli example but was unable to do so.

please share problems that run into, we do want to ease issue creation process, we appreciate feedback.
Did you fail to download jars ?


checkstyle is sourcecode analyzer (not a bitecode), so it works badly with bycode generation tools. Lombok is inly one of them, if we allow lombok there will another issue to allow something else.
Lombok can probably change/extend behavior of annotation and we will have to deal with different versions and .... .

As this is not a main checkstyle project, we have more freedom to experiments.
What if we will just create new Check based on EitherLogOrThrowCheck and name it EitherLogOrThrowLombokCheck and let it validate code as you suggested. So you will use new Check and all other users will not be affected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants