parse.y/directives: Reject directives as statements

Avoid the issue outlined in #2289 where the @if or @else is taken as the
statement of an `if`, `for` or `while` by rejecting such constructs.

Effectively this means the following scripts are now rejected:

    # Print's "cond true" with Zeek 5.0 even though the `if ( F )`
    # should be in effect.

    if ( F )
        @if ( T )
            print "cond true";
        @else
            print "cond false";
        @endif

or

    # Print's "hello" once with Zeek 5.0
    local v = vector( 1, 2, 3 );

    for ( i in v )
        @if ( T )
        print("hello")
        @endif

To make above work as intended, additional braces can be used.

    if ( T )
        {
    @if ( cond )
            print "cond true";
    @else
            print "cond false";
    @endif
        }

    for ( i in v )
        {
    @if ( T )
        print("hello")
    @endif
        }

NEWS

@@ -60,6 +60,13 @@ Breaking Changes
   value alone, we aren't able to deprecate the original version of the method.
   This may cause build of packages to fail if they were using this method.
 
+- Conditional directives (``@if``, ``@ifdef``, ``@ifndef``, ``@else`` and ``@endif``)
+  can not be placed directly following ``if``, ``for`` or ``while`` statements
+  anymore. This was interpreted non-intuitively and could lead to subtle bugs.
+  The statement following the directive was placed outside of its intended block.
+  Placing braces after ``if``, ``for`` or ``while`` should result in the
+  intended behavior.
+
 New Functionality
 -----------------
 

src/Stmt.cc

@@ -158,6 +158,12 @@ const ReturnStmt* Stmt::AsReturnStmt() const
 	return (const ReturnStmt*)this;
 	}
 
+const NullStmt* Stmt::AsNullStmt() const
+	{
+	CHECK_TAG(tag, STMT_NULL, "Stmt::AsNullStmt", stmt_name)
+	return (const NullStmt*)this;
+	}
+
 bool Stmt::SetLocationInfo(const Location* start, const Location* end)
 	{
 	if ( ! Obj::SetLocationInfo(start, end) )
@@ -1785,6 +1791,8 @@ TraversalCode InitStmt::Traverse(TraversalCallback* cb) const
 	HANDLE_TC_STMT_POST(tc);
 	}
 
