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

[prototype runtime] Add --autoload option #1561

Merged
merged 2 commits into from Oct 16, 2023
Merged

Conversation

ksss
Copy link
Collaborator

@ksss ksss commented Oct 5, 2023

I've added the --autoload option to include objects from autoload destinations as analysis targets.

rbs prototype runtime has an issue where it cannot read from autoload destinations, unlike rbs prototype rb. To resolve this, I've added an option to forcibly load Module#autoload.

Using TracePoint, it target Module#autoload to hook and retrieve its arguments, which are then loaded later. There is an issue where nested autoload cannot be read, but in most cases, it resolves the problem.

Since it's using the target option of TracePoint#enable, there's hardly any performance degradation.

Example

before

$ bundle exec rbs prototype runtime -r rack 'Rack::*' 2>/dev/null | wc
       4       6      36

after

$ bundle exec rbs prototype runtime -r rack --autoload 'Rack::*' 2>/dev/null | wc
    2076    5331   44641

It allows objects from autoload destinations to be treated as analysis targets.
@soutaro
Copy link
Member

soutaro commented Oct 11, 2023

@ksss

Thank you for working for this issue. I have two questions:

  1. Should we load the autoloaded constants by default?
  2. It seems like enumerating all (possibly autoloaded) constants by Module#constants and Kernel.autoload? is a better implementation, but it requires a lot more rewrite in prototype/runtime.rb. What do you think?

@ksss
Copy link
Collaborator Author

ksss commented Oct 11, 2023

  1. Should we load the autoloaded constants by default?

Setting this option as default seems challenging, but it appears to be a good idea.
One risk is having to deal with unexpected errors, but most problems can be anticipated.

It seems like enumerating all (possibly autoloaded) constants by Module#constants and Kernel.autoload? is a better implementation, but it requires a lot more rewrite in prototype/runtime.rb. What do you think?

Indeed, it's possible to load autoload using the Module#constants method and then Object.const_get.
Furthermore, by running it recursively, it can handle nested autoloads.
It's a bit complicated, but I have implemented the same process before.

However, if there's no autoload at all, the concern is that unnecessary processing might degrade performance.
At the library level, it might be within the margin of error, but as the scale becomes as large as a Rails application, performance issues are more likely to arise.
We might need to narrow down the root for executing Module#constants based on the specified pattern.

Using the TracePoint method, if we specify the target with the enable method, there's hardly any performance degradation.

There are trade-offs.


Anyway, adding more features to the runtime will likely require significant rewriting.
For example, if we're using RBS::Prototype::Runtime as a library rather than from the CLI, as in this PR, the autoload loading feature won't be enabled. It's necessary to include the loading phase within the class.

@ksss
Copy link
Collaborator Author

ksss commented Oct 11, 2023

There's also a multi-step approach.

Step 1: Offer the --autoload option with a default of (true or false). Keep code changes to a minimum. Using TracePoint.
Step 2: Offer the --autoload option with a default of true. Undertake a major rewrite. Using Module#constants. Maybe for RBS v4?

@soutaro soutaro added this to the RBS 3.3 milestone Oct 12, 2023
@soutaro
Copy link
Member

soutaro commented Oct 12, 2023

Got it! Let's merge this PR for the next version of RBS. Using tracepoint, defaults to false.

@soutaro soutaro added this pull request to the merge queue Oct 16, 2023
Merged via the queue into ruby:master with commit 2ae959e Oct 16, 2023
23 checks passed
@ksss ksss deleted the autoload branch October 16, 2023 09:23
@soutaro soutaro added the Released PRs already included in the released version label Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

None yet

2 participants