Rename current completor to RegexpCompletor and refactored for future extension

[tompng]

Current completion will be renamed to RegexpCompletor and another type of completor will be added in the future.

Completor needs to implement

class FoobarCompletor < BaseCompletor
  def completion_candidates(preposing, target, postposing, bind:)
    # input: preposing="puts(", target="2.a", postposing=")"
    # output: ['2.abs', '2.allbits?']
  end

  def doc_namespace(preposing, matched, postposing, bind:)
    # input: preposing="puts(", target="2.abs", postposing=")"
    # output: 'Integer.abs'
  end
end

IRB::InputCompletor::CompletionProc IRB::InputCompletor::PerfectMatchedProc IRB::RelineInputMethod::SHOW_DOC_DIALOG is removed and refactored.

## Requirements These utility methods is needed in each completor so leave it in InputCompletor BaseCompletor. - BaseCompletor.retrieve_files_to_require_from_load_path - BaseCompletor.retrieve_files_to_require_relative_from_current_dir

The calculation order is:

1. First, completion candidates is calculated from target, preposing, postposing and binding.
2. After that, display_document or doc_namespace calculation(show document in dialog) is performed from matched(≒target). preposing and postposing is not available in this phase.

For advanced completor, display_document and doc_namespace needs preposing, postposing and binding information. Each completor implementation can cache them in completion_candidates phase, but I want to encapsulate it in InputCompletor so that each completor implementation doesn't need to know about calculation order hack.

Displaying document logic is not completor specific. It should be implemented in InputCompletor by using the result of completor.doc_namespace(matched).

Conclusion

Each completor implements completion_candidates and doc_namespace method.
Instance of completor is created just before completion_candidates calculation and reused in doc_namespace calculation.
Completor is initialized with target, preposing, postposing and bining.

In completion calculation phase, InputMethod calls Completor.new(target, preposing, postposing, bind:) and completor.completion_candidates(). RelineInputMethod will cache the instance.

In document namespace calculation phase, RelineInputMethod calls completor.doc_namespace(target)

display_document logic (only for noautocomplete mode) is moved from InputCompletor::PerfectMatchedProc to RelineInputMethod#display_document. It uses completor.doc_namespace(matched)

Completor needs to implement

class FoobarCompletor < BaseCompletor
  def initialize(target, preposing, postposing, bind:); end
  def completion_candidates(); end
  def doc_namespace(matched); end
end

Example of parameters passed for code if 1.ab

Parameters passed to constructor are target='1.ab', preposing='if ', postposing='' and binding.
completor.completion_candidates returns ['1.abs', '1.abs2'].
matched passed to doc_namespace is '1.abs' or '1.abs2'. Not '1.ab'.

[st0012]

I don't see any benefit of having these retrieve_* methods as class methods. Let's define these methods as instance methods too so we can simply call it here and memoize it on this instance.

[tompng]

Since completor is created many times, memoizing in instance will make this cache disappear.

[st0012]

What's the benefit of 1) initialising a new completer in every new invocation over 2) initialising it once and pass required arguments to it (like #711)?

IMO, using 2) would make memoizing easier to maintain and understand as we can cache the files in its ivars.

Also, using parent class' class methods for caching feels a bit anti-pattern to me 🤔

[tompng]

An example of not recreating a completor instance should be like this:

# Full spec. Reset when binding is modified.
class Completor
  def reset_cache
    # Invoked when binding.eval(something) is executed in IRB.
    # All cache should be invalidated because lvars, ivars and other might be changed.
    # Calculation result might change even if `binding` `preposing` `target` `postposing` is the same value.
    # For example, `Readline=Reline` will change the completion canidates of `Readline.a`
  end

  def completion_candidates(preposing, target, postposing, binding)
    cache.fetch([preposing, target, postposing]) { calculate_candidates }
  end

  def doc_namespace(preposing, matched_target, postposing, binding)
    cache.fetch([preposing, target, postposing]) { calculate_doc }
  end
end

# Simplified spec. Reset when one of preposing, target, postposing is changed or binding is modified.
class Completor
  def reset_and_cache(preposing, target, postposing, binding)
    # Always invalidate old cache and cache the parameters
  end

  def completion_candidates
    # Use cached value or recalculate from cached parameters
  end

  def doc_namespace(matched_target)
    # Use cached value or recalculate from cached parameters
  end
end

class Completor
  def completion_candidates(preposing, target, postposing, binding)
    # Should always cache, always invalidate old cache
  end

  def doc_namespace(matched_target)
    # Should use the cache
  end
end

class Completor
  def completion_candidates(preposing, target, postposing, binding)
    # Don't cache
  end

  def doc_namespace(preposing, matched_target, postposing, binding)
    # Always recalculate
  end
end

