Rakudo Issue 1622: Fix bug with iteration of empty character class

[jstuder-gh]

Check that there is at least one character class before applying the
regex nodes.

When compiling the 'conj' regex node, it expects to be able to shift
off at least one child node and iterates past the end as a result.

Fixes Rakudo Issue #1622

[lizmat]

Looks good to me.

[jstuder-gh]

Ok then if there are no objections I will merge this in tonight.

[jnthn]

I think this should probably give an error rather than work; surely nobody ever intends to write an empty character class.

[AlexDaniel]

But <[]> does not give an error, so this PR seems to be consistent.

[jstuder-gh]

I experimented with adding an error message like so (see this commit), but then NQP would not build without displaying that error so I backed off from that.

I can try experimenting a bit more with it tonight, but that was the reason I submitted it without the error. Looking at it again, that isn't the best place for that assertion (as it's only checking the first character class), so maybe it could work but is just poorly placed.

[jstuder-gh]

I've updated the branch so that it will panic if it encounters an empty char class (<[ ]>, <+[ ]>, or <-[ ]>). I had to add the check for whether the rxtype is 'cclass'; cclass_backslash nodes have no children and were causing the NQP build to fail.

When running the spectest these tests cause it to die and thus fail. Do we modify the tests in this case?

[jstuder-gh]

Looking at the RT thread related to the tests that fail with the panic added, it looks like the empty character class is an intentional thing. I guess I'll remove the panic commit? That looks to be the proper behavior.

[AlexDaniel]

These tests are part of 6.c-errata. I can't say if it's a “proper” behavior or not, but it is what it is in v6.c. We can easily merge this without the panic commit, or we can merge if it panics in v6.d only.

Well, unless someone can justify why the current behavior is very wrong.

[jstuder-gh]

How about I remove the panic commit from this branch, reapply on another branch, and submit a separate PR or issue inviting comment on whether the panic is appropriate to include in v6.d? That way we can merge in the sure thing and defer the panic question for now.

Can we specify in the roast that certain blocks adhere to a particular standard (ie lexically scoped 'use v6.*' statements)? So if we add a test to indicate that the <-[ ]> does not error now, and if the panic behavior is adopted for v6.d we can clarify the old tests apply to v6.c and make new tests that apply only to v6.d?

[AlexDaniel]

@jstuder-gh Your patch should work both under v6.c and v6.d at the same time, doing the “right” thing depending on which version is used (e.g. use v6.c vs use v6.d.PREVIEW).

Yes, please split it in two and we'll discuss the second part separately.

[jstuder-gh]

@AlexDaniel I re-pushed the branch but this time without the panic commit. I will resubmit that commit as a separate PR soon.