ruby-lsp icon indicating copy to clipboard operation
ruby-lsp copied to clipboard

Add inline variable refactor

Open vinistock opened this issue 1 year ago • 3 comments

[!NOTE] This issue is aimed at those attending the RubyConf 2024 Hack Day

Add a new code action to inline a local variable. For example:

# BEFORE
def foo
  a = 5 * 10
  puts a
  b = a - 5
end

# AFTER
def foo
  puts 5 * 10
  b = 5 * 10 - 5
end

Here's an example PR adding a refactor end to end https://github.com/Shopify/ruby-lsp/pull/2372.

vinistock avatar Aug 28 '24 17:08 vinistock

I was reading through this issue, and wanted to note that the resulting expression may need to have parenthesis in order to keep order of operations good. I'm not sure if it would be possible to calculate that it's not needed. Some examples counter to the example in the issue:

# Subtraction before multiplication
# BEFORE
def foo
  a = 5 - 10
  puts a  # => -5
  b = a * 5  # => -25
end

# Without ()
def foo
  puts 5 - 10 # => -5
  b = 5 - 10 * 5 # => **-45**
end

# With ()
def foo
  puts (5 - 10) # => -5
  b = (5 - 10) * 5 # => **-25**
end

I can't think of an example for this other thought quick enough, but I suspect there would be issues with certain method calling instances as well, akin to what you see with 1..10.map vs (1..10).map... you know, all those classic cases where you end up needing parens afterall.

I just wanted to highlight this concern as I feel like this refactoring tool could create some easy-to-miss bugs if it doesn't wrap the expression in parens.

ttilberg avatar Sep 20 '24 02:09 ttilberg

Maybe there should be a check for "does the replacement candidate, or replacement targets include any method calls" in the ast? And if so, we should use parens? This may be naïve, but it seems like that's where things can get sticky.

ttilberg avatar Sep 20 '24 02:09 ttilberg

That's a really good point. Maybe the implementation should check if the target of the inlining is a unary/math expression and then enforce parenthesis in those cases.

vinistock avatar Sep 20 '24 12:09 vinistock