Skip to content

Commit c671d39

Browse files
committed
Don't lazily retrieve gem specs for completion
There are a few downsides of the current approach: 1. Because gem specs are lazily retrieved, this computation happens in every irb completion test case, which is not necessary. (In tests we don't cache the result of `retrieve_files_to_require_from_load_path`) 2. Gem::Specification.latest_specs is sensible to the content of LOAD_PATH. And when combined with 1, tests fail "randomly" if they try to mutate LOAD_PATH, even though the test subject it's something else. So by pre-computing and storing the gem paths in a constant, it guarantees that the computation only happens once and it doesn't get affected by test cases. One argument could be made against the change is that, it'll store unnecessary data for users that disable autocompletion. But the counter-arguments are: 1. Since autocompletion is enabled by default, this should not be the case for most users. 2. For users with autocompletion enabled, IRB already caches the result of `retrieve_files_to_require_from_load_path` in memory, which should have a similar size of GEM_SPECS. And we currently haven't received any report about problems caused by such memory consumption.
1 parent 0ec18f0 commit c671d39

File tree

1 file changed

+17
-15
lines changed

1 file changed

+17
-15
lines changed

lib/irb/completion.rb

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -64,25 +64,27 @@ def self.absolute_path?(p) # TODO Remove this method after 2.6 EOL.
6464
if File.respond_to?(:absolute_path?)
6565
File.absolute_path?(p)
6666
else
67-
if File.absolute_path(p) == p
68-
true
69-
else
70-
false
71-
end
67+
File.absolute_path(p) == p
7268
end
7369
end
7470

71+
GEM_PATHS =
72+
if defined?(Gem::Specification)
73+
Gem::Specification.latest_specs(true).map { |s|
74+
s.require_paths.map { |p|
75+
if absolute_path?(p)
76+
p
77+
else
78+
File.join(s.full_gem_path, p)
79+
end
80+
}
81+
}.flatten
82+
else
83+
[]
84+
end.freeze
85+
7586
def self.retrieve_gem_and_system_load_path
76-
gem_paths = Gem::Specification.latest_specs(true).map { |s|
77-
s.require_paths.map { |p|
78-
if absolute_path?(p)
79-
p
80-
else
81-
File.join(s.full_gem_path, p)
82-
end
83-
}
84-
}.flatten if defined?(Gem::Specification)
85-
candidates = (gem_paths.to_a | $LOAD_PATH)
87+
candidates = (GEM_PATHS | $LOAD_PATH)
8688
candidates.map do |p|
8789
if p.respond_to?(:to_path)
8890
p.to_path

0 commit comments

Comments
 (0)