Reject ungrounded productions #382
Merged
+263
−208
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Soar experts would never (on purpose) write something like:
sp { propose*foo
(state <s>)
(<s> -^results <any>)
-->
(<s> ^operator <o> + =)
(<o> ^name do-foo)
}
Notice the missing `^superstate` or `^type` test. However, Soar has always
accepted these, at least since I've been using it. Under the hood, Soar will add
attribute-value tests where it thinks it's obvious that they belong, and `state/
impasse` is one of those places. The above production becomes:
sp {propose*foo
(state <s> ^<a*1> <v*1> -^results <any>)
-->
(<s> ^operator <o> + ^operator <o> =)
(<o> ^name do-foo)
}
The `^<a*1> <v*1>` match all top-state attributes, so this production proposes
6 operators at present time.
I found this confusing when first learning Soar (and when trying to pick it up
again!), and I spent many hours on my Soar parsers trying to figure out what the
correct grammar rule was, since the grammar in the manual also doesn't specify
that the tests can be implied here.
CIC Soar experts believe that this production syntax is just wrong, and I agree,
so disallow it and add a friendly message to remind users to add a positive
test. This requires two changes:
1. Signal from `parse_head_of_conds_for_one_id` that the parsed condition is a
state or impasse, and
2. using this signal in `parse_tail_of_conds_for_one_id`, refuse to generate
default attribute-value tests for states and impasses.
Add unit tests to check that various forms of `(state ...)` do or do not
successfully parse.
Fixes #377. Also fixes #378, although I don't know what the root cause was
there, so we might just be hiding the real issue.
It seems like the reording logic may have lived in ebc at one time, though I can't be sure about that. The logic that checks and warns for ungrounded productions generated warnings related to chunking and used chunking trace settings, even though it's fully possible to trigger these from regular `sp` commands (and there is an additional call in RL, as well). Move all of the chunking-related warnings and settings back to ebc. Instead of setting `m_failure_type` on the agent's chunking manager, return the error code from `reorder_and_validate_lhs_and_rhs` and check that in ebc to trigger logging and agent stopping logic. Place the logic moved there into a new dedicated method. Update other consumers of this method to check for `reorder_success` instead of `true`, as well. Put the reorder failure logging behind `OM_WARNINGS`, which is on by default and is (correctly) not specific to chunking/ebc. Rename `EBCFailureType` and `display_ebc_error` to reflect that fact that they they aren't chunking-specific. Update unit tests to check the warnings printed for ungrounded productions.
Add additional ways to test the state/impasse legally, just to clarify what's allowed vs. what's not. Better differentiate each of the production names. Even though it's unnecessary for the test to function, it makes discussion easier.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change rejects productions like the one in #377 that have no positive attribute-value test on the state/impasse. I think it makes Soar more beginner friendly, while also fixing a crash (#378).
For the reviewer: the TL;DR of changes to what Soar will allow for production syntax is in FullTests.cpp, in
void FullTests_Parent::testUngroundedLHS().