Fixes #1801 and Fixes #1784. #1811

Dunbaratu · 2016-09-09T02:21:46Z

The problem in issue #1801 was that the logic in the Compiler's IterateuserFunctions() that does a tree walk through everywhere a function statement might exist to preform function preprocessing on it, was failing to descend into the instruction_block of an anon function.

This was because the instruction_block of an anon function is a kind of expr instead of being a kind of instruction, and the case statement wasn't set up to descend into expr's.

It turns out that, as a bonus, this also fixed issue #1784 as well, for the same reason.

Fixes #1784
Fixes #1801

The problem was that the logic in the Compiler's IterateuserFunctions() that does a tree walk through everywhere a ``function`` statement might exist to preform function preprocessing on it, was failing to descend into the ``instruction_block`` of an anon function. This was because the ``instruction_block`` of an anon function is a kind of ``expr`` instead of being a kind of ``instruction``, and the case statement wasn't set up to descend into ``expr``'s.

hvacengi · 2016-09-21T02:03:18Z

@Dunbaratu I'm testing out this PR to merge. I noticed that you've tagged #1784 as being fixed as well but that there wasn't a function test for it. So I added the following script, and I still get the error: "The given key was not present in the dictionary."

// functest33.ks
// Testing issue 1784 - lock steering nested in anon functions.

function testNamed {
    lock steering to up.
}

set testAnon to {
    lock steering to up.
}.

testNamed().
testAnon().

Am I missing something in the testing of that error?

Dunbaratu · 2016-09-21T04:59:56Z

I'll give your test case a try and see what I get. I was sure I tested that case but maybe I didn't, or maybe something hidden about how I tested it caused it to avoid the problem but not in a fully fixed way.

Dunbaratu · 2016-09-21T07:12:58Z

Okay now I know why my test didn't catch the problem and yours did.

Although this crashes:

set testAnon to {
  lock steering to up.
}.
testAnon().

This works fine, and doesn't crash:

local testAnon is {
  lock steering to up.
}.
testAnon().

The error does not surface when using a declaration statement like local or global, and thus my tests didn't catch it.

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.

hvacengi merged commit 81de368 into KSP-KOS:develop Sep 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1801 and Fixes #1784. #1811

Fixes #1801 and Fixes #1784. #1811

Dunbaratu commented Sep 9, 2016 •

edited by hvacengi

Loading

hvacengi commented Sep 21, 2016

Dunbaratu commented Sep 21, 2016 •

edited

Loading

Dunbaratu commented Sep 21, 2016 •

edited

Loading

Fixes #1801 and Fixes #1784. #1811

Fixes #1801 and Fixes #1784. #1811

Conversation

Dunbaratu commented Sep 9, 2016 • edited by hvacengi Loading

hvacengi commented Sep 21, 2016

Dunbaratu commented Sep 21, 2016 • edited Loading

Dunbaratu commented Sep 21, 2016 • edited Loading

Dunbaratu commented Sep 9, 2016 •

edited by hvacengi

Loading

Dunbaratu commented Sep 21, 2016 •

edited

Loading

Dunbaratu commented Sep 21, 2016 •

edited

Loading