Don't leak variable name when assigning to ivar/cvar in method signature by asterite · Pull Request #6007 · crystal-lang/crystal

asterite · 2018-04-25T14:58:43Z

The idea is that this:

def foo(@var)
end

expands to this:

def foo(var __arg0)
  @var = __arg0 # the name __arg0 could be another one, you don't know it
end

instead of:

def foo(var)
  @var = var
end

so you don't have access to that "implicit" local variable anymore.

I think this is good. It's a breaking change, but it's more intuitive. If you want it assigned to a local variable, explicitly assign it to a local variable.

jhass · 2018-04-25T15:12:40Z

src/compiler/crystal/syntax/parser.cr

+      name = "__arg#{@temp_arg_count}"
+      @temp_arg_count += 1
+      name
+    end


Can/Should this check if the name already exists in the current scope and discard it? Say I do

def foo(__arg0, @foo) end

The convention is that variables that start with __ are reserved for the compiler, so they shouldn't clash with user variables. If a user choose to name their variables __argN, they are warned that that's problematic (at least in my mind :-P).

I don't want to make things more complex than they should, at this point.

Mmh, not asking to do this here, but do you think it would actually be possible to make trying to use these a parser error then? I don't think it's too uncommon for somebody to try to use such variable names, especially when dealing with macros.

I'm not sure it can be done. Sometimes methods are passed to macros, and these are to_s'd, and then reparsed again... and that would error when it shouldn't.

Maybe it's something to consider in the future.

