Track class new in ConstantDefinition #1044
Conversation
|
Applied the suggested refactors and shortener the ConstantDefinition a bit. However, there are two tests that are now failing because certain modules are no longer generated for them. I'm not 100% sure what's going on. |
Also, it should be supplied the `caller_locations` so that we can process the caller locations without the calls to this method in between.
This allows dynamically created constants to be attributed to the
file that triggered their creation instead of the actual location that
created the module.
For example, given a file `foo.rb`:
```ruby
module Foo
class_methods do
# ...
end
end
```
we expect a `Foo::ClassMethods` module to be created when this file is
processed. That module will be created inside Active Support using
something like `const_set(:ClassMethods, Module.new)`, but that module
should be attributed to `foo.rb` where the `class_methods` call is
being made.
The `ActionController::Base::HelperMethods` module should never have been generated inside the `some_engine` gem RBI file in the first place.
ghost
added
the
| key = name_of(tp.self) | ||
| file = tp.path | ||
| lineno = tp.lineno | ||
| TracePoint.trace(:class, :c_return) do |tp| |
There was a problem hiding this comment.
This is entirely a suggestion, feel free to disregard - I personally find the single TracePoint with the case statement hard to parse. Some of the lines are pretty deeply nested, and the two cases don't really share any common functionality, aside from setting the key-value-pairs in @class_files.
What would you think about calling TracePoint twice, separating out the behavior for class and c_return, and then extracting @class_files[key] ||= Set.new ... into a shared method? I personally would find that much easier to read, but I don't think it's a huge deal either way.
(I'm also curious if there's any performance benefit to calling TTracePoint once?)
| TracePoint.trace(:class, :c_return) do |tp| | |
| TracePoint.trace(:class) do |tp| | |
| next if tp.self.singleton_class? | |
| if tp.path == "(eval)" | |
| caller_location = T.must(caller_locations) | |
| .drop_while { |loc| loc.path == "(eval)" } | |
| .first | |
| next unless caller_location | |
| loc = ConstantLocation.new( | |
| path: caller_location.absolute_path || "", | |
| lineno: caller_location.lineno | |
| ) | |
| else | |
| loc = build_constant_location(tp, caller_locations) | |
| end | |
| add_to_class_files(tp.self, loc) | |
| end | |
| TracePoint.trace(:c_return) do |tp| | |
| next unless tp.method_id == :new | |
| next unless Module === tp.return_value | |
| key = tp.return_value | |
| loc = build_constant_location(tp, caller_locations) | |
| add_to_class_files(key, loc) | |
| end |
There was a problem hiding this comment.
I like that option as well. The latest commit changed it.
Motivation
Based on @paracycle's work in this commit.
Start handling
c_returnto capture constants defined byConstant = Class.new. Currently, there's a bug in main where we can't generate RBIs including locations because these occurrences aren't tracked.After this PR, generating all RBIs in Tapioca with locations works.
Implementation
Something = Class.newdon't have names by the time we capture themc_returnin ConstantDefinition to capture these occurrencesStackLevelTooDeeperror that is caused by theNamePatch. As per @paracycle's suggestion, I inlined the implementation ofqualified_name_of, which fixes the recursion issueevent.constantinstead ofevent.symbolin the source location listener and stopped trying to add comments iflocationisnil(which can still happen for constant aliases)Tests
Added a test that does not pass without handling
c_return.