Skip to content

Commit

Permalink
Fix in response to @hvacengi's review comment.
Browse files Browse the repository at this point in the history
Why it wasn't noticed before:
-----------------------------

The problem only happened when you use a SET command to
assign an anonymous function to a variable, as in

SET anon TO {lock foo to 1.}.

But if you happened to use a DECLARE command of some sort to do so,
like a LOCAL... or GLOBAL, like so:

LOCAL anon IS {lock foo to 1.}.

then it didn't exhibit the problem.  This is why I didn't
notice it before.  It turns out all my tests were doing it
that way.

The Cause:
----------

The problem had been caused by the fact that when
Compiler.IterateUserFunctions() walked the parse tree to try to
find all the user functions and lock statements, it failed
to recurse into the anon functions in some cases and thus didn't
find the locks that were inside them.  (It only pre-processes the
lock statements when this walk finds them).

It failed to recurse into some of the kinds of parse tree nodes
which could contain anon functions because it only recursed into
node types that were explicitly mentioned in a case statement list,
and it assumed anything not on the list cannot contain further
statements nested inside it.  This list was not entirely up to
date and correct, missing a few types of statement that were now
capable of containing further nested statements.  Now that anon
functions exist, almost every kind of expression could be an anon
function with statements nested inside it.

The Fix:
--------

Given that two different recent issues both were caused by failures
of this walk to recurse into some kinds of parse node, and some
other issues in the past were also caused by the very same
section of code failing to mention a new part of speech after
the grammar file changed,  I decided it was high time I flipped
the logic around. Now it operates on a blacklist instead of a
whitelist.  Now it automatically descends into every kind of
node *except* the ones explicitly mentioned that we know can't
contain any further open-ended expressions inside them.  This
way if we forget to mention a node type on that list, the failure
results in merely a bit of wasted execution time for failing
to prune the tree walk early enough, rather than a fatal error
for forgetting to mention a node like we had before.
  • Loading branch information
Dunbaratu committed Sep 21, 2016
1 parent 7b89977 commit 81de368
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 29 deletions.
28 changes: 28 additions & 0 deletions kerboscript_tests/user_functions/functest33.ks
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Testing that anon functions can have locks in them.
// (Test added in response to issue #1801).

// Tests that the compiler will properly recurse into
// all the weird places an anon function might be,
// which is pretty much *any* expression, and walk
// their contents looking for LOCK statements.

function named_func { lock foo to 1. }

function func_that_invokes_delegate { parameter del. del(). }
function func_that_returns_delegate { return {lock foo to 1.}. }

global declare_global_anon_func is { lock foo to 1. }.

set set_stmt_anon_func to { lock foo to 1. }.

print "Should see no output, no errors. Just program end".
print "==================================================".
named_func().
declare_global_anon_func().
set_stmt_anon_func().
{ // do-nothing nested braces for local scoping.
local declare_local_anon_func is { lock foo to 1. }.
declare_local_anon_func().
}
func_that_invokes_delegate( { lock foo to 1. } ).
func_that_invokes_delegate( func_that_returns_delegate() ).
85 changes: 57 additions & 28 deletions src/kOS.Safe/Compilation/KS/Compiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -238,42 +238,71 @@ private void RearrangeParseNodes(ParseNode node)

private void IterateUserFunctions(ParseNode node, Action<ParseNode> action)
{
bool doChildren = true;
bool doInvoke = false;
switch (node.Token.Type)
{
// Statements that can have other statements nested inside them need to be
// recursed through to search for instances of the special statements
// we are looking for here:
// Any statement which might have another statement nested inside
// it should be recursed into to check if that other statement
// might be a lock, or a user function (anonymous or named).
//
case TokenType.Start:
case TokenType.instruction_block:
case TokenType.instruction:
case TokenType.if_stmt:
case TokenType.fromloop_stmt:
case TokenType.until_stmt:
case TokenType.for_stmt:
case TokenType.on_stmt:
case TokenType.when_stmt:
case TokenType.declare_identifier_clause:
case TokenType.expr:
case TokenType.declare_function_clause:
foreach (ParseNode childNode in node.Nodes)
IterateUserFunctions(childNode, action);
break;
// We assume by default a node should be recursed into unless explicitly
// told otherwise here. Doing it this way around is the safer default
// because of what happens when we fail to mention a part of speech here.
// If we fail to recurse when we should have, that can be fatal as it makes
// the compiler produce wrong code. But if we do recurse when we didn't have
// to, that just wastes a bit of CPU time, which isn't as bad.
//
case TokenType.onoff_trailer:
case TokenType.stage_stmt:
case TokenType.clear_stmt:
case TokenType.break_stmt:
case TokenType.preserve_stmt:
case TokenType.run_stmt:
case TokenType.compile_stmt:
case TokenType.list_stmt:
case TokenType.reboot_stmt:
case TokenType.shutdown_stmt:
case TokenType.unset_stmt:
case TokenType.sci_number:
case TokenType.number:
case TokenType.INTEGER:
case TokenType.DOUBLE:
case TokenType.PLUSMINUS:
case TokenType.MULT:
case TokenType.DIV:
case TokenType.POWER:
case TokenType.IDENTIFIER:
case TokenType.FILEIDENT:
case TokenType.STRING:
case TokenType.TRUEFALSE:
case TokenType.COMPARATOR:
case TokenType.AND:
case TokenType.OR:
case TokenType.directive:
doChildren = false;
break;

// These are the statements we're searching for to work on here:
//
case TokenType.declare_stmt: // for DECLARE FUNCTION's
// for catching functions nested inside functions, or locks nested inside functions:
// Depth-first: Walk my children first, then iterate through me. Thus the functions nested inside
// me have already been compiled before I start compiling my own code. This allows my code to make
// forward-calls into my nested functions, because they've been compiled and we know where they live
// in memory now.
foreach (ParseNode childNode in node.Nodes)
IterateUserFunctions(childNode, action);

action.Invoke(node);
case TokenType.declare_stmt: // for DECLARE FUNCTION's.
case TokenType.instruction_block: // just in case it's an anon function's body
doInvoke = true;
break;
default:
break;

}
// for catching functions nested inside functions, or locks nested inside functions:
// Depth-first: Walk my children first, then iterate through me. Thus the functions nested inside
// me have already been compiled before I start compiling my own code. This allows my code to make
// forward-calls into my nested functions, because they've been compiled and we know where they live
// in memory now.
if (doChildren)
foreach (ParseNode childNode in node.Nodes)
IterateUserFunctions(childNode, action);
if (doInvoke)
action.Invoke(node);
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/kOS.Safe/Compilation/ProgramBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ private void ReplaceLabels(List<Opcode> program)
labels.Add(program[index].Label, index);
}
}

// replace destination labels with the corresponding index
for (int index = 0; index < program.Count; index++)
{
Expand Down

0 comments on commit 81de368

Please sign in to comment.