Note that right now the __arg or __temp hack is used all over the place in the compiler (like, when you do if a() || b(), this isn't something new. And so far it didn't cause any problem, so I guess it's fine.

bew · 2018-04-25T16:24:06Z

I don't like the sound of this.. How do you resolve this then: #3413

straight-shoota · 2018-04-25T16:27:03Z

src/compiler/crystal/types.cr


    def initialize(program, namespace, name, @superclass, add_subclass = true)
      super(program, namespace, name)
+      superclass = @superclass


It might be a style issue, but I think it makes more sense the other way around: @superclass = superclass (and superclass as arg). That's how it worked implicitly before and prevents surprises caused by mismatching types between argument and ivar.

bew · 2018-04-25T16:31:05Z

src/range.cr

+    @reached_end = false

-    def initialize(@range : Range(B, E), @current = range.begin, @reached_end = false)
+    def initialize(@range : Range(B, E))


Here you're also changing the ctor signature, we can no more setup current & reached_end. Is that intentional?

It's a private class. And the removed arguments don't seem to be used anywhere. Wouldn't make much sense anyway.

The constructor was used internally, so no problem. Same in other places.

bew · 2018-04-25T16:32:38Z

src/socket/address.cr


-    def initialize(@address : String, @port : Int32)
+    def initialize(address : String, @port : Int32)
+      @address = address


Why this change?

Ditto a few times below, where inline ivar init is ok.

address is used in the next line and it should be type String. @address would be String?.

Because @address is String? and the methods/conditions bellow won't work. But address is String and that works.

Oh wow, that's weird to read ^^ but makes sense, ty

asterite · 2018-04-25T16:53:51Z

@bew I don't know the fix to #3413 but I don't know how much it has to do with this PR. The fix is probably disallowing instance variables after = in those places, but you can anyway blow up at runtime if you call a method that accesses an uninitiaized instance var (but that's yet another issue to fix)

bew · 2018-04-25T17:02:09Z

I see it related by the suggested solution #3413 (comment)

class A
  getter :a, :b

  def initialize(@a : Int32, @b : Int32 = a)
  end
end

a = A.new a: 1
puts a.b # => 1

That won't work anymore due to this PR.

To me this solution was beautifully clean, but I can understand why having an implicit local var can be unexpected..

Maybe that other issue could be solved (in a later PR) by changing @b = @a to @b = __var0?

straight-shoota · 2018-04-25T17:12:28Z

@bew True that workaround won't work but I think that's a good thing. Shadowing ivar arguments as local vars can be unexpected. In this example it would seem that a refers to the getter.

asterite · 2018-04-25T17:15:16Z

That won't work anymore due to this PR.

Right, you'll have to do:

class A
  getter :a, :b

  def initialize(a : Int32, b : Int32 = a)
    @a = a
    @b = b
  end
end

Aaaaand... that's why I'd like to remove the @a thingy in method arguments. It's incomplete. It's a nice idea, but (as many other features in the language) it's incomplete and it will probably never be implemented well. So, instead, I'd like to remove features that can't be properly implemented.

asterite · 2018-04-25T17:16:55Z

@straight-shoota And in fact it will now refer to the getter, and give you 0 (uninitialized memory). That's a separate issue, though... but yeah, with this PR it's likely than a lot of code right now will compile just fine and behave in a weird way (segfaults, wrong memory, etc.)

asterite · 2018-04-25T17:21:24Z

Meh, I don't know why I'm even trying... my conclusion, again, as usual, is that nothing can be changed/fixed at the moment because there are so many broken/missing things that changing one thing will break another thing. This whole thing needs to be redone (and first rethought) from scratch.

straight-shoota · 2018-04-25T17:26:10Z

Are you sure about those memory issues? I assume that'd only be a problem with constructors, right?

And even there... what can break that's currently working and won't raise a compiler error with this PR?

There shouldn't be an issue with uninitialized memory in the example- With the PR it's essentially going to be equal to this:

class A
  def a_method
    @a
  end

  def initialize(@a : Int32, @b : Int32 = a_method)
  end
end

bew · 2018-04-25T17:26:22Z

To me the old solution was beautifully clean, but I can understand why having an implicit local var can be unexpected..

What's wrong here?

asterite · 2018-04-25T17:59:29Z

@straight-shoota Right, and the problem is that a_method is invoked before @a ever being assigned, hence the segfault/bad-memory.

See how that feature is not intuitive at all?

asterite · 2018-04-25T18:02:04Z

Essentially, it gets rewritten as:

class A
  def a_method
    @a
  end

  def initialize(a : Int32)
    initialize(a, a_method) # oops!
  end

  def initialize(a : Int32, b : Int32)
    @a = a
    @b = b
  end
end

But maybe someone can find a better way to implement that...

asterite · 2018-04-25T18:10:53Z

Maybe we should always fully expand the body with named arguments... a lot of code duplication, and it will slow down compilation, but it's probably for the best.

RX14 · 2018-04-25T18:25:49Z

I like this PR, it shouldn't be closed. Yes, working on the compiler is frustrating but that's not a reason not to do it. It seems we're just missing a few rules about the scopes that default arguments can access.

straight-shoota · 2018-04-25T19:16:52Z

@asterite My code works fine right now. It gives a nice compiler error. I don't see a reason why this PR would change that.

If there was really an issue, a simple fix would just be to disallow instance method calls as default arguments in constructors.

asterite · 2018-04-27T12:54:22Z

Reopened.

Do we really want to go forward with this change, or just documenting that:

def foo(@var)
end

expands to:

def foo(var)
  @var = var
end

is good? I don't mind the second option. It's how the language has been working so far. And if you have a local method with that same name, just use var().

What do you think?

asterite · 2018-04-27T14:18:03Z

I don't understand why it only fails in Travis :-(

sdogruyol · 2018-05-18T23:45:40Z

This deserves more ❤️

asterite · 2018-05-19T13:29:50Z

I think this should be closed. The current behaviour is fine.

jhass reviewed Apr 25, 2018

View reviewed changes

straight-shoota reviewed Apr 25, 2018

View reviewed changes

bew reviewed Apr 25, 2018

View reviewed changes

asterite closed this Apr 25, 2018

RX14 added the breaking-change May have breaking effect in some edge cases. Mostly negligible, but still noteworthy. label Apr 25, 2018

asterite reopened this Apr 27, 2018

Sija mentioned this pull request Apr 27, 2018

Improve WebSocket handshake validation #5327

Merged

asterite added 4 commits April 28, 2018 09:56

Don't leak variable name when assining to ivar/cvar in method signature

0c8a699

Revert order of ivar/lvar assignment

54418e6

Fix nil exception

8e0ce3e

Fix sample

2b3c696

asterite force-pushed the feature/no-implicit-instance-var branch from 49b239d to 2b3c696 Compare April 28, 2018 12:56

sdogruyol changed the title ~~Don't leak variable name when assining to ivar/cvar in method signature~~ Don't leak variable name when assigning to ivar/cvar in method signature May 7, 2018

sdogruyol added the topic:compiler label May 18, 2018

bew mentioned this pull request May 19, 2018

Undefined behavior when accessing an instance variable as default parameter #3413

Closed

asterite closed this May 19, 2018

asterite mentioned this pull request Jul 13, 2018

Parser: don't use invalid internal names for arguments like @select or @@select #6379

Merged

straight-shoota deleted the feature/no-implicit-instance-var branch April 29, 2025 07:23

Uh oh!

Conversation

asterite commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bew commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asterite commented Apr 25, 2018

Uh oh!

bew commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

straight-shoota commented Apr 25, 2018

Uh oh!

asterite commented Apr 25, 2018

Uh oh!

asterite commented Apr 25, 2018

Uh oh!

asterite commented Apr 25, 2018

Uh oh!

straight-shoota commented Apr 25, 2018

Uh oh!

bew commented Apr 25, 2018

Uh oh!

asterite commented Apr 25, 2018

Uh oh!

asterite commented Apr 25, 2018

Uh oh!

asterite commented Apr 25, 2018

Uh oh!

RX14 commented Apr 25, 2018

Uh oh!

straight-shoota commented Apr 25, 2018

Uh oh!

asterite commented Apr 27, 2018

Uh oh!

asterite commented Apr 27, 2018

Uh oh!

sdogruyol commented May 18, 2018

Uh oh!

asterite commented May 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

straight-shoota Apr 25, 2018 •

edited

Loading

bew commented Apr 25, 2018 •

edited

Loading