Skip to content

[java] Revamp GuardLogStatementRule to allow var, field and array accesses#5169

Merged
adangel merged 19 commits into
pmd:masterfrom
jsotuyod:issue-5151
Aug 29, 2024
Merged

[java] Revamp GuardLogStatementRule to allow var, field and array accesses#5169
adangel merged 19 commits into
pmd:masterfrom
jsotuyod:issue-5151

Conversation

@jsotuyod

@jsotuyod jsotuyod commented Aug 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Describe the PR

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@jsotuyod jsotuyod added the a:false-positive PMD flags a piece of code that is not problematic label Aug 18, 2024
@jsotuyod jsotuyod added this to the 7.5.0 milestone Aug 18, 2024
@ghost

ghost commented Aug 18, 2024 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 Compared to master:
This changeset changes 0 violations,
introduces 6 new violations, 0 new errors and 0 new configuration errors,
removes 28 violations, 0 errors and 0 configuration errors.
Download full report as build artifact
✅ Compared to master:
This changeset changes 0 violations,
introduces 6 new violations, 0 new errors and 0 new configuration errors,
removes 28 violations, 0 errors and 0 configuration errors.
Download full report as build artifact
✅ Compared to master:
This changeset changes 0 violations,
introduces 6 new violations, 0 new errors and 0 new configuration errors,
removes 28 violations, 0 errors and 0 configuration errors.
Download full report as build artifact
✅ Compared to master:
This changeset changes 0 violations,
introduces 6 new violations, 0 new errors and 0 new configuration errors,
removes 28 violations, 0 errors and 0 configuration errors.
Download full report as build artifact
✅ Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 29 violations, 2 errors and 7 configuration errors.
Download full report as build artifact
✅ Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 16 violations, 2 errors and 7 configuration errors.
Download full report as build artifact
✅ Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 13 violations, 2 errors and 7 configuration errors.
Download full report as build artifact
✅ Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 13 violations, 2 errors and 7 configuration errors.
Download full report as build artifact

Generated by 🚫 Danger

@jsotuyod jsotuyod mentioned this pull request Aug 18, 2024
Closed
@jbisotti

jbisotti commented Aug 19, 2024

Copy link
Copy Markdown

It would be great if the documentation could also be expanded to explain the nuances here. Maybe include examples like in my original Issues that should pass the check and then slight variations of them that would not. Thanks.

@jsotuyod
Properly handle all qualifiable expressions
4033315 
 - Up date the documentation to better show what is being flagged
@jsotuyod

jsotuyod commented Aug 22, 2024

Copy link
Copy Markdown
Member Author

@jbisotti I changed the rule description slightly and added some extra examples, feel free to comment.

The bottom line is, we should flag any "expensive operations", with expensive being one of:

  • string manipulation / concatenation
  • method invocations
  • constructor invocations

The last 2 are specially dangerous as the methods may also have side-effects, so you are using a log call to actually perform business logic.

Accessing fields / variables / array elements / method references directly is ok. Doing that from the return of a method invocation / constructor call is not.

jsotuyod and others added 4 commits August 24, 2024 00:11
@jsotuyod
Handle computed array access keys
96f60e9 
 - Tidy up the code, the fact the `getLogLevelName` was cheating and
   returning null when it considered a log to be safe was making the
   code harder to think about
@jsotuyod
Merge branch 'master' into issue-5151
8ecc21c
@jsotuyod
Unify compile-time constant detection
980eaac 
 - Improving the constant folder allows other rules to work better
 - Fixes pmd#3602
 - Curiously, InefficientStringBuffering has been broken since PMD 7.0.0
   (regression introduced in pmd#3113)
@jsotuyod
Update changelog, refs pmd#3602
1c274fd
@jsotuyod jsotuyod mentioned this pull request Aug 24, 2024
Closed
@jsotuyod
Suppress PMD warning
c9c0558 
 - "Simplifying" the return here would simply make it harder to read
@jsotuyod jsotuyod changed the title [java] Allow field accesses in GuardLogStatementRule [java] Revamp GuardLogStatementRule to allow var, field and array accesses Aug 24, 2024
@adangel adangel self-requested a review August 27, 2024 18:00
adangel
adangel approved these changes Aug 29, 2024
View reviewed changes

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@adangel adangel merged commit 40688ae into pmd:master Aug 29, 2024
@jsotuyod jsotuyod deleted the issue-5151 branch August 29, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment