Crystal: `with` . `yield` compilation error should mention current receiver

[mpcjanssen]

While writing a DSL using with ... yield, I ran into a compilation error which took me a long time to fix because the actual receiver was not mentioned in the error (which turned out to be nil).

A reduced example:

def make_object
  "test"
  puts "This was my error"
end

def doit
    with make_object() yield
end

doit {
  size
}

Which gave the following error:

Error in line 10: instantiating 'doit()'

in line 10: instantiating 'doit()'

in line 11: undefined method 'size'

I was surprised by this error because the string object should implement the size method. Of course my error would have been much easier to discover if the error message would have included the fact that the receiver was of class Nil.

Contrast this with:

def make_object
  "test"
  puts "this was my error"
end

make_object().size

giving the more helpful:

Error in line 7: undefined method 'size' for Nil

Rerun with --error-trace to show a complete error trace.

[asterite]

The problem is that with ... yield will look up errors in two scopes: the with scope and self. Then the error is shown for the last scope used. And right now the compiler doesn't show errors for two scopes.

Will all of that said, I believe having two search scopes is super confusing. I'd like to remove this feature from the language.

[Sija]

@asterite with ... yield is a godsend for DSLs, wouldn't be better to fix the implementation?

[asterite]

It's not about the implementation, it's about it being confusing. No matter how you implement it, it's not readable.

[mpcjanssen]

Why does the compiler also lookup items in the self scope?

[asterite]

Because otherwise you wouldn't be able to do anything. For example you wouldn't be able to call puts.

[mpcjanssen]

How is that different from the scenario where an instance method uses puts?

[mpcjanssen]

In the scope of a block passed to with obj yield, self should be obj. The original self should play no role.

[asterite]

You are right, it's exactly the same:

class Foo
  def bar
    size
  end
end

Foo.new.bar # => undefined local variable or method 'size'

So this issue has nothing to do with with ... yield. Maybe the error should also mention what the current scope is (except that in with .. yield there would be two scopes).

[mpcjanssen]

Except in that case, it mentions the actual class in the errorline before so it's much clearer:

Error in line 7: instantiating 'Foo#bar()'

in line 3: undefined local variable or method 'size'

Mentioning both scopes would have made my debugging session easier indeed.

However, I still don't understand completely why there have to be two scopes. I would expect the scope of with obj yield to be obj only as if the block was defined as a method on ObjClass

[asterite]

@mpcjanssen Should this work in your opinion?

class Foo
  def bar
    bar = Bar.new
    bar.with_bar do
      method_in_foo(method_in_bar)
    end
  end

  def method_in_foo(x)
    puts x
  end
end

class Bar
  def with_bar
    with self yield
  end

  def method_in_bar
    10
  end
end

Foo.new.bar

[asterite]

Maybe we did give with ... yield an incorrect semantic after all. When using instance_eval in Ruby, it can't find method_in_foo because the scope changes completely. So maybe with ... yield should completely change the scope in Crystal too... except that if we do that then visually tracking reading/assignment of instance variables becomes impossible.

More reasons to remove this feature...

[mpcjanssen]

IMO the scope should be changed completely. Anything else will be very confusing. So your example should indeed give an error:

undefined local variable or method 'method_in_foo' for Bar

Also note that the error message being _incomplete_ is not an issue only with with yield . try behaves exactly the same.

def make_object
  "test"
  puts "this was my error"
end

def doit(&block)
  make_object.try block
end

doit {
  size
}

Error in line 11: undefined local variable or method 'size'

[mpcjanssen]