For reset_cache() and reset_and_cache(value_to_cache) pattern, I think recreating an instance is better than implementing reset for an object.
For caching-in-completion_candidates pattern, there are implicit rules that completion_candidates should always invalidate old cache, doc_namespace can always use the cache.

Recreating an instance instead of resetting an instance will simplify it.

[st0012]

I don't see any "production" use case for resetting cache. If it's simply for testing, I think we can just memoise with ivar and recreate instances in test teardown. WDYT?

[tompng]

Reset is for production. not for test.

irb(main):001> a=apple=1
#=> 1
irb(main):002> a            # complete('a','','')        #=> ["a", "apple", "at_exit", ...]
irb(main):002> a.           # complete('a.','','')       #=> ["a.abs", "a.allbits?", ...]
irb(main):002> a.t          # complete('a.t','','')      #=> ["a.to_s", "a.to_int", ...]
irb(main):002> a.to_s       # doc('a.to_s')              #=> "Integer#to_s"
irb(main):002> a=:a;a.t     # complete('a.t','a=:a;','') #=> ["a.to_s", "a.to_sym", "]
irb(main):002> a=:a;a.to_s  # doc('a.to_s')              #=> "Symbol#to_s"
irb(main):002> a=avocado='!'
#=> "!"                     # reset
irb(main):003> a            # complete('a','','')        #=> ["a", "apple", "avocado", ...]
irb(main):003> a.           # complete('a.','','')       #=> ["a.ascii_only?", "a.b", ...]
irb(main):003> a.t          # complete('a.t','','')      #=> ["a.to_s", "a.to_str", ...]
irb(main):003> a.to_s       # doc('a.to_s')              #=> "String#to_i"

Reset (or re-initialize) is needed because:
After calculating completion, doc('a.to_s') will change from "Integer#to_s" to "Symbol#to_s".
After evaluating input, complete('a','','') will change from ["a", "apple", "at_exit", ...] to ["a", "apple", "avocado", ...]

However, I think we can first implement with not recreating instance, and consider whether to change later. What do you think?

[st0012]

Sorry I think I didn't specify what I meant by caching.
I think to match the current behaviour, we only need to cache the result of retrieve_files_to_require_from_load_path in ivars?

So I prefer recreating instances with this spec:

class Completor
  def completion_candidates(preposing, target, postposing, binding)
    # Don't cache
  end

  def doc_namespace(preposing, matched_target, postposing, binding)
    # Always recalculate
  end
end

[tompng]

👍
I'll update this pull request with that later

[st0012]

Why can't we simply do completor = @completor and call it from the proc?

[tompng]

It is needed to get the latest completor value.

Completor is created every time completion_candidates is calculated and reused only for doc_namespace calculation.
This is to make cache lifetime of [preposing, target, postposing] to match completor instance lifetime and make completor's implementation simple.

[st0012]

Nice extraction 👍
I'd like to store the driver in RelineInputMethod because it doesn't need to be initialised every time we show the doc, and memoise it with lazy initialisation makes testing complicated.

[tompng]

For performance, I think storing driver is better but not important here.
RDoc::RI::Driver.new takes 0.18sec for the first time and after that, takes 0.035sec.
In here, driver is only initialized when user press TAB key twice to explicitly show document in noautocomplete mode. I think 0.035sec is acceptable.

The problem is more with document dialog of autocomplete mode. It does not cache driver and it takes each 0.035sec while scrolling completion dialog by TAB key.

SHOW_DOC_DIALOG calling RDoc::RI::Driver.new (in branch master)

irb/lib/irb/input-method.rb

Lines 291 to 293 in 1d2d35d

	options = {}
	options[:extra_doc_dirs] = IRB.conf[:EXTRA_DOC_DIRS] unless IRB.conf[:EXTRA_DOC_DIRS].empty?
	driver = RDoc::RI::Driver.new(options)

Creating RDoc::RI::Driver.new at startup will slowdown IRB's startup time, so lazily initialize it and use it from both show_doc_dialog_proc and display_document might be better. (maybe after this pull request?)

[st0012]

Good catch on the performance penalty of initialising it repeatedly in dialog 👍
That said, I don't think 0.035 sec at the startup time would be too big of an issue. Storing it in the constructor makes the dependency easy to understand, which outweighs the minor delay IMO.

But I agree we should deal with this in another PR, and also let's do a proper profiling on IRB startup?

[st0012]

Almost there, just 1 question left 👍

[st0012]

Why don't we store the completor in ivar for ReadlineInput as well?

[tompng]

Nice catch! done

[st0012]

Good catch on the performance penalty of initialising it repeatedly in dialog 👍
That said, I don't think 0.035 sec at the startup time would be too big of an issue. Storing it in the constructor makes the dependency easy to understand, which outweighs the minor delay IMO.

But I agree we should deal with this in another PR, and also let's do a proper profiling on IRB startup?

[st0012]

Thanks for the great work 👏