ruby-style-guide icon indicating copy to clipboard operation
ruby-style-guide copied to clipboard

Spooky variable declaration

Open danmichaelson opened this issue 11 years ago • 8 comments

Weird things can happen when you declare a variable within a conditional that you intend to use outside of it. Example:

if false
  x = 1
end
puts x # nil, even though the code declaring it was never executed
puts y # crash

See http://stackoverflow.com/questions/12928050/why-does-ruby-seem-to-hoist-variable-declarations-from-inside-a-case-statement-e .

I propose variables should never be declared for the first time within a conditional if you intend to use them outside.

Bad (works, but looks so scary):

if false
  x = 1
end
puts x # nil

Bad (works, but if you made a mistake in your logic you get the previous scenario, so it still looks alarming / requires too much inspection to figure out why it won't blow up):

if false
  x = 1
else
  x = nil
end
puts x # nil

Good (self-evidently safe in a way that doesn't rely on weird parser quirk):

x = nil # Declare the default value for the variable outside the conditional
if false
  x = 1
end
puts x # nil

danmichaelson avatar Sep 03 '14 15:09 danmichaelson

It can be just classified as yet another feature of Ruby that makes code more concise.

Why should one write an additional line of code if it works without it?

abotalov avatar Sep 03 '14 17:09 abotalov

Why should one write an additional line of code if it works without it?

Because sometimes it looks really confusing though your words make sense.

freemanoid avatar Sep 03 '14 18:09 freemanoid

Doesn't it make you do a double take every time you see that a variable may not be declared; knowing that at times undeclared variables will crash Ruby?

Scary feeling == bad style. There are plenty of recommendations in this style guide that add a few more characters of verbosity.

This for example does crash:

puts x
x = 1

So it seems to me the parser's behavior in spookily declaring variables that are out of the execution flow, is rather arbitrary/ill-defined, even though it does handle the common gotcha of declaring inside one fork of a conditional.

Or maybe it's just me, I'm also curious about the discussion / what others think.

danmichaelson avatar Sep 04 '14 12:09 danmichaelson

Agreed (with @danmichaelson and @freemanoid). @abotalov is perfectly correct when he notes that it's a "concise" feature of the language; if we hadn't all seen cases where Ruby isn't so consistent about avoiding crashes, we'd be less spooked by this.

jdickey avatar Sep 05 '14 03:09 jdickey

x = if false
  1
end
puts x

or

x =
  if false
    1
  end
puts x

LavirtheWhiolet avatar Sep 08 '15 18:09 LavirtheWhiolet

I agree with @danmichaelson that this looks a bit weird, especially to people coming from other languages. I'd certainly encourage people to declare variables in a more obvious fashion. :-)

bbatsov avatar Jun 12 '19 08:06 bbatsov

@bbatsov

I'd certainly encourage people to declare variables in a more obvious fashion.

Fervently agreed. Perhaps a gentle reminder in the Style Guide that inspires a warning-level diagnostic in RuboCop and similar? E.g.,

Care is recommended when declaring and assigning to variables. If their only assignment is within a block, consider explicitly assigning nil to the variable immediately before the block, so the reader understands that s/he isn’t missing a previous assignment.

Possibly leading to a tool diagnostic such as

Style/AssignmentReadability: Variable ‘x’ appears to be declared within a block and used outside it. Assigning nil before the block will produce clearer code and silence this warning.

Thoughts?

jdickey avatar Jul 31 '19 18:07 jdickey

No objections from me. It'd be nice to add some lint cop in RuboCop flagging such issues.

bbatsov avatar Jul 31 '19 20:07 bbatsov