Crystal parser to_s can produce invalid code

[anykeyh]

Context

Hello, I'm playing around with the Crystal::Visitor, Parser, AST node and so.

I'm trying to implement a coverage binary which enrich the code with coverage operations.
So far, I've made a proof of concept which is working is most of the cases (note: it's a PoC, the code is ugly) : https://github.com/anykeyh/crystal-coverage.

For that, I'm using the syntax visitor and crystal parser to regenerate code with additional instructions.

I'm also expanding all the relative require to generate one file only enriched with the coverage code. I assume the exported file can be passed to crystal eval operation.

Example of code:

class Test
  def foo
    n = 1
    while (n < 10)
      n += 1
      puts "foo!"
    end
  end

  def bar
    begin
      x = 1

      x = x + 1
      raise "Code below will never be covered"

      puts "Some code below"
    rescue
    end
  end
end

test = Test.new

test.foo

Same code enriched using the coverage binary:

require "coverage/runtime"
::Coverage::File.new("spec/test.cr", "6df201f01e8cfb7f575a585521553d65",[23, 25, 3, 5, 4, 6, 12, 14, 15, 17])
#<loc:"spec/test.cr",0,0>
class Test
  def foo
    ::Coverage[0, 2] #<loc:"spec/test.cr",2,0>
        n = 1
    while (        ::Coverage[0, 4] #<loc:"spec/test.cr",3,0>
    n < 10
)
      ::Coverage[0, 3] #<loc:"spec/test.cr",4,0>
            n += 1
            ::Coverage[0, 5] #<loc:"spec/test.cr",5,0>
      puts("foo!")


    end

  end
  def bar
    ::Coverage[0, 6] #<loc:"spec/test.cr",11,0>
    begin
      x = 1
            ::Coverage[0, 7] #<loc:"spec/test.cr",13,0>
      x = x + 1

            ::Coverage[0, 8] #<loc:"spec/test.cr",14,0>
      raise("Code below will never be covered")

            ::Coverage[0, 9] #<loc:"spec/test.cr",16,0>
      puts("Some code below")

    rescue
    end
  end
end
::Coverage[0, 0] #<loc:"spec/test.cr",22,0>
test = Test.new

::Coverage[0, 1] #<loc:"spec/test.cr",24,0>
test.foo


::Coverage.get_results(Coverage::Outputter::Text.new)

Problem

Since I depend heavily on to_s of the nodes, I've noticed a case where the code outputted is not valid code:

  class Test
    def initialize(@select)
    end
  end

The AST to_s method will produce

  class Test
    def initialize(select)
      @select = select
    end
  end

Since select is a reserved keyword, the code will not be evaluable. It obviously happens for any reserved keywords.

How to reproduce

require "compiler/crystal/syntax/*"

source = <<-CRYSTAL
  class Test
    def initialize(@select)
    end
  end
CRYSTAL

astree = Crystal::Parser.parse(source)
puts astree.to_s

[asterite]

Yes, the shortcut @var in methods was a mistake, it also has other problems. We should probably remove it.

[straight-shoota]

@asterite I think this feature is really nice and the issues are not entirely unsolvable, nor are they serious enough to make it completely fail. For #3413 it'd be relatively easy to disallow ivars as default values and the problem is solved while still being able to set ivars from method arguments.

For this issue here I'd suggest to disallow ivars as arguments that are actually a keyword (this would match with #5930 smoothly). This is a little confinement but still keeps the feature usable and productive, without many problems. At least none that I'm aware of.

[asterite]

Oh, actually, we should just change how they are expanded. I tried it yesterday but got flooded with "invalid memory access" after doing the change and got demotivated. It will also solve a few other problems. The idea is to expand this:

def foo(@var)
end

to this:

def foo(var __arg0)
  @var = __arg0
end

I'll try it later, I'll see if I can make it work.

[anykeyh]

@asterite

I think the fix could break some code if applied in every case (well it's not a big breaking however).

def method(@var)
  puts var #Works currently in crystal
end

Should it be transposed to __argN only when RESERVED_KEYWORDS.include?(variable_name) ?

[straight-shoota]

Changing the expansion is also a viable option. However, it makes it difficult to access the argument value as local variable.

def foo(@var)
  var.do_something # => undefined local variable or method 'var'
end

This seems to be a high price compared to the alternative to simply disallow a few error-prone variable names.

[asterite]

No, no, it's ok to change that behaviour:

#4332

If you have a method var, it's confusing that it would access the local variable. The fact that @var is implemented with a local variable should be transparent to the user. This is a breaking change, but a good one. Using the instance variable instead of the local variable is no problem at all.

[bcardiff]

@asterite I agree that @var could hide var in the scope.
There is though some corner case regarding unions where var and @var are not interchangeable.

class Foo
  @value : Int32?
  
  def set(@value : Int32)
    pp typeof(value) # => Int32
    pp typeof(@value) # => Int32 | Nil
  end
end

Foo.new.set(4)

But then the user can remove the @ in the args and to the assignment manually. It's a good tradeoff.

[asterite]

@bcardiff You are right. In fact I got it working now and I had to change such code in a few places. But yeah, I think it's a good tradeoff. Otherwise, we can close #4332 and say "it's the way the language works", which can always be good. I don't know.

[straight-shoota]

Okay, sounds good. Accessing a ivar argument as local variable is probably a less-known feature anyway. The workaround is easy and more expressive. And avoids surprises as described in #4332.