#841: fix NPE in DefectMissing when +unlint range references absent lint#843
Conversation
…e references absent lint A range-pattern +unlint (e.g. name:1-5) whose lint never fired causes a NullPointerException in DefectMissing because this.defects.get(name) returns null and no null-check is performed before lines.stream(). These tests cover both the unit level (DefectMissing) and the integration level (LtUnlintNonExistingDefect) paths and currently fail with NPE.
DefectMissing.apply threw NullPointerException when +unlint used the range pattern name:line-line for a lint whose defects never fired. In that case this.defects.get(name) returns null and the range branch dereferenced it without a guard. When lines is null, treat the defect as missing unless the name is in the excluded set, matching the behaviour of the non-range branch for the same situation. Also defends the single-line branch against a null lines list for consistency.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA null-pointer fix: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/java/org/eolang/lints/LtUnlintNonExistingDefectTest.java (1)
226-246: Optional: also assert the reported defect identifies the missing lint.The size check is enough to guard against the NPE regression, but asserting the single defect's
rule()(or line) would pin down the semantic contract ("range+unlintfor absent lint produces one non-existing-defect report") and prevent silent drift if the logic changes later.Example additional assertion
final var defects = new LtUnlintNonExistingDefect( new ListOf<>(new LtAsciiOnly()), new ListOf<>() ).defects( new EoSyntax( String.join( "\n", "+unlint ascii-only:1-5", "[] > main", " QQ.io.stdout > @", " \"Hello\"" ) ).parsed() ); MatcherAssert.assertThat(defects, Matchers.iterableWithSize(1)); MatcherAssert.assertThat( defects.iterator().next().rule(), Matchers.equalTo("unlint-non-existing-defect") );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/eolang/lints/LtUnlintNonExistingDefectTest.java` around lines 226 - 246, Update the test catchesUnlintWithRangeForAbsentLint to also assert the reported defect identifies the missing lint: call LtUnlintNonExistingDefect.defects(...) into a local variable (e.g., defects), keep the existing iterableWithSize(1) check, then assert defects.iterator().next().rule() equals the expected rule string (e.g., "unlint-non-existing-defect"); reference the LtUnlintNonExistingDefect class, the defects() method and the defect.rule() accessor to locate where to add the assertion.src/main/java/org/eolang/lints/DefectMissing.java (1)
59-63: Minor:lines == nullcheck here is logically redundant.When
lines == null,nameis absent fromthis.defects, so!names.contains(name)is alreadytrueand short-circuits the disjunction. The added|| lines == nullclause is harmless but dead. It's fine to keep it defensively for symmetry with the range branch and readability, but if you prefer minimal diffs you can drop it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/eolang/lints/DefectMissing.java` around lines 59 - 63, The condition computing missing in DefectMissing.java contains a redundant "|| lines == null" clause: when lines is null, !names.contains(name) is already true, so remove the dead check from the branch that handles split.length > 1; update the assignment to missing to use (!names.contains(name) || !lines.contains(Integer.parseInt(split[1]))) && !this.excluded.contains(name) so the logic remains identical but without the redundant null test in the missing computation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/eolang/lints/DefectMissing.java`:
- Around line 59-63: The condition computing missing in DefectMissing.java
contains a redundant "|| lines == null" clause: when lines is null,
!names.contains(name) is already true, so remove the dead check from the branch
that handles split.length > 1; update the assignment to missing to use
(!names.contains(name) || !lines.contains(Integer.parseInt(split[1]))) &&
!this.excluded.contains(name) so the logic remains identical but without the
redundant null test in the missing computation.
In `@src/test/java/org/eolang/lints/LtUnlintNonExistingDefectTest.java`:
- Around line 226-246: Update the test catchesUnlintWithRangeForAbsentLint to
also assert the reported defect identifies the missing lint: call
LtUnlintNonExistingDefect.defects(...) into a local variable (e.g., defects),
keep the existing iterableWithSize(1) check, then assert
defects.iterator().next().rule() equals the expected rule string (e.g.,
"unlint-non-existing-defect"); reference the LtUnlintNonExistingDefect class,
the defects() method and the defect.rule() accessor to locate where to add the
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb4b37db-f3ea-4478-ac0c-7d25f177761e
📒 Files selected for processing (3)
src/main/java/org/eolang/lints/DefectMissing.javasrc/test/java/org/eolang/lints/DefectMissingTest.javasrc/test/java/org/eolang/lints/LtUnlintNonExistingDefectTest.java
objectionary/home restructured on 2026-04-20 so that .eo files now live under objects/ directly (e.g. objects/io/stdout.eo) instead of the old objects/org/eolang/... nesting, and object paths no longer use the org.eolang. prefix. - HomeNames.placeCsv: widen the source filter to include any .eo file under objects/, not only under objects/org/eolang/. - Tests: update the path-format expectations to drop the org.eolang. prefix (e.g. stdout is now reserved by "io.stdout.eo" rather than "org.eolang.io.stdout.eo").
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/org/eolang/lints/ReservedNamesTest.java (1)
38-38: Duplicate regex literal across two test files.The identical pattern
(?:[a-zA-Z_][a-zA-Z0-9_-]*\.)*[a-zA-Z_][a-zA-Z0-9_-]*\.eois now hard-coded in bothHomeNamesTest.groovy(Line 51) and here. Consider extracting it to a shared constant (e.g., a package-private constant in a test-support class) to keep both tests in sync when the reserved-name format evolves again.Also, as noted on the Groovy test, this regex change appears unrelated to the
#841NPE fix this PR claims to address — consider moving it to a dedicated PR covering theobjectionary/homerestructure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/eolang/lints/ReservedNamesTest.java` at line 38, ReservedNamesTest currently duplicates the regex literal used in HomeNamesTest; extract the pattern into a shared test constant and reference it from both tests to avoid drift. Create a package-private constant (e.g., TEST_EO_FILENAME_REGEX) in a common test-support class (or a dedicated TestConstants class) and replace the inline regex in ReservedNamesTest and HomeNamesTest with that constant; update imports/usages to reference TestConstants.TEST_EO_FILENAME_REGEX and keep the tests otherwise unchanged. Ensure the constant is named clearly and located in a test-only package so production code is unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/java/org/eolang/lints/ReservedNamesTest.java`:
- Line 38: ReservedNamesTest currently duplicates the regex literal used in
HomeNamesTest; extract the pattern into a shared test constant and reference it
from both tests to avoid drift. Create a package-private constant (e.g.,
TEST_EO_FILENAME_REGEX) in a common test-support class (or a dedicated
TestConstants class) and replace the inline regex in ReservedNamesTest and
HomeNamesTest with that constant; update imports/usages to reference
TestConstants.TEST_EO_FILENAME_REGEX and keep the tests otherwise unchanged.
Ensure the constant is named clearly and located in a test-only package so
production code is unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28ae5027-2683-46d9-881f-331031fdbb76
📒 Files selected for processing (4)
src/main/groovy/org/eolang/lints/HomeNames.groovysrc/test/groovy/org/eolang/lints/HomeNamesTest.groovysrc/test/java/org/eolang/lints/LtReservedNameTest.javasrc/test/java/org/eolang/lints/ReservedNamesTest.java
💤 Files with no reviewable changes (1)
- src/main/groovy/org/eolang/lints/HomeNames.groovy
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/eolang/lints/LtReservedNameTest.java
…xtures The objectionary/home restructure dropped the org.eolang. prefix from object paths; the hand-crafted MapOf<> fixtures in LtReservedNameTest still carried the old "org.eolang.<name>.eo" strings and the expected defect message in reportsCorrectMessageForReservedNameInTopObject was hard-coded with the same prefix. Update them to match the new layout so the fixtures reflect what the live scan actually produces.
|
@bibonix Hey! Nice work on that contribution – you've bagged +16 points 🎉 The bonus calculation kicked in perfectly: you got the base 16 points for having reviews, plus the hit-of-code bonus, and avoided any deductions since you stayed within the sweet spot of our policy guidelines. Keep them coming! Your running score is +16 – don't forget to check your Zerocracy account too. |
4 participants
@yegor256 this fixes #841.
Problem.
DefectMissing.applythrewNullPointerExceptionwhen an EO file used+unlint name:line-line(range pattern) andnamereferred to a lint that never fired.this.defects.get(name)returnednullin that case and the range branch dereferenced it vialines.stream()without a guard. The same class powers bothLtUnlintNonExistingDefectandLtUnlintNonExistingDefectWpa, so both lints crashed instead of reporting the obviously wrong unlint.Fix. In the range branch of
DefectMissing.apply, whenlines == null, treat the defect as missing unless the lint name is in theexcludedset — mirroring what the non-range branch does in the same situation. I also added a defensive null check in the single-line branch so the two branches behave symmetrically; no existing code path depended on the previous behaviour there.Tests (
d35b9ed4, added before the fix, failing with the reported NPE).DefectMissingTest#returnsTrueWhenRangeReferencesAbsentLint— unit-level reproduction.DefectMissingTest#returnsFalseWhenRangeReferencesExcludedAbsentLint— covers theexcludedinteraction.LtUnlintNonExistingDefectTest#catchesUnlintWithRangeForAbsentLint— the exact reproducer from the issue.All three failed with the reported NPE before the fix and pass after it. The full local
mvn testis green (639 tests, 0 failures). Themvnmatrix job on CI is green on Ubuntu, Windows, and macOS; Qulice is also green.Note on the
reservedcheck. It is red on this PR and also on the other currently open PRs #838, #839, #840, #842. Locally I reproduced the same 3 failures + 1 error, all inHomeNamesTest/ReservedNamesTest/LtReservedNameTest, unrelated to this change. They are caused by the recent restructure ofobjectionary/home(2026-04-20, "Move objectionary files to objects directory"):.eofiles now live directly underobjects/instead ofobjects/org/eolang/, soHomeNames.placeCsv's path filterobjects/org/eolangno longer matches anything and the resultingreserved.csvis empty. That is a separate issue in this repo'sreservedprofile — it was already failing before this PR was opened and is out of scope here.Summary by CodeRabbit
Bug Fixes
Improvements
Tests