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

Getting SLF4J_FORMAT_SHOULD_BE_CONST on error(String msg) #32

Closed
robertandrewbain opened this issue Apr 15, 2015 · 10 comments
Closed

Getting SLF4J_FORMAT_SHOULD_BE_CONST on error(String msg) #32

robertandrewbain opened this issue Apr 15, 2015 · 10 comments
Assignees

Comments

@robertandrewbain
Copy link

The following code will reproduce the issue:

import lombok.extern.slf4j.Slf4j;

@Slf4j
public class IssueDemonstrator {

    private void triggerWarning() {
        logMessage("This will trigger SLF4J_FORMAT_SHOULD_BE_CONST");
    }

    private void logMessage(String msg) {
        log.error(msg);
    }

}

Maven output to follow:

[INFO] Done FindBugs Analysis....
[INFO]
[INFO] <<< findbugs-maven-plugin:3.0.1:check (default-cli) < :findbugs @ *********** <<<
[INFO]
[INFO] --- findbugs-maven-plugin:3.0.1:check (default-cli) @ * ---
[INFO] BugInstance size is 1
[INFO] Error size is 0
[INFO] Total bugs: 1
[INFO] Format should be constant [
.IssueDemonstrator] At IssueDemonstrator.java:[line 13]
[INFO]

@KengoTODA
Copy link
Owner

Yes, and currently I have no plan to fix this problem, because it's hard to judge value of argument is CONST or not.

@jeffjensen
Copy link

But it is still a problem!

On Fri, Apr 17, 2015 at 6:51 PM, Kengo TODA notifications@github.com
wrote:

Closed #32 #32.


Reply to this email directly or view it on GitHub
#32 (comment).

@robertandrewbain
Copy link
Author

A class constant will cause this issue too when passed into a method. This is a serious bug.

@robertandrewbain
Copy link
Author

Remove the findbugs warning if you can't apply it accurately, otherwise developers are failing builds which should pass.

@KengoTODA
Copy link
Owner

@robertandrewbain @jeffjensen
Could you try this as a workaround? And your PR is always welcome :)

@jeffjensen
Copy link

Yes, we've already been using the FindBugs exclude filters for the specific
classes.

On Sat, Apr 18, 2015 at 11:35 PM, Kengo TODA notifications@github.com
wrote:

@robertandrewbain https://github.com/robertandrewbain @jeffjensen
https://github.com/jeffjensen
Could you try this
http://stackoverflow.com/questions/1829904/is-there-a-way-to-ignore-a-single-findbugs-warning/1829960#1829960
as a workaround? And your PR is always welcome :)


Reply to this email directly or view it on GitHub
#32 (comment)
.

@KengoTODA
Copy link
Owner

@jeffjensen @robertandrewbain
Today I have released version 1.2.0 and it will be deployed onto Maven central soon.
Could you try this version? It should be solution for your case.

Note that I tested 1.2.0 on Java 8. If your product depends on Java 7 or older one, it might have problem.

@KengoTODA KengoTODA self-assigned this Jul 18, 2015
@jeffjensen
Copy link

Just verified it still has false positive on Java 7.

@KengoTODA
Copy link
Owner

Got it, thanks for your verification.

I have a question: is it acceptable for you, to limit scope (visibility) of method to support? I mean, can I keep current behaviour for public & package-private methods?

class Foo {
  :
  :
  :
  private void log(String string) {
    logger.info(string); // VALID; we can verify that all caller do not use dynamically constructed string
  }
  void packagePrivateLog(String string) {
    logger.info(string); // INVALID; we cannot verify that all caller do not use dynamically constructed string
  }
  public void publicLog(String string) {
    logger.info(string); // INVALID; we cannot verify that all caller do not use dynamically constructed string
  }
}

If so, I will handle this issue in #35.

@jeffjensen
Copy link

Yes, this idea is a much better workaround than using FindBugs filters. I like this rule very much, but it gives too much false positive yet.
Thank you for continuing to work on this issue!

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