Warn on class/const assign in condition#9404
Merged
Merged
Conversation
Extend checkAssignmentInCondition to treat ClassVarAsgnNode and ConstDeclNode as assignable nodes so assignments like class vars or constant declarations in conditionals will trigger the existing "assignment in conditional" warning. Also re-enable the test_assign_in_conditional test by removing its exclude in test/mri/excludes/TestParse.rb.
There was a problem hiding this comment.
Pull request overview
Extends JRuby’s existing “assignment in conditional” warning to also catch class variable assignments and constant declarations used as conditional expressions, and re-enables the corresponding MRI parsing test.
Changes:
- Updated
checkAssignmentInConditionto treatClassVarAsgnNodeandConstDeclNodeas assignable nodes eligible for the warning. - Re-enabled the MRI test
test_assign_in_conditionalby removing its exclusion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/mri/excludes/TestParse.rb |
Removes the exclusion so test_assign_in_conditional runs again. |
core/src/main/java/org/jruby/parser/RubyParserBase.java |
Expands assignment-node detection in conditionals to include class var and constant assignment nodes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
@sampokuokkanen sorry I missed this one. I think it looks good but it required a merge so I will merge this once that completes. Looks like the right change and we love to see tests untagged! |
enebo
enabled auto-merge
May 2, 2026 16:06
enebo
disabled auto-merge
May 2, 2026 16:06
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.
Extend checkAssignmentInCondition to treat ClassVarAsgnNode and ConstDeclNode as assignable nodes so assignments like class vars or constant declarations in conditionals will trigger the existing "assignment in conditional" warning. Also re-enable the test_assign_in_conditional test by removing its exclude in test/mri/excludes/TestParse.rb.
So code like following:
and
will now have the
found '= literal' in conditional, should be ==warning.Sorry about the Copilot review, I have now disabled it so it will no longer auto review my PRs from now on.