Skip to content

Implement StringScanner for TruffleRuby in pure Ruby#195

Open
eregon wants to merge 4 commits intoruby:masterfrom
eregon:add-new-truffleruby-implementation
Open

Implement StringScanner for TruffleRuby in pure Ruby#195
eregon wants to merge 4 commits intoruby:masterfrom
eregon:add-new-truffleruby-implementation

Conversation

@eregon
Copy link
Member

@eregon eregon commented Mar 9, 2026 •
edited
Loading

@eregon eregon requested a review from kou March 9, 2026 20:12
@eregon eregon mentioned this pull request Mar 9, 2026
Open
@eregon eregon force-pushed the add-new-truffleruby-implementation branch 3 times, most recently from ca6ba18 to 3fe9dbe Compare March 9, 2026 20:22
eregon
eregon commented Mar 9, 2026
View reviewed changes
test/strscan/test_stringscanner.rb
if RUBY_ENGINE == 'truffleruby' && RUBY_ENGINE_VERSION.to_i < 34
omit 'not supported on TruffleRuby < 34 due to using strscan from stdlib'
end
end
Copy link
Member Author

@eregon eregon Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be useful for #193.
It's not practical to implement new methods on TruffleRuby < 34, but it's easy to do so on TruffleRuby >= 34 by modifying lib/strscan/truffleruby_strscan.rb.

Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can we drop support for TruffleRuby < 34?
Do we need to run tests with TruffleRuby < 34 in this repository because TruffleRuby < 34 doesn't use implementation in this repository.