+NullStmt::NullStmt(bool arg_is_directive) : Stmt(STMT_NULL), is_directive(arg_is_directive) { }
+
 ValPtr NullStmt::Exec(Frame* /* f */, StmtFlowType& flow)
 	{
 	RegisterAccess();

src/Stmt.h

@@ -525,7 +525,7 @@ protected:
 class NullStmt final : public Stmt
 	{
 public:
-	NullStmt() : Stmt(STMT_NULL) { }
+	NullStmt(bool arg_is_directive = false);
 
 	ValPtr Exec(Frame* f, StmtFlowType& flow) override;
 	bool IsPure() const override;
@@ -536,6 +536,12 @@ public:
 
 	// Optimization-related:
 	StmtPtr Duplicate() override { return SetSucc(new NullStmt()); }
+
+	// Returns true if this NullStmt represents a directive (@if..., @else, @endif)
+	bool IsDirective() const { return is_directive; };
+
+private:
+	bool is_directive;
 	};
 
 // A helper class for tracking all of the information associated with

src/StmtBase.h

@@ -34,6 +34,7 @@ class ExprStmt;
 class ForStmt;
 class IfStmt;
 class InitStmt;
+class NullStmt;
 class PrintStmt;
 class ReturnStmt;
 class StmtList;
@@ -92,6 +93,7 @@ public:
 	const WhileStmt* AsWhileStmt() const;
 	const WhenStmt* AsWhenStmt() const;
 	const SwitchStmt* AsSwitchStmt() const;
+	const NullStmt* AsNullStmt() const;
 
 	void RegisterAccess() const
 		{

src/input.h

@@ -28,6 +28,7 @@ extern void do_atifdef(const char* id);
 extern void do_atifndef(const char* id);
 extern void do_atelse();
 extern void do_atendif();
+extern void reject_directive(zeek::detail::Stmt* s);
 extern void do_doc_token_start();
 extern void do_doc_token_stop();
 

src/parse.y

@@ -1773,6 +1773,7 @@ stmt:
 
 	|	TOK_IF '(' expr ')' stmt
 			{
+			reject_directive($5);
 			set_location(@1, @4);
 			$$ = new IfStmt({AdoptRef{}, $3}, {AdoptRef{}, $5}, make_intrusive<NullStmt>());
 			script_coverage_mgr.AddStmt($$);
@@ -1780,6 +1781,8 @@ stmt:
 
 	|	TOK_IF '(' expr ')' stmt TOK_ELSE stmt
 			{
+			reject_directive($5);
+			reject_directive($7);
 			set_location(@1, @4);
 			$$ = new IfStmt({AdoptRef{}, $3}, {AdoptRef{}, $5}, {AdoptRef{}, $7});
 			script_coverage_mgr.AddStmt($$);
@@ -1794,12 +1797,14 @@ stmt:
 
 	|	for_head stmt
 			{
+			reject_directive($2);
 			$1->AsForStmt()->AddBody({AdoptRef{}, $2});
 			script_coverage_mgr.AddStmt($1);
 			}
 
 	|	TOK_WHILE '(' expr ')' stmt
 			{
+			reject_directive($5);
 			$$ = new WhileStmt({AdoptRef{}, $3}, {AdoptRef{}, $5});
 			script_coverage_mgr.AddStmt($$);
 			}
@@ -1906,7 +1911,7 @@ stmt:
 			}
 
 	|	conditional
-			{ $$ = new NullStmt; }
+			{ $$ = new NullStmt(true /* is_directive */); }
 	;
 
 stmt_list:

src/scan.l

@@ -847,6 +847,14 @@ void do_atendif()
 	--conditional_depth;
 	}
 
+// If the given Stmt is a directive, trigger an error so that its usage is rejected.
+void reject_directive(zeek::detail::Stmt* s)
+	{
+	if ( s->Tag() == STMT_NULL )
+		if ( s->AsNullStmt()->IsDirective() )
+			zeek::reporter->Error("incorrect use of directive");
+	}
+
 // Be careful to never delete things from this list, as the strings
 // are referred to (in order to save the locations of tokens and statements,
 // for error reporting and debugging).

testing/btest/Baseline/language.at-if-else-if-no-way/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/at-if-else-if-no-way.zeek, line 12: incorrect use of directive

testing/btest/Baseline/language.at-if-else-if-no-way/.stdout

@@ -0,0 +1 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.

testing/btest/Baseline/language.at-if-reject-2/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/at-if-reject.zeek, line 6: incorrect use of directive

testing/btest/Baseline/language.at-if-reject-3/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/at-if-reject.zeek, line 5: incorrect use of directive

testing/btest/Baseline/language.at-if-reject-4/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/at-if-reject.zeek, line 5: incorrect use of directive

testing/btest/Baseline/language.at-if-reject-5/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/at-if-reject.zeek, line 6: incorrect use of directive

testing/btest/Baseline/language.at-if-reject/.stderr

@@ -0,0 +1,3 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/at-if-reject.zeek, line 9: incorrect use of directive
+error in <...>/at-if-reject.zeek, line 13: syntax error, at or near "else"

testing/btest/language/at-if-else-if-no-way.zeek

@@ -0,0 +1,14 @@
+# @TEST-DOC: Regression test for #2289 from vpax - previously this printed "There's way this should happen", now it's a syntax error.
+# @TEST-EXEC-FAIL: zeek -b %INPUT
+# @TEST-EXEC: btest-diff .stdout
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr
+
+event zeek_init()
+	{
+@if ( T )
+	if ( F )
+@else
+	if ( F )
+@endif
+		print "There's no way this should happen";
+	}

testing/btest/language/at-if-reject.zeek

@@ -0,0 +1,57 @@
+# @TEST-DOC: Test for #2289 - reject directives appearing as statements
+# @TEST-EXEC-FAIL: zeek -b %INPUT
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr
+
+event zeek_init()
+	{
+	if ( F )
+		@if ( T )
+			print "Bad branch true";
+		@else
+			print "Bad branch false";
+		@endif
+	else
+		print "That's the right branch";
+	}
+
+@TEST-START-NEXT
+event zeek_init()
+	{
+	if ( F )
+		print "That would be okay";
+	else
+		@if ( T )
+			print "That isn't";
+		@endif
+	}
+
+@TEST-START-NEXT
+event zeek_init()
+	{
+	local vec = vector(1, 2, 3);
+	for ( i in vec )
+		@if ( T )
+			print "Bad branch true";
+		@endif
+	}
+
+@TEST-START-NEXT
+event zeek_init()
+	{
+	local i = 10;
+	while ( --i != 0 )
+		@if ( T )
+			print "Bad branch true";
+		@endif
+	}
+
+@TEST-START-NEXT
+global cond = T;
+event zeek_init()
+	{
+	local vec = vector(1, 2, 3);
+	for ( i in vec )
+		@if ( cond  )
+			print "Bad branch true";
+		@endif
+	}