"System.ArgumentException: An element with the same key already exists in the dictionary." when attempting to call one script from another

[space-is-hard]

Forgive me if this worded incorrectly; I'm putting this here at the request of Dunbaratu.

I'll run this script (already compiled), and it tries to call this script (also already compiled) a little further on. At this point it throws an error and points to the line at which the second script is called from. Relevant excerpt from output.log.

[Dunbaratu]

I'd recommend waiting until 0.17 is out and seeing if the problem persists. We've done some big changes to how script calling works, so much so that trying to trace the problem in the older codebase at this point would probably be an inefficient way to spend our time.

I'm not trying to belittle the problem you're encountering. It's important. I'm just saying it's hard to debug it when the code you're running on and what we have in development are currently so very different from each other, especially in the area of how scripts call other scripts.

After we get 0.17 out, please do let us know if it persists and I'll look into it then.

[space-is-hard]

No worries, I've found a way to work around it anyways.

[Dunbaratu]

I disagree with this being on the 0.17.0 punchlist. The order of events I was imagining would take place was:

1. @ascensionislander waits for us to release 0.17.0.
2. we release 0.17.0.
3. @ascensionislander tries it again. If it's still broken, then I look into the issue further.

[space-is-hard]

I was able to replicate this again in 0.17 with a similar script. I'm going to try to narrow it down as much as possible.

[Dunbaratu]

You appear to be trying to call a KSM file (from the original issue description) KSM files may not work right without a new recompile, when the mod gets updated. We may want to put a version number in the KSM file so we can detect this.

But anyway, make sure you delete all KSM files when installing a kOS upgrade. If you want them back, recompile them after the upgrade.

[space-is-hard]

The first script is actually called by a boot script that (re)compiles and copies all of the necessary scripts, so that wouldn't be it.

I gave up on trying to narrow this one down. I couldn't replicate it with test scripts, and I couldn't replicate it with the same script with major sections commented out. I've got no idea what else to try.

[Dunbaratu]

I'm going to close this as un-replicatable.

[Dunbaratu]

Should there be a github label for "can't replicate"?

[erendrake]

Sure, or it could fall under 'won't fix' even though it's not quite the same.

[jdthorne]

Hey, I think I found a minimal test case that replicates this issue (KSP 1.0.4 and kOS 0.17.3).

Some background: I'm basically recreating MechJeb in kOS as an excuse to learn more about the mathematics behind control systems and orbital mechanics. I'm up to about 2,000LOC of Kerboscript, across ~24 files, and it takes a full 20 seconds to load them all on my machine (2015 rMBP). This happens whenever I switch vessels etc, so it's a bit annoying, and I was hoping to save time by only recompiling the scripts as they change.

Anyway, this script reliably reproduces this issue for me (saved and ran from the archive as test_case.ks):

FUNCTION One { GLOBAL LOCK TestCaseLock TO 0. }

LOG "FUNCTION Two { GLOBAL LOCK TestCaseLock TO 0. }" TO "compiled.ks".
COMPILE compiled.ks TO compiled.ksm.

RUN compiled.ks.  // This works fine
RUN compiled.ksm. // This doesn't

The issue seems to be related to loading multiple GLOBAL LOCK statements inside FUNCTIONs that lock the same global variable. In my case it was multiple LOCK Throttle TO statements that triggered the issue, but it seems to happen with any variable (like TestCaseLock above). Some other things I noticed while narrowing it down:

• If you comment out RUN compiled.ks, the issue is still triggered, presumably because there's still a GLOBAL LOCK loaded from both the test case script (in One()), and the compiled module (in Two())
• If you comment out FUNCTION One at the top, the issue is still triggered, presumably because there are again still two GLOBAL LOCK TestCaseLocks - one loaded from RUN compiled.ks, and another one from RUN compiled.ksm.
• If you comment out both FUNCTION One and RUN compiled.ks, then RUN compiled.ksm works fine.
• If you comment out RUN compiled.ks, and change FUNCTION One { } to GLOBAL LOCK SomeOtherVariableName, then that works fine too.

Also, here's my ksp.log while running that test case. Let me know if I can provide anything else that might be helpful.

[hvacengi]

I've been able to reproduce this issue as well without global locks but instead by calling a compiled library that declares functions. Given the two files:
libtest1.ks:

declare function testing1 {
    print "testing 1".
}
testing1().

libtest2.ks:

