* Optimize SourceBuffer line and column handling

[marcandre]

#line_begins:

• store line_begin instead of [line_begin, index]
• simplify code by storing an additional "virtual" line_begin at the end

caches for position => line / column: Use same cache instead of two.

#decompose_position: use cache there too

I added tests for behavior of some methods with out-of-range arguments.
Behavior arguably less bad after this PR.

[marcandre]

Prelude to another PR to improve behavior of SourceBuffer when frozen, with the target of making Nodes Ractor-shareable.

[iliabylich]

Looks good overall, thanks!

[iliabylich]

👍

[JoshCheek]

I read this code for a long time and decided that the memoization of @line_range is not a bug. But, FWIW, memoizing an ivar based on a parameter to a method feels quite precarious. Esp given that it's only conditionally defined on old Rubies, so it'd be easy for subtle bugs to escape notice.

Anyway, it seems like it's not an issue since it's ultimately calculated from @source, which will raise an error if set twice.

That said, nothing verifies that it was set the first time. So the API does have ways it can be used, which are incorrect. eg:

# Versions
$ ruby -r parser -ve 'puts Parser::VERSION'
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [arm64-darwin20]
3.0.1.1


# Obvious way of using the lib explodes
$ ruby -r parser -e '
  p Parser::Source::Buffer.new("some_file.rb").line_for_position(1)
'
Traceback (most recent call last):
	3: from -e:2:in `<main>'
	2: from /Users/josh/.gem/ruby/2.7.2/gems/parser-3.0.1.1/lib/parser/source/buffer.rb:223:in `line_for_position'
	1: from /Users/josh/.gem/ruby/2.7.2/gems/parser-3.0.1.1/lib/parser/source/buffer.rb:332:in `line_index_for_position'
/Users/josh/.gem/ruby/2.7.2/gems/parser-3.0.1.1/lib/parser/source/buffer.rb:320:in `line_begins': undefined method `index' for nil:NilClass (NoMethodError)


# This is how the code seems intended to be used:
$ ruby -r parser -e '
  p Parser::Source::Buffer.new("some_file.rb").tap { |b| b.source = "" }.line_for_position(1)
'
1

I assume we don't want to break the API in backwards incompatible ways, which is totally understandable. Or perhaps there's a use-case that I'm ignorant to.

But, if the option is available and reasonable, then I'd advocate making source= (here) and raw_source= (here) private, and changing #initialize's source keyword to be required (here). Then we could treat it like a value object and the interface to this class would become fairly obvious and fairly obviously correct.

eg:

$ ruby -r parser -e '
  p Parser::Source::Buffer.new("some_file.rb", source: "").line_for_position(1)
'
1

Cheers 🍻

[marcandre]

@JoshCheek Note that you can now pass source: to Parser::Source::Buffer.new. It should have been mandatory imo, but can not be to maintain compatibility...

[JoshCheek]

Aye 👍 I think this was released on a major version bump (I found this PR by looking through the changelog), but as that's past now, we probably can't make that change I mentioned.