Fix Style/ConditionalAssignment crash on multi-line regex in conditional
#14599
+58
−3
Conversation
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
martinemde
force-pushed
the
martinemde/multi-line-regex-conditional-in
branch
2 times, most recently
from
1a2e504 to
cebd9d6
Compare
martinemde
changed the title
Style/ConditionalAssignment crash on multi-line regex in conditional
koic
reviewed
Oct 11, 2025
|
|
||
| white_space = white_space_range(child, column) | ||
| corrector.remove(white_space) if white_space.source.strip.empty? | ||
| corrector.remove(white_space) if white_space && white_space.source.strip.empty? |
There was a problem hiding this comment.
That's just my two cents. Can you refactor to the following?
diff --git a/lib/rubocop/cop/style/conditional_assignment.rb b/lib/rubocop/cop/style/conditional_assignment.rb
index af9478704..f7433967e 100644
--- a/lib/rubocop/cop/style/conditional_assignment.rb
+++ b/lib/rubocop/cop/style/conditional_assignment.rb
@@ -442,9 +442,9 @@ module RuboCop
branch.each_node do |child|
next if child.source_range.nil?
next if child.parent.dstr_type?
+ next unless (white_space = white_space_range(child, column))
- white_space = white_space_range(child, column)
- corrector.remove(white_space) if white_space && white_space.source.strip.empty?
+ corrector.remove(white_space) if white_space.source.strip.empty?
end
if condition.loc.else && !same_line?(condition.else_branch, condition)There was a problem hiding this comment.
I think I found an even better option, no complexity increase and the white_space_range code makes a bit more sense to me now. We only return non-nil when the range is actually whitespace, instead of checking at the caller location.
Fix `Style/ConditionalAssignment` multi-line regex crash Multi-line regexes caused a crash on when trying to check or fix conditional assignment with enforced style assign_inside_conditional.
martinemde
force-pushed
the
martinemde/multi-line-regex-conditional-in
branch
from
cebd9d6 to
bd03b85
Compare
martinemde
commented
Oct 11, 2025
Comment on lines
446
to
+447
| white_space = white_space_range(child, column) | ||
| corrector.remove(white_space) if white_space.source.strip.empty? | ||
| corrector.remove(white_space) if white_space |
There was a problem hiding this comment.
I went back and forth on next unless (white_space = white_space_range(child, column) but I think this reads better.
There was a problem hiding this comment.
The repeated appearance of white_space is a bit redundant, but it's not strictly required at this point. Thank you for resolving it!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Multi-line regexes caused a crash when trying to apply conditional assignment assign_in_condition.
The culprit is multi-line regex. The original had an interpolation, but it still crashes without the interpolation. I chose to keep both tests.
Unfortunately, I had to bypass PerceivedComplexity, but other metrics were already skipped. Not sure if it's worth refactoring.
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.