Add a test case for issue #290 #343
Conversation
|
Ideally we'd have a fix :) As a second possibility, we can add an "ignore" test to remind us to fix it, but I don't think we really want a complex test setup thingy that we're bound to be confused by in the future (at least I'll be confused!). |
|
Yeah, I was figuring I would look into this more now, since I'm concerned fixing it the way yacc does might require a semver bump. If there is no mutual recursive one, I can think of another approach that would fix it without the semver bump which may be a lot less work. So I'll go ahead and put draft on it, for now and look into that stuff. |
This test case is not caught by the error that I proposed in pr softdevteam#294.
| @@ -551,6 +577,8 @@ fn resolve_shift_reduce<StorageT: 'static + Hash + PrimInt + Unsigned>( | |||
| // If the token and production don't both have precedences, we use Yacc's default | |||
| // resolution, which is in favour of the shift. | |||
| actions[off] = StateTable::encode(Action::Shift(stidx)); | |||
| let ridx = grm.prod_to_rule(pidx); | |||
| *rule_usage.get_mut(usize::from(ridx)).unwrap() = (ridx, true); | |||
There was a problem hiding this comment.
I believe this marking things as used when there is a conflict resolved in their favor may not be right,
in cases when there is some subsequent conflict with the production that it was resolved in favor of
There was a problem hiding this comment.
I must admit that I have no idea!
There was a problem hiding this comment.
Well, I'm going to try and avoid relying on that, if we look at what is still reachable after conflicts have been removed.
and use the forward pass merely for rules which are plain unused. It seems like a less fragile than trying to rely the above.
The actual problem I was running into with the case above, is the wrong pidx, and it seems much more difficult (if actually possible) to go from stidx back to a pidx, which working based on what was discarded it appears doesn't need to happen.
There was a problem hiding this comment.
From memory, a StIdx can represent multiple productions. I'm not sure if that helps you either way!
|
I'm not sure if this one needs action? |
|
Going to close for now, as it has stalled in being able to get anything useful which doesn't have false positives. |
Here is an attempt to add a test case for #290. I tried to do something so that CI wouldn't trip over it,
but still allowed you to run it via passing
cargo test --features test_known_failures. This could be flipped around,so that it runs locally but you need to pass a feature to ignore it for CI.
Or it may be we just want to work on a fix before we commit a test for it?