Note that the example with with yield replaced with try works as expected (i.e. it doesn't)

class Foo
  def bar
    bar = Bar.new
    bar.with_bar do
      method_in_foo(method_in_bar)
    end
  end

  def method_in_foo(x)
    puts x
  end
end

class Bar
  def with_bar
    with self yield
  end

  def method_in_bar
    10
  end
end

Foo.new.bar

Error in line 24: instantiating 'Foo#bar()'

in line 5: undefined local variable or method 'method_in_bar'

So what I really want is:

• Make with...yield behave the same as try in lookup.
• Add the receiver of the block in errors for try and with..yield

So probably what I am describing is a try without the nil check.

[asterite]

Yeah, we should probably make that change. Just changing the scope would make the error message already correct, I think.

[mpcjanssen]

I would not mind making a PR for this, but I have no clue where with yield and yield itself are implemented. A text search in the source is not helping much.

[asterite]

Don't worry, it's super complex. In the compiler it's named as with_scope. But that will have to go away I think.

[asterite]

Actually, I still don't know what we would do with instance variables inside that scope... so for now I wouldn't change anything.

[mpcjanssen]

Instance variables are also resolved in the receiver object. Anything else is bug.

If Ruby has this buggy behavior I suggest a new method on Object, maybe called invoke, which does the right thing and then deprecate with ... yield. I wouldn't mind if this required an explicit block in the parameter list.

[asterite]

Maybe we can support reading instance variables but not writing to them inside those blocks. Or maybe we should stop supporting instance vars at all inside those blocks, that's probably possible.

This can't be implemented with a regular method in Crystal, only with a magical method or with a language construct. We went with a language construct.

[mpcjanssen]

Regarding reading only, that would probably even be more confusing.

So with obj yield v2 spec with a different name if needed:

• Runs completely in the context of obj for method and variable lookup. The effect should be indistinguishable from defining an instance method on ObjClass with the yielded block as the body and calling that.
• Because the compilation of the block is non-local (thus not in the actual class definition of ObjClass), the error message in case of compilation errors only mentions the local enclosing scope and not the compiling scope
• So to make the error more helpful (and close this issue) the scope of ObjClass should be added to the actual error.
• If this means any compilation error of a yielded block gives the possibly redundant scope, that is OK IMO
• This also means that there is a self in the yielded block scope .i.e obj.

I am willing to do this work. Just need an initial pointer to where with yield is defined.

[mpcjanssen]

Note that the spec above matches Kotlin's DSL workhorse _lambda with receiver_ to the letter. https://kotlinlang.org/docs/reference/lambdas.html#function-literals-with-receiver

[asterite]

@mpcjanssen Just in case you want to take a look at this, the implementation is all over the place in the compiler: there's semantic and codegen involved in this. Just search for node : Yield in the codebase. But I will try to change the semantics myself if I can. It would be also nice to be able to capture these blocks and execute them in another context, though I think that's an entirely different type of Proc.

[bcardiff]

@mpcjanssen what do you mean by variable lookup in?

• Runs completely in the context of obj for method and variable lookup.

The blocks (method local) variables are scoped by the lexical scope. In that sense that is also the reason why instance vars are accessible both for reading and writing. Otherwise the user will be forced to do var = @ivar before calling a method that will call a block changing the receiver.

And the same behavior holds for normal blocks, otherwise it will even more confusing.

I have a draft of a proposal for replacing the with ... yield, maybe is time to share it as is.

[mpcjanssen]

@bcardiff I meant that a reference like @var will be looked up as an instance var of the receiver. So that means the lexical scope is completely irrelevant.
I am not exactly sure how that would work with closures with captured variables.

[mpcjanssen]

Btw I think the worry about not knowing where a method is/can be defined in a block is a bit exaggerated. It's no different from finding where inherited or mixed in methods come from.
However clearly mentioning the lookup context when compilation fails is essential.

[asterite]

As a first step I would:

• change the current implementation to only lookup things like foo (no explicit receiver) in the context of the object given by with ... yield
• disallow accessing instance variables inside those blocks

When we implemented with ... yield we got the semantics wrong from Ruby... or at least they are different. In Crystal right now you can access the with ... yield scope as well as the "current" scope: the former is searched first and if nothing is found the latter is searched next. That should be changed to only search in the with ... yield scope.

That will make it a bit hard to share data between the current scope and the with ... yield scope, but you can use local variables for that. Specifically, you could do:

this = self
method_that_uses_with_yield do
  # here you can access self through `this`
end

I can work on a fix for this if other core team members agree about this change.

Then we can also consider @bcardiff 's proposal. Maybe having that implicit scope isn't that bad after all, but making it more explicit (and also easier to implement) might be a good idea.

[bcardiff]

not been able to do

def build
  html do
    p do
      @name 
    end
  end
end

or

def build(name)
  html do
    p do
      name 
    end
  end
end

is not acceptable IMO.

I _think_ I'm ok if what we is hidden are methods of the caller, hence forcing a this = self. But then we will force things like

def build
  _name = name # call to getter
  html do
    p do
      _name 
    end
  end
end

or

def build
  _self = self
  html do
    p do
      _self.name 
    end
  end
end

and I would rather leave prefer to have the former scope as fallback. (Of course with nice errors).
But I aim to have joyful DSLs :-)

[asterite]

Then I guess the foo& syntax sugar is the best option... because it's just syntax sugar.

[straight-shoota]

@bcardiff The second example would work because name is a local variable. But yeah, it would be better if you could also call for example helper methods from the lexical scope.

[ysbaddaden]

In Ruby none of your unacceptable examples are possible @bcardiff. If you need helpers they must be included in the new context. If you need ivars, well... ActionView copies them from the controller so they're accessible, for example :/

To be honest, I really don't like the foo& {} proposal. Let's make with foo yield completely change the context to foo (following Ruby's instance_eval) or let's just drop this feature, and require an explicit receiver: no more confusion would be possible.

But let's avoid introducing a weird syntax —we're already abusing the & symbol.

[mpcjanssen]

It seems that this issue is now discussing two separate things:

1. The behavior of with ... yield
2. The error message when the compilation of the block fails.

I initially raised this issue to discuss point 2. Most discussion seems to relate to point 1. Should that be split in a different issue?

[asterite]

@mpcjanssen You are right. I'll send a PR fo fix 2.

[asterite]

And probably the current semantics are fine: you can use the DSL (methods without a receiver will first hit the with ... yield scope), you can use self and access self's instance vars, and you can invoke self methods (if they clash with the with ... yield scope you can do self.method). So there's probably no need to change anything... yet.