Bug: instance variable leaks from child to parent scopes

[ms-ati]

See modern-project/modern-ruby@e1eee26 where Docile was replaced with instance_eval to avoid the leakage of an instance variable:

https://github.com/modern-project/modern-ruby/blob/e1eee26187e5281f711ac3d52aaf925cf1364b31/lib/modern/dsl.rb#L10-L13

[ms-ati]

This case is very interesting, as it raises a design question for Docile.

Background

The core design goal of Docile is to make variables, instance variables, and methods that are visible in the context in which the DSL block is written, available for usage within the DSL block. That is, for example:

# context of the block, outside the DSL object
@temperature = 99

# treat an instance of Array as a DSL
dsl = []

Docile.dsl_eval_with_block_return(dsl) do
  append @temperature if @temperature > 100
end

#=> []

In order to make this work for instance variables like @temperature above, Docile copies all instance variables from the outside context into a proxy object. The block will be executed in the context of the proxy object, which will provide fallback of method lookup: first to the DSL object, and second to the outside context.

Design problem

The above only describes the use case of reading outside instance variables inside the DSL block. Docile also supports writing instance variables inside the DSL block, and having them propagate to the outside context.

That is, in order for Docile.dsl_eval to allow mutation of outside instance variables, including setting new ones, from within the DSL block: it copies all the instance variables that exist in the proxy object after the block has executed, back out into the surrounding context. For example:

# before
@an_instance_var
#=> nil

Docile.dsl_eval([]) do
  @an_instance_var = 42
end

# after
@an_instance_var
#=> 42

The problem at hand is that, in a multi-level DSL such as found here in modern-ruby, the behavior of copying the instance variables out may not actually be the desired behavior expected by most users.

Potential solutions

Brainstorming (please add more)

1. Document existing behavior more clearly, help users code to the existing API with more explicit docs
2. Stop copying instance variables out entirely, full stop (breaking change)
3. Add configuration options to control this behavior (decide the default behavior)
4. Add a new API call that differentiates this behavior (keep existing behavior for existing calls), like dsl_eval_readonly_ivars
5. Think of some more clever heuristic, which would usefully decide whether and when to copy the instance variables out after the DSL block runs on the proxy object

Additional ideas are very welcome!

[taichi-ishitani]

Hi @ms-ati ,
I consider the dsl_eval method an enhanced instance_eval method.
Therefore, I think it is better that new instance variables will be copied to the dsl object.

o = Docile.dsl_eval(Object.new) { @foo = 1 }
o.instance_variables # => [@:foo]

[ms-ati]

Thank you as always @taichi-ishitani!

You voted for instance variables written in the block being written to the DSL object. Isn’t this going to remain true no matter what we do?

My understanding is that the question is whether to continue copying the instance variables “out” to the block’s lexical context.

Perhaps I have misunderstood, if so can you please help me to understand?

[taichi-ishitani]

Hi @ms-ati ,

Sorry for delay because I'm on year end/new year holidays.

You voted for instance variables written in the block being written to the DSL object.

Your understanding is correct. I think this issue is to determine basis of Docile and is very important. Therefore, I added a new behavior.

I think what users want Docile to behave is depending on their contexts and it is better that Docile offer the option to control where instance variables will be written back. This thought is close to #3 or #4 maybe.

How about implementing this option as a new argument like below?

Docile.dsl_eval([], readonly_ivars: true) { @foo = 1 }
Docile.dsl_eval([], writeback_ivars_to: :block_context) { @foo = 1 }
Docile.dsl_eval([], writeback_ivars_to: dsl_object) { @foo = 1 }

keyword argument is good for this purpose if you can drop legacy ruby support.

[ms-ati]

@taichi-ishitani I'd like to come to a conclusion for how to support. the goals of this ticket in a future Docile 2.0. If you'd like to write up a more complete design document explaining this as a feature request, it could be very helpful?