Many parser should not handle infinite loop #198
Conversation
1 similar comment
|
I really like this! It could be backwards incompatible though, if someone uses a parser with side effects in it. I think maybe it makes more sense to wait for Parsimmon 2.0 to accept this PR? Hmm. |
|
As you pointed out, there's a possibility to break backward compatibility. All context-dependent parsers, which decides to consume input by context or not, will be affected by this change. It means the parser switches context inside infinite loop. Such kind of parser is too dependent to current FYI I researched other parsing library Nom(Rust) and Parsec(Haskell). Both of these reject empty string. Here's a reference:
My opinion is it is okay to ship this under current 1.x version. |
|
Hmm, I think you're right. I should clarify in https://github.com/jneen/parsimmon/blob/master/API.md#side-effects that side effects also include randomness or reading from global state during parsing. It's already an expectation that bad things can happen if you do that. |
test/core/many.test.js
Outdated
| @@ -18,4 +18,9 @@ suite('many', function() { | |||
| assert.equal(parser.parse('xxxxxy').value, 'y'); | |||
| }); | |||
|
|
|||
| test('can process infinite loop', function() { | |||
There was a problem hiding this comment.
test('aborts on 0-character parse', ...) maybe?
src/parsimmon.js
Outdated
| return Parsimmon(function(input, i) { | ||
| var accum = []; | ||
| var result = undefined; | ||
|
|
||
There was a problem hiding this comment.
please remove the spaces on this line
| for (;;) { | ||
| result = mergeReplies(self._(input, i), result); | ||
| if (result.status) { | ||
| if (i === result.index) { | ||
| return makeFailure(i, 'Infinite loop has been detected in many() parser'); | ||
| } |
There was a problem hiding this comment.
hmm, should this use makeFailure, or throw new Error(...). it isn't a failed parse? it's a broken parser? i don't know.
There was a problem hiding this comment.
Seems you are right. I will change to Error so that it is more suitable in this context.
There was a problem hiding this comment.
this is off topic but i wish if parsimmon had custom errors containing its own kind...
There was a problem hiding this comment.
@minamorl Do you still have time to change to throw new Error(...)?
|
@minamorl do you still want to work on this PR? or should I finish it |
Related to #197
This changes contains runtime detection of infinite loop. If current parsing position is exact same as each parser's result, it should be infinite loop.