Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: instance variable leaks from child to parent scopes #37

Open
ms-ati opened this issue Oct 24, 2019 · 5 comments
Open

Bug: instance variable leaks from child to parent scopes #37

ms-ati opened this issue Oct 24, 2019 · 5 comments
Labels
Bug Bug in existing features

Comments

@ms-ati
Copy link
Owner

ms-ati commented Oct 24, 2019

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 ms-ati added the Bug Bug in existing features label Oct 24, 2019
@ms-ati
Copy link
Owner Author

ms-ati commented Dec 19, 2019

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
Copy link
Contributor

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
Copy link
Owner Author

ms-ati commented Dec 28, 2019

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
Copy link
Contributor

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
Copy link
Owner Author

ms-ati commented Jan 13, 2021

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug in existing features
Projects
None yet
Development

No branches or pull requests

2 participants