Open
Conversation
a35f492 to
0c01ce7
Compare
Contributor
|
✅ Cop-check results (8 shards)
All shards passed — ready to merge. |
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
Style/StringHashKeyscorpus conformance.Refs #460
Details
Style/StringHashKeysResult
Changes
Agent conversation excerpt
Reading
docs/agent-ci.mdfirst, then I’ll inspect the current cop, RuboCop’s implementation, and the existing fixtures to pin down the two false negatives before editing anything.Reading
docs/agent-ci.mdfirst, then I’ll inspect the current cop, RuboCop’s implementation, and the existing fixtures to pin down the two false negatives before editing anything.I have the current implementation and the two failing shapes. Next I’m checking how Prism parses
__FILE__ => ...in both contexts, because these FNs usually mean the cop is only looking at one node kind or one traversal path.I have the current implementation and the two failing shapes. Next I’m checking how Prism parses
__FILE__ => ...in both contexts, because these FNs usually mean the cop is only looking at one node kind or one traversal path.The fixtures already contain the two missed cases, which is useful. I’m drilling into the visitor and Prism node shapes now so I can make the smallest possible detection change and keep the existing 688k matches stable.
The fixtures already contain the two missed cases, which is useful. I’m drilling into the visitor and Prism node shapes now so I can make the smallest possible detection change and keep the existing 688k matches stable.
The targeted test run is in progress. In parallel I’m preparing to inspect the AST shape for
__FILE__, because the likely gap is that Prism represents it differently from quoted strings and the visitor only handlesStringNode.The targeted test run is in progress. In parallel I’m preparing to inspect the AST shape for
__FILE__, because the likely gap is that Prism represents it differently from quoted strings and the visitor only handlesStringNode.The failure is confirmed and isolated: the fixture expects 12 offenses, the cop only emits 10, exactly missing the two
__FILE__keys. I’m checking the generated Prism node API next so I can patch this without broadening the matcher beyond RuboCop’s behavior.The failure is confirmed and isolated: the fixture expects 12 offenses, the cop only emits 10, exactly missing the two
__FILE__keys. I’m checking the generated Prism node API next so I can patch this without broadening the matcher beyond RuboCop’s behavior.Plan:
__FILE__hash keys and isolate the Prism node type involved.src/cop/style/string_hash_keys.rsto treat that node like RuboCop does, while preserving the existing narrow exemptions.scripts/check_cop.py.Plan:
__FILE__hash keys and isolate the Prism node type involved.src/cop/style/string_hash_keys.rsto treat that node like RuboCop does, while preserving the existing narrow exemptions.scripts/check_cop.py.I’ve narrowed the gap to
SourceFileNodehandling. Before editing, I’m doing a quick RuboCop sanity check on the exact__FILE__patterns so the patch stays aligned with the source of truth.I’ve narrowed the gap to
SourceFileNodehandling. Before editing, I’m doing a quick RuboCop sanity check on the exact__FILE__patterns so the patch stays aligned with the source of truth.One thing surfaced from the RuboCop check: the standalone
__FILE__ => [...]snippet isn’t valid Ruby on its own. I’m going to tighten the fixture to a valid minimal reproduction of the real corpus case before patching the cop, so the test exercises the actual AST shape instead of an error-recovery parse.One thing surfaced from the RuboCop check: the standalone
__FILE__ => [...]snippet isn’t valid Ruby on its own. I’m going to tighten the fixture to a valid minimal reproduction of the real corpus case before patching the cop, so the test exercises the actual AST shape instead of an error-recovery parse.I’m correcting the new fixture lines themselves too. Both were added with placeholder
^markers at column 0, so they won’t validate the real source locations until those columns are fixed.I’m correcting the new fixture lines themselves too. Both were added with placeholder
^markers at column 0, so they won’t validate the real source locations until those columns are fixed.I’m editing two places now: the offense fixture to use valid, correctly annotated
__FILE__reproductions, and the cop to treat PrismSourceFileNodekeys like RuboCop’s string-key handling.I’m editing two places now: the offense fixture to use valid, correctly annotated
__FILE__reproductions, and the cop to treat PrismSourceFileNodekeys like RuboCop’s string-key handling.The patch is in place. I’m running the targeted cop test now to confirm the two
__FILE__cases are detected and that the existing exemptions still hold.Task prompt (3482 tokens)
Fix Style/StringHashKeys — 0 FP, 2 FN
Instructions
You are fixing ONE cop in nitrocop, a Rust Ruby linter that uses Prism for parsing.
Current state: 688,706 matches, 0 false positives, 2 false negatives.
Focus on: FN (RuboCop flags code nitrocop misses).
⚠ 688,706 existing matches must not regress. Validate with
check_cop.pybefore committing.Workflow
behavior on BOTH the specific FP case AND the general pattern:
tests/fixtures/cops/style/string_hash_keys/offense.rbwith^annotationtests/fixtures/cops/style/string_hash_keys/no_offense.rbcargo test --lib -- cop::style::string_hash_keyssrc/cop/style/string_hash_keys.rscargo test --lib -- cop::style::string_hash_keys///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 isStyle/StringHashKeys: <message text>.See the Current Fixture sections below for real examples from this cop.
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/style/string_hash_keys.rsandtests/fixtures/cops/style/string_hash_keys/cargo test --lib -- cop::style::string_hash_keysto verify your fix (do NOT run the full test suite)python3 scripts/check_cop.py Style/StringHashKeys --rerun --clone --sample 15before committing to catch regressionsgit stashPrism Notes
{}) and KeywordHashNode (keyword argsfoo(a: 1)). If you handle one, check if you need the other.Foo) and ConstantPathNode (qualifiedFoo::Bar). If you handle one, check if you need the other.Current Fixture: offense.rb
tests/fixtures/cops/style/string_hash_keys/offense.rbCurrent Fixture: no_offense.rb
tests/fixtures/cops/style/string_hash_keys/no_offense.rbKey Source Files
src/cop/style/string_hash_keys.rsvendor/rubocop/lib/rubocop/cop/style/string_hash_keys.rbvendor/rubocop/spec/rubocop/cop/style/string_hash_keys_spec.rbRead these files before making changes.
Start Here
Use the existing corpus data to focus on the most concentrated regressions first.
Helpful local commands:
python3 scripts/investigate_cop.py Style/StringHashKeys --repos-onlypython3 scripts/investigate_cop.py Style/StringHashKeys --contextpython3 scripts/verify_cop_locations.py Style/StringHashKeysTop FN repos:
archan937__motion-bundler__9cbfc90(1 FN) — exampletest/unit/require/test_tracer.rb:81simplecov-ruby__simplecov__522dc7d(1 FN) — examplespec/simplecov_spec.rb:35Representative FN examples:
archan937__motion-bundler__9cbfc90: test/unit/require/test_tracer.rb:81— Prefer symbols instead of strings as hash keys.simplecov-ruby__simplecov__522dc7d: spec/simplecov_spec.rb:35— Prefer symbols instead of strings as hash keys.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.
FN #1:
archan937__motion-bundler__9cbfc90: test/unit/require/test_tracer.rb:81NOT DETECTED — CODE BUG
The cop fails to detect this pattern. Fix the detection logic.
Full-file enclosing chain: block ({..}) (line 80:
assert_equal({) > block (do..end) (line 71:it "should log dependencies as expected" do) > block (do..end) (line 70:describe "tracing require statements" do) > block (do..end) (line 7:describe MotionBundler::Require::Tracer do) > class body (line 5:class TestTracer < MiniTest::Unit::TestCase) > module body (line 4:module Require) > module body (line 3:module Unit)Message:
Prefer symbols instead of strings as hash keys.Ready-made test snippet (add to offense.rb, adjust
^count):Full file context (30 lines before offense):
FN #2:
simplecov-ruby__simplecov__522dc7d: spec/simplecov_spec.rb:35NOT DETECTED — CODE BUG
The cop fails to detect this pattern. Fix the detection logic.
Full-file enclosing chain: block (do..end) (line 34:
it "uses the result from Coverage" do) > block (do..end) (line 29:context "when running" do) > block (do..end) (line 13:context "with merging disabled" do) > block (do..end) (line 7:describe ".result" do) > block (do..end) (line 6:describe SimpleCov do)Message:
Prefer symbols instead of strings as hash keys.Ready-made test snippet (add to offense.rb, adjust
^count):Full file context (30 lines before offense):