//run once libtest1.
run libtest1.
declare function testing2 {
    print "testing 2".
}
testing2().

I get an exception if I compile libtest1 and libtest2. If I compile neither, or only one of them, no exception. The exception is identical to that reported in both cases above. I was surprised to find that the issue persists if I use @Dunbaratu's new run once command from PR #1185.

The script I used to test this was:

copy libtest1 from 0.
copy libtest2 from 0.
//compile libtest1.
//compile libtest2.
run libtest1.
run libtest2.

Make sure to either revert or delete the ksm files in between tests.

I'm going to reopen the issue for now. If it still falls under "won't fix" we can close it back down, but with 2 reports in a month I think it's worth looking into again.

[Dunbaratu]

The system does a cheesy hash checksum of the body of a function to identify it. If two different functions return the same hash, it causes a duplicate key error. The reason this is done is so that if you do lock steering to 2*foo. then lock steering to bar/2. then lock steering to 2*foo. again, it knows the second instance of using 2*foo is really the same thing as the first one was, and doesn't need a new section of code compiled for it. (This checksum is done on the compiled parse tree of the body, not the text, so therefore 2*foo and 2 * foo get the same checksum. The checksum is on the parse branch snippet: "expression consisting of thing1 multiplied by thing2, where thing1 consists of number literal 2 and thing2 consists of an identifier 'foo'", rather than being based on the exact string "2*foo".)

This was there purely for the sake of the lock command, and user functions inherited it because they had to to fit into the same system.

Later, the parse file name (i.e. "libtest1") was added to the hash to ensure that two different instances of the same expression in two different compiled files don't end up being mashed together into the same key.

I suspect the problem is in how the file name is being added to the hash. It's probably not properly dealing with the difference between "libtest1", "libtest1.ks" and "libtest1.ksm", and either it's calling both the ksm and the ks file the same when it shouldn't, or it's calling libtest1 and libtest1.ks different when it shouldn't.

[Dunbaratu]

I tried trimming down @jdthorne 's example case to something simpler and found that doing GLOBAL LOCK inside a function in a subprogram doesn't even work anyway even without all the stuff about compiling on the fly.

Even just a very dumb example like so:

file: subprogram.ks:

Function One {
    Print "Proof Function One is executing".
    GLOBAL LOCK testLock to 0.
    PRINT "testLock should now be set to " + testLock.
}

file program.ks:

run subprogram.
One().
print "in main program, lock is now : " + testLock.

This produces the following output:

Proof Function One is exeucting
testLock should now be set to 0.
Undefined Variable Name 'testlock'.
At program on archive, line 3
print "in main program, lock is now : " + testLock.

So unless even THAT much works, then the more complex case of trying to deal with the ramifications of compiling it on the fly is going to be too hard to trace down. We don't even have a simpler case that does work to compare it to.

(EDIT: Just realized it works if you change this:

print "in main program, lock is now : " + testLock.

To this:

print "in main program, lock is now : " + testLock().

The problem is that the compiler must build an OpcodeCall to use a lock, but must build an OpcodePush to use a passive vanilla variable, and it uses the fact that it's seen the variable used as a lock elsewhere in the compilation file to know which it is... but the main program in this case is unaware that the subprogram is going to call it a lock, so it doesn't know it should build an OpcodeCall instead of an OpcodePush when asked for the value of testLock. Adding the parens makes it explicit that it IS a function call and thus needs the Call not Push.

Fixing this is super ugly. I'd almost rather rewind time and make LOCK not even a thing in the language at all because it breaks almost everything and should just explicitly BE a function call. since that's what they really are.

It's a bit like I imagine the compiler has to deal with Properties in C#, which look like vanilla fields but are really method calls. But the difference is that C# does not use late runtime binding. The compiler CAN know ahead of time that the identifier in question has been defined that way. We can't because the system has runtime bindings - you don't know until it actually gets run which kind of identifier testLock is going to be.,

[ozin370]

I ran into this problem today - locking throttle inside a subprogram's function. This workaround worked for me:

in the main program:

global lockToggle is false.
until exit {
    flightcontroller().
    if lockToggle { set lockToggle to false. lock throttle to th. }
}

in the subprogram:

`function flightcontroller {
    ...
    //need to lock thottle:
    set lockToggle to true.
    ...
}

[Dunbaratu]

This is "half fixed" by #1216. I'm not sure how to "half close" an issue.