Feature #15903
closedMove RubyVM.resolve_feature_path to Kernel.resolve_feature_path
Description
RubyVM contains mostly MRI-specific features but resolve_feature_path
is clearly not MRI-specific.
So I propose to move it as a class method of Kernel
.
I think this makes sense given the related load
and require
are defined in Kernel
too.
Moreover, moving this method outside RubyVM
is necessary for other Ruby implementations to implement it, and keep the clean separation that RubyVM
is only defined on MRI (see #15752).
So, can I move RubyVM.resolve_feature_path
to Kernel.resolve_feature_path
?
Do we need to keep the method on RubyVM (and deprecate it), or can we just remove it since anyway API under RubyVM is not stable?
Updated by Eregon (Benoit Daloze) over 5 years ago
- Related to Feature #15230: RubyVM.resolve_feature_path added
Updated by Eregon (Benoit Daloze) over 5 years ago
- Related to Feature #15752: A dedicated module for experimental features added
Updated by Eregon (Benoit Daloze) over 5 years ago
- Assignee set to Eregon (Benoit Daloze)
Updated by byroot (Jean Boussier) over 5 years ago
This is kinda tengential, so sorry if it's shifting the discussion. But if resolve_feature_path
is to be made a first class public API, I wonder if it could be the occasion to make Kernel#require
invoke Kernel#resolve_feature_path
under the hood.
The reasoning is similar to [#11140] which made Kernel#autoload
invoke Kernel#require
and allowed tools like bootscale / bootsnap.
If the feature resolution logic was swappable it could make these tools much simpler, and open the door to avoiding $LOAD_PATH entirely.
Updated by mame (Yusuke Endoh) over 5 years ago
(I'm an author of RubyVM.resolve_feature_path
.)
Sorry but I'm not so positive. From perspective of module design, I agree that Kernel
module looks the best place to add the method. However, we can't be too careful to add anything to Kernel
nowadays. At least, I don't want to do that until we receive an actual request to make the method available in production. Currently, I have no reason to move it to Kernel
, except module design consistency.
This is just my opinion. It is all right if matz accepted this.
Updated by Eregon (Benoit Daloze) over 5 years ago
- Assignee changed from Eregon (Benoit Daloze) to matz (Yukihiro Matsumoto)
mame (Yusuke Endoh) wrote:
However, we can't be too careful to add anything to
Kernel
nowadays.
I propose only as a class method, not an instance method, so I think there is literally no chance for conflicts. What's your concern?
At least, I don't want to do that until we receive an actual request to make the method available in production.
We very rarely receive this, e.g., even for RubyVM::InstructionSequence which is now used in production (bootsnap).
I think it is not a good criteria, it's just too easy to use RubyVM
in user code.
I understand we should have an actual use-case, but we already have since the feature was introduced.
It would be useful when wanting to have more control over loading files (e.g., I guess this could be useful in RubyGems), and potentially bootsnap as @byroot (Jean Boussier) just said above.
Currently, I have no reason to move it to
Kernel
, except module design consistency.
I think that's a good enough reason on its own.
RubyVM
shouldn't become a random collections of classes & methods of which part of it are MRI-specific and part not, part stable and part not.
That's just so messy, so I'd like to fix that.
This issue is a trivial fix for I think an obvious case that does not belong under RubyVM
.
This is just my opinion. It is all right if matz accepted this.
OK, I'll assign to him and add to the developer meeting's agenda.
Updated by Eregon (Benoit Daloze) over 5 years ago
byroot (Jean Boussier) wrote:
This is kinda tengential, so sorry if it's shifting the discussion. But if
resolve_feature_path
is to be made a first class public API, I wonder if it could be the occasion to makeKernel#require
invokeKernel#resolve_feature_path
under the hood.
Could you file a separate feature request and show or explain how bootsnap would use it?
Updated by Eregon (Benoit Daloze) over 5 years ago
deep-cover might be interested by this too, cc @marcandre (Marc-Andre Lafortune)
Updated by mame (Yusuke Endoh) over 5 years ago
Eregon (Benoit Daloze) wrote:
mame (Yusuke Endoh) wrote:
However, we can't be too careful to add anything to
Kernel
nowadays.I propose only as a class method, not an instance method
Oh sorry I missed the point. Fair enough. I'll ask matz's opinion at the next meeting.
At least, I don't want to do that until we receive an actual request to make the method available in production.
We very rarely receive this, e.g., even for RubyVM::InstructionSequence which is now used in production (bootsnap).
I think it is not a good criteria, it's just too easy to useRubyVM
in user code.
Let's try to remove it and see how many people are killed. (Joke)
Updated by nobu (Nobuyoshi Nakada) over 5 years ago
How about $LOAD_PATH.resolve_feature_path
?
Updated by Eregon (Benoit Daloze) over 5 years ago
nobu (Nobuyoshi Nakada) wrote:
How about
$LOAD_PATH.resolve_feature_path
?
As a singleton method, and because $LOAD_PATH
cannot be re-assigned?
It's a fun idea, although I suspect this will be very hard to find and document properly, so I think it's better on Kernel
as a class method.
Updated by mame (Yusuke Endoh) over 5 years ago
This ticket was discussed at yesterday dev meeting. Currently there is no singleton method to Kernel
, so some people were reluctant. Nobu counterproposed $LOAD_PATH
as above, and matz said he waits for eregon's response to the counterproposal.
Updated by Eregon (Benoit Daloze) over 5 years ago
Thanks for discussing the issue at the meeting.
I think having singleton-only methods on Kernel
would be OK, and probably most of us agree having the instance method is not warranted for a rarely-used method.
Kernel
makes sense to me, because it's where require
and load
are defined, and resolve_feature_path
is a subset of those methods.
My main concern with $LOAD_PATH
is documentation (e.g., where would it be listed on https://docs.ruby-lang.org/, there is no class, globals.rdoc
doesn't seem right) and discoverability (hard to find it when searching for it), but otherwise it sounds fine.
Another concern with defining it on $LOAD_PATH
is resolve_feature_path
does not depend only on $LOAD_PATH
but also on other values such as Dir.pwd
, and maybe other things if the feature lookup changes behavior in the future.
Dear @matz (Yukihiro Matsumoto), could you decide between Kernel
and $LOAD_PATH
?
I would be happy with either, and I believe that would be much better than RubyVM
(as explained in the description).
Updated by matz (Yukihiro Matsumoto) over 5 years ago
I vote for $LOAD_PATH.resolve_feature_path
. We need to improve the documentation as well.
Matz.
Updated by nobu (Nobuyoshi Nakada) over 5 years ago
- Status changed from Open to Closed
Applied in changeset git|d77b84ca82e1cef10ef06776a207151ef864b3ca.
$LOAD_PATH.resolve_feature_path
Moved from RubyVM. [Feature #15903]
Updated by Eregon (Benoit Daloze) over 5 years ago
Thanks for the decision, and thanks to @nobu (Nobuyoshi Nakada) for already moving the method.
I noticed the documentation is still on RubyVM, I'll try to fix that.
Updated by Eregon (Benoit Daloze) over 5 years ago
I documented the new method in globals.rdoc and added a NEWS entry.