Skip to content

#349: Implement non final argument pattern#1283

Open
ogluvu-ship-it wants to merge 3 commits into
cqfn:masterfrom
ogluvu-ship-it:fix-issue-349-non-final-argument
Open

#349: Implement non final argument pattern#1283
ogluvu-ship-it wants to merge 3 commits into
cqfn:masterfrom
ogluvu-ship-it:fix-issue-349-non-final-argument

Conversation

@ogluvu-ship-it

@ogluvu-ship-it ogluvu-ship-it commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Added the “Non final argument” (P35) pattern to detect methods and constructors with parameters that are not declared final.
    • Registered the pattern for use in the application’s pattern configuration.
  • Bug Fixes

    • Prevented duplicate findings when multiple non-final parameters occur in the same method or constructor.
  • Documentation

    • Added guidance and a Java example for the new pattern.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Non-final argument pattern

Layer / File(s) Summary
AST-based detection and tests
aibolit/patterns/non_final_argument/non_final_argument.py, test/patterns/non_final_argument/test_non_final_argument.py
NonFinalArgument.value now accepts an AST and reports each offending method or constructor line once; tests build AST inputs explicitly.
Pattern registration and documentation
aibolit/config.py, PATTERNS.md
Registers P35 and documents its matching rule with a Java example.

SPDX header normalization

Layer / File(s) Summary
SPDX comment formatting
aibolit/metrics/*/*.xml, aibolit/metrics/*/pom.xml, scripts/ruleset.xml
Normalizes leading whitespace in SPDX header metadata comments.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • cqfn/aibolit#832: Refactors pattern APIs and tests to use AST-based value(...) calls.
  • cqfn/aibolit#1318: Applies similar SPDX header formatting changes in scripts/ruleset.xml.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: implementing the non-final argument pattern.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Checkov (3.3.8)
aibolit/metrics/halsteadvolume/pom.xml

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

aibolit/metrics/npath/pom.xml

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

🔧 PMD (7.26.0)
aibolit/metrics/halsteadvolume/pom.xml

openjdk version "17.0.19" 2026-04-21 LTS
OpenJDK Runtime Environment Corretto-17.0.19.10.1 (build 17.0.19+10-LTS)
OpenJDK 64-Bit Server VM Corretto-17.0.19.10.1 (build 17.0.19+10-LTS, mixed mode, sharing)
[ERROR] Error at scripts/ruleset.xml:23:36
21|
22|
23|
^^^^^ Value should be 'countImports'

24|
25| : '|' should be 'countImports'
[ERROR] Error at scripts/ruleset.xml:19:3
17|
18|
19|
^^^^^ Error while parsing rule reference

20|
21| : Error at scripts/ruleset.xml:23:36
21|
22|
23|
^^^^^ Value should be 'countImports'

24|
25|
[ERROR] Cannot load ruleset scripts/ruleset.xml: 2 XML validation errors occurred
[WARN] Progressbar rendering conflicts with reporting to STDOUT. No progressbar will be shown. Try running with argument -r to output the report to a file instead.

aibolit/metrics/npath/pom.xml

openjdk version "17.0.19" 2026-04-21 LTS
OpenJDK Runtime Environment Corretto-17.0.19.10.1 (build 17.0.19+10-LTS)
OpenJDK 64-Bit Server VM Corretto-17.0.19.10.1 (build 17.0.19+10-LTS, mixed mode, sharing)
[ERROR] Error at scripts/ruleset.xml:23:36
21|
22|
23|
^^^^^ Value should be 'countImports'

24|
25| : '|' should be 'countImports'
[ERROR] Error at scripts/ruleset.xml:19:3
17|
18|
19|
^^^^^ Error while parsing rule reference

20|
21| : Error at scripts/ruleset.xml:23:36
21|
22|
23|
^^^^^ Value should be 'countImports'

24|
25|
[ERROR] Cannot load ruleset scripts/ruleset.xml: 2 XML validation errors occurred
[WARN] Progressbar rendering conflicts with reporting to STDOUT. No progressbar will be shown. Try running with argument -r to output the report to a file instead.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ogluvu-ship-it

Copy link
Copy Markdown
Contributor Author

@yegor256 awaiting approval

@ogluvu-ship-it

Copy link
Copy Markdown
Contributor Author

@yegor256 awaiting approval

@yegor256

Copy link
Copy Markdown
Member

@ogluvu-ship-it build is broken

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/patterns/non_final_argument/test_non_final_argument.py (1)

16-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for de-duplication and final-only declarations.

The current fixtures each contain only one offending parameter. Add cases proving that multiple non-final parameters produce one line per declaration, and that methods/constructors whose parameters are all final are ignored.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/patterns/non_final_argument/test_non_final_argument.py` around lines 16
- 24, Extend the NonFinalArgument tests with fixtures containing multiple
non-final parameters and assert one reported line per declaration, then add
constructor and method fixtures where every parameter is final and assert no
results. Update the relevant test methods near
test_find_non_final_argument_in_ctor and test_find_non_final_argument_in_method
while preserving the existing single-violation coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@PATTERNS.md`:
- Line 646: Standardize the pattern name to “Non-final argument” in both
PATTERNS.md:646-646 and aibolit/config.py:204-204, updating the documentation
title and the registry display name consistently.

---

Nitpick comments:
In `@test/patterns/non_final_argument/test_non_final_argument.py`:
- Around line 16-24: Extend the NonFinalArgument tests with fixtures containing
multiple non-final parameters and assert one reported line per declaration, then
add constructor and method fixtures where every parameter is final and assert no
results. Update the relevant test methods near
test_find_non_final_argument_in_ctor and test_find_non_final_argument_in_method
while preserving the existing single-violation coverage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a18d919-8860-468e-8b05-4693a0848979

📥 Commits

Reviewing files that changed from the base of the PR and between be4499d and 0983a83.

⛔ Files ignored due to path filters (1)
  • test/stats/results_test.csv is excluded by !**/*.csv
📒 Files selected for processing (8)
  • PATTERNS.md
  • aibolit/config.py
  • aibolit/metrics/halsteadvolume/pom.xml
  • aibolit/metrics/npath/npath.xml
  • aibolit/metrics/npath/pom.xml
  • aibolit/patterns/non_final_argument/non_final_argument.py
  • scripts/ruleset.xml
  • test/patterns/non_final_argument/test_non_final_argument.py

Comment thread PATTERNS.md

***

*Title*: Non final argument

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use Non-final argument consistently in the registry and documentation.

  • PATTERNS.md#L646-L646: change the title to Non-final argument.
  • aibolit/config.py#L204-L204: change the display name to Non-final argument.
🧰 Tools
🪛 LanguageTool

[grammar] ~646-~646: Use a hyphen to join words.
Context: ...tList { // here } ``` *** Title: Non final argument Code: P35 *Descri...

(QB_NEW_EN_HYPHEN)

📍 Affects 2 files
  • PATTERNS.md#L646-L646 (this comment)
  • aibolit/config.py#L204-L204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@PATTERNS.md` at line 646, Standardize the pattern name to “Non-final
argument” in both PATTERNS.md:646-646 and aibolit/config.py:204-204, updating
the documentation title and the registry display name consistently.

Source: Linters/SAST tools

@ogluvu-ship-it

Copy link
Copy Markdown
Contributor Author

@yegor256 awaiting merge

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

Successfully merging this pull request may close these issues.

3 participants