Skip to content

Conversation

@pkoenig10
Copy link
Member

Before this PR

The LogsafeArgName check would fail if using a compile time constant identifier (static final variables) for args names.

This fails with the following exception:

java.lang.AssertionError: An unhandled exception was thrown by the Error Prone static analysis plugin.
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.4.0
     BugPattern: LogsafeArgName
     Stack Trace:
     java.lang.ClassCastException: class com.sun.tools.javac.tree.JCTree$JCIdent cannot be cast to class com.sun.tools.javac.tree.JCTree$JCLiteral (com.sun.tools.javac.tree.JCTree$JCIdent and com.sun.tools.javac.tree.JCTree$JCLiteral are in module jdk.compiler of loader 'app')
  	at com.palantir.baseline.errorprone.LogsafeArgName.matchMethodInvocation(LogsafeArgName.java:77)
  	at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:451)

After this PR

The LogsafeArgName ignores arg names that are not literals.

@changelog-app
Copy link

changelog-app bot commented Aug 3, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

The LogsafeArgName now ignores arg names that are not literals.

Check the box to generate changelog(s)

  • Generate changelog entry

@pkoenig10 pkoenig10 requested a review from ferozco August 3, 2020 16:45
.addFix(builder.replace(tree.getMethodSelect(), unsafeArg + ".of")
.build())
.build();
if (argNameExpression instanceof JCTree.JCLiteral) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we might as well combine these two conditional expressions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just get rid of the compile time constant matcher, right? All literals are compile time constants.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right

@bulldozer-bot bulldozer-bot bot merged commit 8524f81 into develop Aug 3, 2020
@bulldozer-bot bulldozer-bot bot deleted the pkoenig/argNameExpression branch August 3, 2020 17:01
@svc-autorelease
Copy link
Collaborator

Released 3.36.1

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants