Closed
Conversation
61bd1b6 to
770258c
Compare
Owner
|
Closing — the per-cop byte scanner approach is too fragile and doesn't scale. The root cause (Prism parses files that RuboCop's Parser gem rejects as syntax errors) affects all cops equally. PR #153 for Lint/HashCompareByIdentity hit the same issue and correctly documented it rather than adding a per-cop workaround. The right fix is either:
The 70-line byte scanner for specific regex escape patterns would also false-positive on those patterns appearing in strings or comments. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated fix fix for
Lint/UnusedMethodArgumentcorpus conformance.Details
Lint/UnusedMethodArgumentResult
Changes
Agent conversation excerpt
Reading
docs/agent-ci.mdfirst as requested, then I’ll verify RuboCop behavior on the two FP patterns before touching fixtures or cop logic.Reading
docs/agent-ci.mdfirst as requested, then I’ll verify RuboCop behavior on the two FP patterns before touching fixtures or cop logic.I’ve finished the CI rules read. Next I’m running RuboCop directly on the reported FP shape and a broader control case to pin down the exact context RuboCop exempts.
I’ve finished the CI rules read. Next I’m running RuboCop directly on the reported FP shape and a broader control case to pin down the exact context RuboCop exempts.
RuboCop flags the extracted
yaml_new(..., tag, ...)snippet as an offense in my local run, so I’m now checking full corpus context with the helper script to find what suppresses RuboCop there (likely config/disable context).RuboCop flags the extracted
yaml_new(..., tag, ...)snippet as an offense in my local run, so I’m now checking full corpus context with the helper script to find what suppresses RuboCop there (likely config/disable context).The default investigation output only covers the standard corpus (which is already perfect), so I’m checking the script options to pull the extended divergence context you cited.
The default investigation output only covers the standard corpus (which is already perfect), so I’m checking the script options to pull the extended divergence context you cited.
I confirmed extended divergence data, and both FP paths are under vendored directories. I’m now verifying RuboCop on the
setup_parser(..., debug=false)shape as a second control check.I confirmed extended divergence data, and both FP paths are under vendored directories. I’m now verifying RuboCop on the
setup_parser(..., debug=false)shape as a second control check.Both extracted snippets are offenses in isolation under RuboCop, which strongly suggests these two corpus mismatches are contextual (likely parse/config/disable behavior in full files). I’m checking repo-level investigation output next to identify that context before deciding whether a cop logic change is valid.
Both extracted snippets are offenses in isolation under RuboCop, which strongly suggests these two corpus mismatches are contextual (likely parse/config/disable behavior in full files). I’m checking repo-level investigation output next to identify that context before deciding whether a cop logic change is valid.
I’m digging into whether the mismatch is truly cop logic or corpus context noise; next I’ll sanity-check RuboCop on plain singleton-method arguments to avoid introducing a broad suppression that RuboCop still flags.
I’m digging into whether the mismatch is truly cop logic or corpus context noise; next I’ll sanity-check RuboCop on plain singleton-method arguments to avoid introducing a broad suppression that RuboCop still flags.
I’ve confirmed RuboCop still flags the broader singleton-method pattern, so a suppression in cop logic would be too broad. I’m validating whether path-level exclusion (
vendor/**) is the differentiator for these two corpus paths.I’ve confirmed RuboCop still flags the broader singleton-method pattern, so a suppression in cop logic would be too broad. I’m validating whether path-level exclusion (
vendor/**) is the differentiator for these two corpus paths.The FP shape aligns with RuboCop’s default
vendor/**exclusion behavior rather than a cop semantic gap. I’m pulling the exact file content at the reported commits to confirm whether these locations are under exclusion/disable context in the real repos.The FP shape aligns with RuboCop’s default
vendor/**exclusion behavior rather than a cop semantic gap. I’m pulling the exact file content at the reported commits to confirm whether these locations are under exclusion/disable context in the real repos.Both repos cloned successfully. I’m checking out the exact commits from the corpus IDs and inspecting the target files for any RuboCop exclusion/disable context.
Both repos cloned successfully. I’m checking out the exact commits from the corpus IDs and inspecting the target files for any RuboCop exclusion/disable context.
I inspected both files directly: no inline
rubocop:disablearound the methods. Next I’m reproducing RuboCop behavior on those exact repo files using both explicit-file mode and project-scan mode to confirm exclusion semantics.I inspected both files directly: no inline
rubocop:disablearound the methods. Next I’m reproducing RuboCop behavior on those exact repo files using both explicit-file mode and project-scan mode to confirm exclusion semantics.I found the likely differentiator: both FP files contain Ruby syntax that RuboCop treats as parse errors, so RuboCop doesn’t emit
UnusedMethodArgumentthere while nitrocop still does. I’m now adding a fixture that captures “syntax-error file should not emit this cop” and then guarding the cop accordingly.I found the likely differentiator: both FP files contain Ruby syntax that RuboCop treats as parse errors, so RuboCop doesn’t emit
UnusedMethodArgumentthere while nitrocop still does. I’m now adding a fixture that captures “syntax-error file should not emit this cop” and then guarding the cop accordingly.I’m now validating whether Prism exposes these parser-incompatibility cases as
errorsorwarnings; that determines whether a safe guard can be implemented in this cop without broad suppression.I’m now validating whether Prism exposes these parser-incompatibility cases as
errorsorwarnings; that determines whether a safe guard can be implemented in this cop without broad suppression._tmp = scan(/\A(?-mix:[0-9A-Za-z\200-\377])/)
RUBY
cargo run --quiet -- --format json `
I kicked off a direct nitrocop run on a minimal reproducer containing the old regex escape pattern; once it returns, I’ll know if this cop currently fires despite parser incompatibility.
I kicked off a direct nitrocop run on a minimal reproducer containing the old regex escape pattern; once it returns, I’ll know if this cop currently fires despite parser incompatibility.
Task prompt (10420 tokens)
Fix Lint/UnusedMethodArgument — 2 FP, 0 FN
Cross-Corpus Risk
Risk class: extended-only edge case against a standard-perfect baseline.
Treat the extended examples as a narrow edge case, not proof that the broad pattern is safe.
Any carve-out that reduces the extended FP/FN count but regresses the standard corpus is a bad fix.
Prefer the smallest context-specific change you can justify from RuboCop behavior and the vendor spec.
This is concentrated in only a few repos, which increases the risk of overfitting to one local shape.
Instructions
You are fixing ONE cop in nitrocop, a Rust Ruby linter that uses Prism for parsing.
Current state: 29,967 matches, 2 false positives, 0 false negatives.
Focus on: FP (nitrocop flags code RuboCop does not).
Workflow
behavior on BOTH the specific FP case AND the general pattern:
tests/fixtures/cops/lint/unused_method_argument/offense.rbwith^annotationtests/fixtures/cops/lint/unused_method_argument/no_offense.rbcargo test --lib -- cop::lint::unused_method_argumentsrc/cop/lint/unused_method_argument.rscargo test --lib -- cop::lint::unused_method_argument///doc comment on the cop struct documenting what you found and fixedFixture Format
Mark offenses with
^markers on the line AFTER the offending source line:The
^characters must align with the offending columns. The message format isLint/UnusedMethodArgument: <message text>.Mixed issues: some code bugs, some config issues
Pre-diagnostic shows SOME patterns are correctly detected in isolation (config issues)
and SOME are genuinely missed (code bugs). See the per-example diagnosis below.
If your test passes immediately
If you add a test case and it passes without code changes, the corpus mismatch is
caused by config/context differences, not a detection bug.
Do NOT loop trying to make the test fail. Instead:
src/config/or the cop's config handling, not detection logica
///comment on the cop struct and commitCRITICAL: Avoid regressions in the opposite direction
When fixing FPs, your change MUST NOT suppress legitimate detections. When fixing FNs,
your change MUST NOT flag code that RuboCop accepts. A fix that eliminates a few issues
in one direction but introduces hundreds in the other is a catastrophic regression.
Before exempting a category of patterns, verify with RuboCop that the general case
is still an offense:
If RuboCop flags the general pattern but not your specific case, the difference is in
a narrow context (e.g., enclosing structure, receiver type, argument count) — your fix
must target that specific context, not the broad category.
Rule of thumb: if your fix adds an early
returnorcontinuethat skips a wholenode type, operator class, or naming pattern, it's probably too broad. Prefer adding a
condition that matches the SPECIFIC differentiating context.
Rules
src/cop/lint/unused_method_argument.rsandtests/fixtures/cops/lint/unused_method_argument/cargo test --lib -- cop::lint::unused_method_argumentto verify your fix (do NOT run the full test suite)git stashPrism Notes
Foo) and ConstantPathNode (qualifiedFoo::Bar). If you handle one, check if you need the other.Start Here
Use the existing corpus data to focus on the most concentrated regressions first.
Helpful local commands:
python3 scripts/investigate-cop.py Lint/UnusedMethodArgument --repos-onlypython3 scripts/investigate-cop.py Lint/UnusedMethodArgument --contextpython3 scripts/verify-cop-locations.py Lint/UnusedMethodArgumentTop FP repos:
liaoziyang__stackneveroverflow__8f4dce2(1 FP) — examplevendor/bundle/ruby/2.3.0/gems/rdoc-4.3.0/lib/rdoc/markdown/literals_1_8.rb:21pitluga__supply_drop__d64c50c(1 FP) — exampleexamples/vendored-puppet/vendor/puppet-2.7.8/lib/puppet/util/zaml.rb:208Representative FP examples:
liaoziyang__stackneveroverflow__8f4dce2: vendor/bundle/ruby/2.3.0/gems/rdoc-4.3.0/lib/rdoc/markdown/literals_1_8.rb:21— Unused method argument -debug.pitluga__supply_drop__d64c50c: examples/vendored-puppet/vendor/puppet-2.7.8/lib/puppet/util/zaml.rb:208— Unused method argument -tag.Pre-diagnostic Results
Diagnosis Summary
Each example was tested by running nitrocop on the extracted source in isolation
with
--force-default-configto determine if the issue is a code bug or config issue.Note: source context is truncated and may not parse perfectly. If a diagnosis
seems wrong (e.g., your test passes immediately for a 'CODE BUG'), treat it as
a config/context issue instead.
FP #1:
liaoziyang__stackneveroverflow__8f4dce2: vendor/bundle/ruby/2.3.0/gems/rdoc-4.3.0/lib/rdoc/markdown/literals_1_8.rb:21NOT REPRODUCED in isolation — CONTEXT-DEPENDENT
nitrocop does not flag this in isolation. The FP is triggered
by surrounding code context or file-level state.
Investigate what full-file context causes the false detection.
Source context:
Message:
Unused method argument -debug.FP #2:
pitluga__supply_drop__d64c50c: examples/vendored-puppet/vendor/puppet-2.7.8/lib/puppet/util/zaml.rb:208CONFIRMED false positive — CODE BUG
nitrocop incorrectly flags this pattern in isolation.
Fix the detection logic to not flag this.
Enclosing structure: if branch (line:
if respond_to? :yaml_new and yaml_new(self, :tag, "message" => "blurp").message != "blurp")The offense is inside this structure — this is likely WHY
RuboCop does not flag it. Your fix should detect this context.
Full source context (add relevant parts to no_offense.rb):
Message:
Unused method argument -tag.Current Rust Implementation
src/cop/lint/unused_method_argument.rsRuboCop Ruby Implementation (ground truth)
vendor/rubocop/lib/rubocop/cop/lint/unused_method_argument.rbRuboCop Test Excerpts
vendor/rubocop/spec/rubocop/cop/lint/unused_method_argument_spec.rbCurrent Fixture: offense.rb
tests/fixtures/cops/lint/unused_method_argument/offense.rbCurrent Fixture: no_offense.rb
tests/fixtures/cops/lint/unused_method_argument/no_offense.rb