@eregon eregon force-pushed the add-new-truffleruby-implementation branch from 3fe9dbe to 87bc55e Compare March 9, 2026 20:28
@eregon eregon mentioned this pull request Mar 9, 2026
Merged
@eregon
Implement StringScanner for TruffleRuby in pure Ruby
bc0fd1e 
* Fixes ruby#194.
* This is a fresh new implementation from scratch,
  contributed directly to ruby/strscan, under BSD-2-Clause.
  This was implemented using the strscan tests,
  specs from ruby/spec and the documentation (https://docs.ruby-lang.org/en/master/StringScanner.html).
* lib/strscan.rb now handles the loading for all implementations.
* Test on TruffleRuby 33 to ensure it keeps working on older TruffleRuby releases,
  which do not have the necessary Primitives used by this new implementation.
@eregon eregon force-pushed the add-new-truffleruby-implementation branch from 87bc55e to bc0fd1e Compare March 9, 2026 20:37
@eregon
Copy link
Member Author

eregon commented Mar 9, 2026

I extracted the documentation fix to #196 since there is no dependency on it and it's an orthogonal change.

@brixen
Copy link

brixen commented Mar 9, 2026

@eregon IANAL but my understanding is that if you've had exposure to the licensed code, it's pretty hard to claim "rewrote from scratch".

I understand the difficulty of relicensing code from experience, but I also respect that contributors can require people to respect their IP rights for whatever reason they choose.

@eregon
Copy link
Member Author

eregon commented Mar 9, 2026 •
edited
Loading

I get your point (though see e.g. this on the subject), but it is a fresh new implementation from scratch (i.e. starting with an empty file) that I wrote between Sun Mar 8 20:26:51 2026 +0100 and Mon Mar 9 21:37:13 2026 +0100 (commit timestamps of bc0fd1e).
I haven't had a single look at that code from rubysl-strscan while implementing and only used the tests, specs and docs.
To find the correct Primitive names I looked at the list of @Primitive in the TruffleRuby Java code.

FWIW it's probably not even possible to get the list of contributors to rubysl-strscan since that repository no longer exists on GitHub (https://github.com/rubysl is empty and https://github.com/search?q=rubysl-strscan&type=repositories gives no results). Anyway, I'm not looking to relicense with so many contributors and complications. The new implementation is likely better anyway by just matching current StringScanner behavior and not having to worry about past behavior.

kou pushed a commit that referenced this pull request Mar 10, 2026
@eregon
Fix documentation of StringScanner#skip_until (#196)
3dc627d 
I noticed this while working on #195.
@kou
Copy link
Member

kou commented Mar 10, 2026

If implementing by eregon may have any problem for licensing, I can implement this. I haven't seen the rubysl-strscan implementation nor the current implementation in TruffleRuby.

@eregon
Copy link
Member Author

eregon commented Mar 10, 2026

There is no problem, as I said I have implemented this carefully based on the tests/specs/docs alone.
I would very much like to use this new clean implementation, could you review it?

If it's helpful I could merge this new implementation in truffleruby/truffleruby first, and delete the rubysl-strscan-based implementation in truffleruby/truffleruby, let me know if you prefer that.

@kou
Copy link
Member

kou commented Mar 11, 2026

@brixen Do you have any more comments? Can we proceed this with eregon's implementation?

@eregon
Copy link
Member Author

eregon commented Mar 12, 2026 •
edited
Loading

If anyone wants to compare, this is the original rubysl-strscan code and this is the new implementation.
They have basically nothing in common besides implementing a similar set of methods (since they both implement the StringScanner API). They have very different state, approach, structure, style, etc.
So I think it should be obvious to everyone there is no IP concern there.

And this is the current implementation of strscan in truffleruby. The state, approach, structure, style, etc are clearly different. As an example the implementation of scan-related methods is very different (and that's the major and key part of a StringScanner implementation). Both use the same public TruffleRuby Primitive's of course, and that's expected as they are TruffleRuby's API to implement things which are not available as regular Ruby methods (such as matching /\A/ at the current position instead of at the start of the String).

@eregon
Copy link
Member Author

eregon commented Mar 14, 2026

@kou If you prefer to reimplement it yourself, that's fine by me, as long as I can review it for performance and simplicity (minimal state is best).
Of course from my POV it is unnecessary and this implementation is fine as-is, but do as you prefer.
If you're hesitating maybe ask someone else to look at the implementations (linked in #195 (comment)).

If you decide to reimplement it, here are some useful tips:

  • This is the documentation about public TruffleRuby Primitives, some of them are necessary to implement StringScanner
  • For tests, bundle exec rake as usual
  • For specs, cd /path/to/ruby/spec; RUBYOPT=-I/.../strscan/lib mspec spec/ruby/library/stringscanner

What I did is get the tests passing, then the specs.
I used the docs exclusively, not strscan.c. I would recommend to do the same, strscan.c is 2300 lines long vs 440 for this implementation, and in my experience translating from C to Ruby is quite some work to refactor it to idiomatic Ruby and tends to leave unnecessary conditions originating from the C implementation. Using mostly the docs and looking up the C implementation when docs are unclear is probably pragmatic though.

@kou
Copy link
Member

kou commented Mar 15, 2026

Let's wait for at least a few weeks before we proceed this. It's for collecting comments from others.

eregon added a commit to truffleruby/truffleruby that referenced this pull request Mar 19, 2026
@eregon
Reimplement StringScanner from scratch
e21643c 
* This is a fresh new implementation from scratch under BSD-2-Clause.
  This was implemented using the ruby/strscan tests, specs from ruby/spec
  and the official documentation (https://docs.ruby-lang.org/en/master/StringScanner.html).
* No AI involved, I fully wrote this myself on March 8-9 2026.
* This implementation uses minimal state and has good performance.
* Originally contributed to ruby/strscan#195
  but given slow progress it will be merged in truffleruby/truffleruby first.
@eregon eregon mentioned this pull request Mar 19, 2026
Merged
kou
kou reviewed Mar 26, 2026
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection. Let's proceed this with the eregon's implementation.

lib/strscan/truffleruby_strscan.rb
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove _strscan suffix because it's redundant?

lib/strscan/truffleruby_strscan.rb

def rest = @string.byteslice(@pos..)

def rest_size = rest.bytesize
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@string.bytesize - @pos will be faster.

lib/strscan/truffleruby_strscan.rb
alias_method :bol?, :beginning_of_line?

def eos?
raise ArgumentError, 'uninitialized StringScanner object' unless @string
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this check? It seems that @string must not be nil.

lib/strscan/truffleruby_strscan.rb
def matched = @last_match&.to_s

def [](group)
raise TypeError, 'no implicit conversion of Range into Integer' if Primitive.is_a?(group, Range)
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this check? Does @last_match[group] already have this check?

lib/strscan/truffleruby_strscan.rb
end

def scan_byte
if rest?
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using return nil if eos? like get_byte does?

lib/strscan/truffleruby_strscan.rb
if rest.start_with?(pattern)
to = prev + pattern.bytesize
@last_match = Primitive.matchdata_create_single_group(pattern, @string, prev, to)
to - prev
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works but pattern.bytesize may be better for readability.

Suggested change
to - prev
pattern.bytesize

lib/strscan/truffleruby_strscan.rb
if from = @string.byteindex(pattern, prev)
to = from + pattern.bytesize
@last_match = Primitive.matchdata_create_single_group(pattern, @string, from, to)
@string.byteslice(prev, to - prev)
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use pattern instead? I think that pattern is faster than @string.byteslice.

lib/strscan/truffleruby_strscan.rb
end
Primitive.always_split self, :search_full

# Keep the following 8 methods in sync, they are small variations of one another
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you implement these methods separately instead of implementing it in scan_full/search_full like the C implementation does? There are many duplicated code in these methods.

Performance? How much faster?

lib/strscan.rb
require 'strscan'
end
else
raise "Unknown Ruby: #{RUBY_ENGINE}"
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using NotImplementedError?

test/strscan/test_stringscanner.rb
if RUBY_ENGINE == 'truffleruby' && RUBY_ENGINE_VERSION.to_i < 34
omit 'not supported on TruffleRuby < 34 due to using strscan from stdlib'
end
end
Copy link
Member

@kou kou Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can we drop support for TruffleRuby < 34?
Do we need to run tests with TruffleRuby < 34 in this repository because TruffleRuby < 34 doesn't use implementation in this repository.

@kou
Copy link
Member

kou commented Mar 26, 2026

Could you also update

strscan/Rakefile

Lines 9 to 23 in 4243751

task :bump do
strscan_c_path = "ext/strscan/strscan.c"
strscan_c = File.read(strscan_c_path).gsub(/STRSCAN_VERSION "(.+?)"/) do
version = $1
"STRSCAN_VERSION \"#{version.succ}\""
end
File.write(strscan_c_path, strscan_c)
strscan_java_path = "ext/jruby/org/jruby/ext/strscan/RubyStringScanner.java"
strscan_java = File.read(strscan_java_path).gsub(/STRSCAN_VERSION = "(.+?)"/) do
version = $1
"STRSCAN_VERSION = \"#{version.succ}\""
end
File.write(strscan_java_path, strscan_java)
end
?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kou kou kou left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add strscan implementation for TruffleRuby in this repository

3 participants

@eregon @brixen @kou