Fix anchors

[pyrmont]

Six years ago, the Rouge library author, @jneen, reported that there was a bug in the handling of anchors in StringScanner. At the time, StringScanner did not have a maintainer and the view was expressed that this behaviour was intended.

The Bug

In short, the bug is that anchors in regular expressions don't work properly in StringScanner. This can be seen in the sample code below (taken from the original bug report):

require 'strscan'
ss = StringScanner.new("ab")
ss.scan(/./)
#=> "a"
ss.scan(/^./) # expecting nil, since the head is in the middle of a line
#=> "b"

The Cause

The cause of the bug is the way strscan_do_scan() is implemented in ext/strscan/strscan.c. This function calls out to onig_match() and onig_search() like this:

strscan/ext/strscan/strscan.c

Lines 480 to 490 in cace20f

	if (headonly) {
	ret = onig_match(re, (UChar* )CURPTR(p),
	(UChar* )(CURPTR(p) + S_RESTLEN(p)),
	(UChar* )CURPTR(p), &(p->regs), ONIG_OPTION_NONE);
	}
	else {
	ret = onig_search(re,
	(UChar* )CURPTR(p), (UChar* )(CURPTR(p) + S_RESTLEN(p)),
	(UChar* )CURPTR(p), (UChar* )(CURPTR(p) + S_RESTLEN(p)),
	&(p->regs), ONIG_OPTION_NONE);
	}

The reason that the anchors don't work is because the second argument to each method is CURPTR(p). This tells Onigmo to treat the scan head as the beginning of the string to be searched. This is a mistake.

The Fix

It is clear from the method definitions for onig_match() and onig_search() (see here and here) that you can begin searching within a string by passing a pointer to the scan head as the fourth argument. The equivalent function used in the implementation of Regexp's search methods (eg. Regexp#match) takes this approach: passing different pointers for the start of the string and the location to begin searching.

We can do this in strscan_do_scan():

        if (headonly) {
            ret = onig_match(re, (UChar* )S_PBEG(p),
                             (UChar* )(CURPTR(p) + S_RESTLEN(p)),
                             (UChar* )CURPTR(p), &(p->regs), ONIG_OPTION_NONE);
        }
        else {
            ret = onig_search(re,
                              (UChar* )S_PBEG(p), (UChar* )(CURPTR(p) + S_RESTLEN(p)),
                              (UChar* )CURPTR(p), (UChar* )(CURPTR(p) + S_RESTLEN(p)),
                              &(p->regs), ONIG_OPTION_NONE);

However, this change is not enough and will cause various tests in StringScanner's test suite to fail. Because StringScanner calls onig_match() and onig_search() in the 'wrong way', it has to do various adjustments throughout the file to correct the values of p->regs set by Onigmo. Once we remove these adjustments, the tests pass.

The Result

This PR is not intended to be a breaking change. Although, it changes the implementation in ext/strscan/strscan.c, the Ruby API returns the same results as it did before the change. Instead, this PR increases functionality by allowing users to use anchors in regular expressions.

It also conforms with the principle of least surprise. A user of StringScanner should be able to use the same regular expressions they could use in Regexp#match and get the same results.

Hopefully that all makes sense but please let me know if I can provide any more information.

[pyrmont]

I need to update some tests. Will look into that.

[pyrmont]

Needs more work. Withdrawn for the moment.

[pyrmont]

I've updated the OP with a better explanation of the bug and how this fix works.

[pyrmont]

Any response to this? It fixes a long-standing bug, passes all tests, is there any other information I can provide?

[kou]

I'm neutral for this change.
Could you show a real world use case?

[jneen]

Yes! As was mentioned, we have to hack around this in rouge because StringScanner cannot tell when the beginning of a line is. See

https://github.com/rouge-ruby/rouge/blob/master/lib/rouge/regex_lexer.rb#L299
https://github.com/rouge-ruby/rouge/blob/master/lib/rouge/regex_lexer.rb#L20

We basically have to look at the string representation of every regular expression, guess whether it starts with ^, and manually check #beginning_of_line?. This of course is not a general solution, as things like /(^...|...)/ are possible.

[kou]

Thanks.
I accept the improvement.
I'll review the implementation later.

[kou]

I've merged with some modifications.
Thanks.

[pyrmont]

One minor comment:

[jneen]

Apologies for posting on a closed pull request, but I believe this is missing a test. The new test only tests the happy case - that /^a/ matches when the scanner is at the beginning of the line. A test like:

s = StringScanner.new("ab\nc")
assert_equal 1, s.skip(/^a/)
assert_nil s.skip(/^b/)
assert_equal 2, s.skip(/b\n/)
assert_equal 1, s.skip(/^c/)

would resolve this by actually testing that /^b/ fails to match because it is in the middle of a line.

[jneen]

(I am happy to open a PR for the above)

[kou]

I think that https://github.com/ruby/strscan/pull/6/files#diff-490aef8dd6acb090e22d2b9c436f5469R326 is a "not happy case".
What is difference of them? Should we need not only \A case but also ^ case?

[jneen]

Unfortunately the example you referenced tests the behavior of \A, not the behavior of ^. In fact, the example there should in fact succeed if ^ is used, since the scanner state is at the beginning of a line.

The original bug caused the scanner head to be not only an anchor for \A but also an anchor for ^ - since this behavior is changing I think it is necessary to have a test in which we expect ^ to not match. I believe a test like this will be helpful in porting this change correctly to other Ruby implementations.

[kou]

I don't understand why you say the \A case doesn't check the scanner head. \A depends on the scanner head.

require "strscan"
s = StringScanner.new("a\nb")
p s.skip(/a\n/)
p s.skip(/\Ab/)

Without this change:

2
1

With this change:

2
nil

But I don't oppose your suggestion strongly. Because the new test will not increase test time and mainteinance cost so much.
You can open a pull request to add a new test case. But please add a new test method instead of changing existing one.

[jneen]

I will open a pull request. For posterity, this change makes \A match only the beginning of the entire stream and not wherever the scanner happens to be, as per your example, is that correct?

In this case, I am suggesting we test that ^ is also now unaffected by the scanner head, since it definitely was before. I believe this will make it easier to understand when porting.