Allow transition from end states to self with EOS as the only token allowed

[viktor-ferenczi]

Fixes #605

[viktor-ferenczi]

@rlouf Simple crash fix

[lapp0]

LGTM, makes sense, haven't smoke tested. Would prefer a test case which fails before this change and passes after though.

[viktor-ferenczi]

A simpler regex to reproduce: r'`\n(\.\n)?`\n'

In this case state 5 is missing from states_to_token_maps, while 6 exists.

The regression test will verify the resulting states_to_token_maps.

[viktor-ferenczi]

Added a regression test case, which fails before the fix: test_regression_regex_missing_final_state

Apparently test_regex has to be fixed to expect the final state in states_to_token_maps. I think that test case was wrong, that's why the bug wasn't detected originally.

Please note that it is much cleaner if we use visibly different token IDs from the state numbers, like token IDs above 100 in the case of my test. This makes the values in states_to_token_maps much more readable in tests.

With the fix applied test_regex_vocabulary_error fails, because it does not get the expected exception. I think the following check is fishy in RegexFSM.__init__:

            # We make sure that it is possible to generate strings in the language
            # of the regular expression with the tokens present in the model's
            # vocabulary.
            if not any(
                regex_fsm.finals.intersection(v.values())
                for v in states_to_token_maps.values()
            ):
                raise ValueError(
                    "The vocabulary does not allow us to build a sequence that matches the input regex"
                )

I think such tests should not remain in the production code, but go into test cases.

Also, the cacheable_vocabulary argument of the create_states_mapping inner function is not used at all, which seems to point to unfinished code.

[rlouf]

This should be covered by this function. We should understand why it isn't in your case.

[rlouf]

I can get on board with this, but if I understand well we should be able to remove the if state in self.final_states: return [eos_token_id] in RegexFSM?

[rlouf]

I think the core issue is that a recent change removed the actual FSM final states from RegexFSM's set of final states. This must have caused the issue that you observed. I will revert this change and add your test case to see if it runs. I will also add a test case for when a regex leads to a FSM with several final states.

[rlouf]

Should be fixed by #734. Closing for now.