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

Change context-mode's default to new mode 4. #138

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

marcandre
Copy link
Member

@marcandre marcandre commented Nov 20, 2020

This new mode uses a copy of the TOPLEVEL_BINDING. This is compatible with refinements (contrary to mode 3), while keeping nested IRB sessions separate.

Current default makes it impossible to call using SomeModule from the top level.
Other modes work:

irb --single-irb # => works (shared context for nested sessions)
irb --context 0 # => works
irb --context 1 # => works
irb --context 2 # => works
irb --context 3 # (current default) => does not work
irb --context 4 # (proposed default) => works

I believe the default mode can not be made to work as it is by design that one can not call using from inside a method.

This PR proposes a 4th mode that seems the most versatile and least error prone: TOPLEVEL_BINDING.dup.

I am surprised that it isn't used already, I was expecting this to be the default. Either there is some limitation that I am not aware of, or else maybe it is for historical reason that is not used?

Fixes redmine bug #9580

This new mode uses a copy of the TOPLEVEL_BINDING. This is compatible with refinements (contrary to mode 3), while keeping nested IRB sessions separate
@aycabta
Copy link
Member

aycabta commented Dec 16, 2020

It's just for historical reason. Great, thank you!

@marcandre marcandre deleted the good_binding branch December 16, 2020 14:03
@marcandre
Copy link
Member Author

Super, thanks!

@marcandre
Copy link
Member Author

Will you take care of copying this commit to main ruby?

@aycabta
Copy link
Member

aycabta commented Dec 18, 2020

@marcandre

Will you take care of copying this commit to main ruby?

Done!

@marcandre
Copy link
Member Author

Great, thanks! 🎉

@eregon
Copy link
Member

eregon commented Feb 12, 2021

I think this change might cause https://bugs.ruby-lang.org/issues/17623#note-3

To be more precise, TOPLEVEL_BINDING captures the local variables of the main script (here bin/irb).

@marcandre
Copy link
Member Author

marcandre commented Feb 12, 2021

@eregon indeed...

I only get TOPLEVEL_BINDING.local_variables # => [:str, :version] though...
Oh, right, there's a conditional.

We could fix this by modifying slightly bin/irb to not create any local variable... I'll open a PR

@eregon
Copy link
Member

eregon commented Feb 12, 2021

I think bin/irb in general might be generated, e.g., when doing gem install irb, so if we change it it should probably be done in RubyGems (the whole logic could be in a -> { ... }.call to avoid local vars).
I guess in general it could be useful if Ruby exposed (a frozen) EMPTY_BINDING or so, which could be a copy of TOPLEVEL_BINDING but before loading the main script (e.g., libraries required through -rmylib see TOPLEVEL_BINDING as empty).

@marcandre
Copy link
Member Author

I see the comment about them being generated, but I also notice that bin/racc has a different layout than the others so I'm no sure they are actually generated.

@marcandre
Copy link
Member Author

I created ruby/ruby#4176

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

Successfully merging this pull request may close these issues.

3 participants