test-lists with one element produce "Unrecognized command" error

[lilyball]

If I have a test-list with a single element, check-sieve produces an "Unrecognized command" error for the test identifier. The only exceptions are for true and false. Adding a second element fixes this, but shouldn't be necessary. The Sieve grammar defines a test-list as containing 1 or more tests, not 2 or more.

It's also a bit surprising that the error says "Unrecognized command" instead of something like "unrecognized test".

This bug is new in version 0.7, version 0.6 accepts this syntax without comment. Version 0.6 also uses the correct "Unrecognized test" warning if I put an actual bogus test in there, as opposed to 0.7's "Unrecognized command".

Example:

if anyof (not true) {
  keep;
} else {
  discard;
}

This produces Unrecognized command "not".

[lilyball]

This may seem like a bit of an edge case, as allof and anyof are pointless if they only are given one test, but I ran into this when parsing my existing sieve rules as it turns out I snuck a not(address :is …) in there instead of not address :is …. My mailserver handled this just fine, and check-sieve 0.6 allowed it as well. This probably should be rejected on the basis that not is defined as not <test1: test> but with a different error specific to not.

Incidentally, when playing with how parens are handled, it turns out that if address :is (true) { keep; } is accepted. This suggests that check-sieve is not validating the expected arguments in the way I thought it would, so maybe not («test») should be accepted too like it was in v0.6 🤷‍♀️.

[dburkart]

Thanks for the test case! This is obviously wrong. In v0.6, we have the following AST:

Mail Sieve
    Branch
        Condition
            Test (anyof)
                Test (not)
                    Test (exists)
                        String List
                            String ("From")
                            String ("Date")
        Block
            Command (discard)

If I comment out the offending validation code, we see that the AST looks quite different in 0.7:

Mail Sieve
    Branch
        Condition
            Test (anyof)
                Test ($COMMAND_RESULT)
                    Command (not)
                        Test (exists)
                            String List
                                String ("From")
                                String ("Date")
        Block
            Command (discard)

The second one is quite a big departure. It looks like this is fallout from issue #40, which aimed to support RFC 6558. This RFC added the notion of a "testable action", which is an action which doubles as an action and uses the result as a test. From the RFC:

This creates a new type of Sieve action, a "testable action". The usage as a test is exactly the same as for an action, and it doubles as an action and a test of the action's result at the same time. See Section 3.2 for an example of how this test can be used.

However, it seems I neglected to adhere to the next paragraph, which states:

Note that defining this new testable action does not change the definitions of any other actions -- it does not imply that other actions can be used as tests. Future extensions might define other testable actions, but those specifications would be responsible for clearly specifying that.

[dburkart]

Incidentally, when playing with how parens are handled, it turns out that if address :is (true) { keep; } is accepted. This suggests that check-sieve is not validating the expected arguments in the way I thought it would, so maybe not («test») should be accepted too like it was in v0.6 🤷‍♀️.

The above behavior is due to true / false being special-cased in the grammar (which is not great). Adhering to a strict interpretation of the RFCs as far as I can tell would mean not allowing parenthesis around individual tests (for grouping purposes, as opposed to define a test-list).

My guess is that grouping parenthesis around a single test case is allowed as a sort of UI affordance, since some people may prefer to group their conditions similar to C-style syntax. Given that there are quite a few test-cases in RFCs that have grouping parenthesis, I'm leaning towards making this more lax.

[dburkart]

This should be fixed now! And this also led me to discover a false positive test case (issue #48), so thank you!