Ignore unknown directives

[grigorescu]

Take testing/btest/language/alternate-prototypes-deprecated-args.zeek from zeek/zeek. When reformatting, we get:

-@TEST-START-FILE hide.zeek
+@ @ TEST - START -  FILE hide.zeek

I realize that this isn't a Zeek script per se, it's a btest. But I'm wondering if we should ignore unknown directives, as the current behavior will break what it doesn't understand.

Or perhaps we make an exception and add the btest-provided directives.

Reason being is that it seems like it'd be handy to format Zeek code in btests in a similar way to other Zeek code.

[ckreibich]

Yeah ... sadly this is tricky, because zeekscript has to work with what the tree-sitter parser feeds it, and the parser struggles with that line. It's actually a cool example because this is the first time I see nested error nodes:

$ echo '@TEST-START-FILE hide.zeek' | zeek-script parse
source_file (0.0,1.0) [error] '@TEST-START-FILE hide.zeek\n'
    ERROR (0.0,0.26) [error] '@TEST-START-FILE hide.zeek'
        ERROR (0.0,0.1) '@'
        expr (0.1,0.26) [error] 'TEST-START-FILE hide.zeek'
            expr (0.1,0.11) 'TEST-START'
                expr (0.1,0.5) 'TEST'
                    id (0.1,0.5) 'TEST'
                - (0.5,0.6)
                expr (0.6,0.11) 'START'
                    id (0.6,0.11) 'START'
            - (0.11,0.12)
            ERROR (0.12,0.16) [error] 'FILE'
                expr (0.12,0.16) 'FILE'
                    id (0.12,0.16) 'FILE'
            expr (0.17,0.26) 'hide.zeek'
                constant (0.17,0.26) 'hide.zeek'
                    hostname (0.17,0.26) 'hide.zeek'
    nullnode (0.0,0.0) ''
parse tree has problems: cannot parse line 0, col 0: "@TEST-START-FILE hide.zeek"

zeekscript's current handling of ERROR nodes is to leave them unmodified except for subtrees that the parser can handle. In normal Zeek scripts you want this, because correct subtrees can make up a large majority of the ERROR range. But here the assumption is flawed from the start because it's ... not a Zeek script. If we'd format "conservatively", meaning "don't touch anything inside error ranges", we'd handle this example correctly. I've been tempted to offer that as a mode via a command-line flag. But that still leaves the basic possibility of the parser succeeding on content it wasn't meant for.

One way or another, the double @ character should not happen and we can hopefully fix that, so I'm going to label this a bug.

I think the btest scenario is tough because it can jump so wildly through implicit languages. BIFs look straightforward by comparison. It would be cool to see how/whether one could write that via layering on top of the existing grammar.js file — I'm optimistic since it's all just JavaScript.

[bbannier]

Are there cases where one cannot put BTest instructions in comments? I am wondering whether it would be worthwhile to add support for BTest instructions with default CommandPrefix @TEST at all when this prefix can be configured.