Don't warn about unused destructured block/method parameters (GH-9034)#9400
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes GH-9034 by aligning JRuby’s unused-variable warning behavior with CRuby for names introduced via destructured block/method parameters, preventing spurious $VERBOSE “assigned but unused variable” warnings.
Changes:
- Mark variables introduced via
assignableInCurr(used for formal/destructured args) as “used” when unused-variable warnings are enabled. - Add a regression spec ensuring no “assigned but unused variable” warnings for inner destructured block and method parameters.
- Add
maven/jruby-jars/.polyglot.Mavenfile(appears to be a generated/derived Maven representation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/regression/GH-9034_unused_variable_warning_for_destructured_block_args_spec.rb | Adds regression coverage to ensure destructured arg locals don’t trigger unused-variable warnings under $VERBOSE. |
| core/src/main/java/org/jruby/parser/RubyParserBase.java | Updates parser bookkeeping to treat formal args introduced via assignableInCurr as “used,” suppressing incorrect warnings. |
| maven/jruby-jars/.polyglot.Mavenfile | Introduces an additional Maven config file that appears derived/generated and risks drifting from the canonical Mavenfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <groupId>org.jruby</groupId> | ||
| <artifactId>jruby-artifacts</artifactId> | ||
| <version>10.1.1.0-SNAPSHOT</version> | ||
| </parent> | ||
| <groupId>rubygems</groupId> | ||
| <artifactId>jruby-jars</artifactId> | ||
| <packaging>gem</packaging> |
There was a problem hiding this comment.
This newly-added .polyglot.Mavenfile looks like a generated/derived Maven POM snapshot of maven/jruby-jars/Mavenfile (it hardcodes the current snapshot version/finalName, duplicates plugin/dependency config, and would easily drift). Unless there’s a build/tooling requirement to commit it, it should be removed from the repo and added to .gitignore to avoid stale configuration affecting future builds.
There was a problem hiding this comment.
Committed by accident, removed it.
c905ee6 to
e86b448
Compare
enebo
left a comment
enebo
left a comment
There was a problem hiding this comment.
Thanks a lot for fixing this. We appreciate your contribution to JRuby.
I approve the fix but added in a comment that I would like the regression spec (and technically this is not a regression) to be moved to ruby/spec project since it would not just benefit us but any other Ruby project (technically this is optional since MRI tests cover thiss).
The second thing is to ask if you can squash this to remove the accidental mavenfile stuff.
| @@ -0,0 +1,53 @@ | |||
| require 'tempfile' | |||
There was a problem hiding this comment.
This is not a regression. Would it be possible for you to submit this to ruby/spec project? You could also update this PR to update our copy that resides in spec/ruby but it may be less friction just to submit against the project directly.
|
@sampokuokkanen Another thing is we have test/mri/ruby/test_parser.rb tagging out the test code you reference for our MRI test suite run. If you remove |
|
Thank you for giving JRuby a try and submitting a fix! I'm glad you enjoyed the talk! It seems like @enebo has this in hand so I'll let him handle further reviews. Welcome to the JRuby community! |
Fixes jrubyGH-9034: prevent spurious "assigned but unused variable" warnings for names introduced via destructured block/method parameters. In RubyParserBase.assignableInCurr, when warnOnUnusedVariables is enabled the code now calls scopedParserState.markUsedVariable(name, 0) after adding the defined variable so inner destructured names are treated as used. Re-enabled MRI test_unused_variable.
e86b448 to
c489554
Compare
|
@enebo The commits should now also be squashed. I removed the test case I created and will create a PR to ruby/spec. |
enebo
left a comment
enebo
left a comment
There was a problem hiding this comment.
Excellent. I will merge this. Glad to have your help solving this problem.
Hi! I started looking into JRuby again after @headius' Hakodate RubyKaigi talk. Great talk btw!
This fixes GH-9034: prevent spurious "assigned but unused variable" warnings for names introduced via destructured block/method parameters. Added a spec and confirmed it failed and that it passes after the fix.
In CRuby the behavior is tested here:
https://github.com/ruby/ruby/blob/8d929853d88b5ca4ff72b78b5b539afc1b8b7d71/test/ruby/test_parse.rb#L1149-L1150
Sorry about the Copilot review, it was auto set to run.