Crystal: previous_def without args simply forwards local variables with the same name as the args

[lbguilherme]

def foo(z)
  z
end
def foo(z)
  ->(z : Int32) { z + previous_def }
end
p foo(2).call(3)

I expect it to output 5 but it prints 6. As the second z variable is simply shadowing the method argument, instead of really editing it. previous_def should really take the arguments, even if there is no syntax to access them at the current scope.

[asterite]

Indeed, though the workaround is very simple so I'll probably take my time to fix this :-)

[asterite]

However... https://carc.in/#/r/12ae

This is Ruby:

class Foo
  def foo(x)
    p x
  end
end

class Bar < Foo
  def foo(x)
    x = 10
    super
  end
end

Bar.new.foo(20)

It prints 10, which means that super effectively forwards the changed x, not the original x. If Ruby does it there must be a reason (maybe simplicity, to not have to store a copy of every method argument). Or maybe do you can change one argument and forward them all, with just one changed.

So now I feel inclined not to change this behavior.

[bcardiff]

But

class Foo
  def foo(x)
    p x
  end
end

class Bar < Foo
  def foo(x)
    [10].each do |x|
      super
    end
  end
end

Bar.new.foo(20)

prints 20 (in ruby). In your sample @asterite the scope is not changed so 10 is the right answer.

[asterite]

Hmmm... then I think it's really hard to implement with syntax sugar. We'd have to store the arguments in a separate, temporary variable, and every assignment to it, that isn't shadowed by a block variable, needs to be reassigned as well. And then super passes those args.

I'd prefer to simply say that super inserts the arguments, for example super(x) in the case above, and if x was reassigned or shadowed then, well, it gets that value. In any case the workaround (or the proper way to do it) is to use other variable names to avoid confusion.

[luislavena]

In any case the workaround (or the proper way to do it) is to use other variable names to avoid confusion.

Can we have the parser spit out some sort of warning in those cases when compiling? That way it will not be a surprise.

Thanks :smile:

[asterite]

@luislavena I don't believe warnings are useful, and the compiler has none right now. We have these options:

1. Disallow shadowing
2. Force the user to explicitly forward arguments in super and previous_def
3. Leave things as they are

We could do 2, I don't think super and previous_def are used that much so explicitly forwarding args will be a bit more cumbersome, but always explicit. Otherwise I'd do 3 :-)

[luislavena]

I think explicitness wins over magic on what-the-heck moments. Of course,
that might be tedious to some. So will shut up on my personal taste for now
😁

Sorry for top posting, sent from mobile.
On Jun 24, 2016 17:11, "Ary Borenszweig" [email protected] wrote:

@luislavena https://github.com/luislavena I don't believe warnings are
useful, and the compiler has none right now. We have these options:

1. Disallow shadowing
2. Force the user to explicitly forward arguments in super and
   previous_def
3. Leave things as they are

We could do 2, I don't think super and previous_def are used that much so
explicitly forwarding args will be a bit more cumbersome, but always
explicit. Otherwise I'd do 3 :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/crystal-lang/crystal/issues/2894#issuecomment-228372594,
or mute the thread
https://github.com/notifications/unsubscribe/AAAQVuz-sGQwc1guKshCk2goQGQKGO-Dks5qO_OkgaJpZM4I7wGE
.

[david50407]

IMO, always pass the same argument (not changed) while super and previous_def got no arguments is better.

and it will be like this

def foo(z)
  z
end
def foo(z)
  ->(z : Int32) { z + previous_def }
end
p foo(2).call(3) #=> 5

class Foo
  def foo(x)
    p x
  end
end

class Bar < Foo
  def foo(x)
    x = 10
    super
  end
end

class Bar2 < Foo
  def foo(x)
    [10].each do |x|
      super
    end
  end
end

Bar.new.foo(20) #=> 20
Bar2.new.foo(20) #=> 20

And if we want new value to be argument in @asterite's code, we can pass new variable manually

class Bar < Foo
  def foo(x)
    x = 10
    super x
  end
end

Bar.new.foo(20) #=> 10

[ozra]

Even though I like the idea of static analysis to help out in the form of warnings (though perhaps _only_ when passing "--gimme-warnings" or such). I think @asterite proposition (2) - to require explicit forwarding - is the cleanest in the long run.
Another, but very breaking option, is that args-variables are always non-assignable, so if you'd like a modified value you'd use a new local variable at all times.

[ozra]

Revisit, after lots of sleeping on it. My bet is definitely on 2.