Function parameters clobbered by higher scope

[gisikw]

When a function parameter has a name collision with something in a higher scope, the higher-scope item is incorrectly resolved to. Put simply: outer scope variables are clobbering inner parameters.

Repro script:

{
  function fn {
    print "The outer function was called".
  }

  {
    function inner_fn {
      print "The inner function was called".
    }

    function test {
      parameter fn.
      fn().
    }

    test(inner_fn@).
  }
}

Expected behavior: "The inner function was called" Actual behavior: "The outer function was called"

[Dunbaratu]

Do you get the same problem when you explicity use the call suffix, like fn:call()? I'm just trying to work out the source of the problem.

[gisikw]

fn:call() explicitly fails with that example.

GET Suffix 'CALL' not found on object 0
An attempt was made to get a suffix called:
    CALL
from an object of type:
    ScalarIntValue
when that object does not have that
suffix usable in that way.
Possible causes are:
  - No ScalarIntValue ever has a CALL suffix.
  - There is such a suffix, but it can't be used
      with a get operation.
  - There is such a suffix on some instances
      of ScalarIntValue, but not this particular one.

That makes sense to me, since the outer fn isn't a delegate. So it's effectively trying to call fn():call(). However, it looks like only functions are causing the clobber. If I assign a delegate at a higher scope, the inner function is called correctly.

{
  function not_fn {
    print "The outer function was called".
  }
  local fn is not_fn@.

  {
    function inner_fn {
      print "The inner function was called".
    }

    function test {
      parameter fn.
      fn:call().
    }

    test(inner_fn@).
  }
}

=> "The inner function was called"

[Dunbaratu]

It may be an easier fix than I thought. I'll have to have a look later today. I have a regular twitch streaming I do on Thursdays starting in the late afternoon, so it might be a little bit before I have a look.

[gisikw]

Oh, no rush at all! I did go through and check the last couple releases, and I was able to reproduce the issue there as well, so it looks like this isn't something new (I'm pretty confident it wasn't an issue in 0.19, but wasn't sure if that was compatible with newer KSP versions, so didn't go back further than that).

Also, it looks like the issue can be explicitly circumvented by using anonymous delegate syntax, which is the same number of characters anyway. That is to say:

{
  local fn is {
    print "the outer function was called".
  }.

  {
    function inner_fn {
      print "the inner function was called".
    }

    function test {
      parameter fn.
      fn:call().
    }

    test(inner_fn@).
  }
}

...exhibits the correct behavior. So it's only the explicit function <name> syntax that clobbers, and that syntax can be avoided anyway with the new delegate syntax. So given that...

• The bug has existed in the last two releases
• Nobody encountered issues with it until now
• Using anonymous delegates is a viable workaround

...if it turns out to be more than a trivial fix, I'd certainly say it's not something worth holding up the 1.0 release for.

[Dunbaratu]

Can you perform the following test and tell me what you get (I am not near my full development computer (just on a primitive laptop) and won't be for several hours). Split it into two files like so:

file1.ks:

  function fn {
    print "The outer function was called".
  }

file2.ks:

   run "file1.ks".
  {
    function inner_fn {
      print "The inner function was called".
    }

    function test {
      parameter fn.
      fn().
    }

    test(inner_fn@).
  }
}

It's the same general idea but makes it compile the global function in a separate compiler pass. I have a suspicion that the problem is related to leftover old code from before we had scoping and it dealt with functions differently.

[gisikw]

Sadly nope, the parameter is still clobbered with that example. "The outer function was called"

[chippydip]

Pretty sure the problem is in variable lookup: https://github.com/KSP-KOS/KOS/blob/c03271dd8913f17acd9361ced32893f703aa3bd7/src/kOS.Safe/Execution/CPU.cs#L662

The function call would be doing a lookup for $fn* which finds the outer function. If the outer function is removed then the initial lookup fails and that method tries again to lookup $fn which will find the local (parameter) variable. Switching to an implicit delegate will also cause the initial resolution to fail since there will now be two $fn variables in different scopes and no $fn* function.

This is obviously only an issue with delegate variables and has probably existed since they were introduced. The variable isn't actually getting clobbered and doing something other than invoking it should resolve to the correct local parameter.

It seems like variable resolution should probably try to resolve both$fn* and $fn and if both are found prefer the one with the largest ScopeId. Alternately (and probably a much bigger change), it seems like variable scope resolution should really match on undercoated names and if function is found when a plain variable was expected that should be an error. For example:

local foo is 123.
{
    function foo { }

    // this can't be a function and currently resolves to the var but
    // probably should resolve to the function and throw an error
    print foo + 1.
}

[Dunbaratu]

@chippydip : this is correct and I'm already looking into the right fix. Unfortunately the fix may be a bit of a refactor that requires a lot of backward compatibilty testing.

Basically, I no longer see any need for the difference between $fn* and $fn like there once was, because now both are essentially delegates. The proper fix, I think, is to strike the logic that appends "*" to identifiers in some cases. That presumption is in more than one place in the code, however, so I have to be careful.

[gisikw]

@Dunbaratu, let me know if there are particular types of examples you need KerboScript tests for - I'm afraid I'm not much use as far as implementing a fix goes, but happy to write some .ks regression tests if they'd help!

[Dunbaratu]

Okay there are Waaaaaay too many places where the assumption that "asterisk means function pointer" is buried deep in the system. This is a messy thing. I think the only real workable solution is to alter the Dictionary such that it views "yadda*" and "yadda" as equal identifier values that "hash" the same.

Edit: Yes I think this is the easiest solution for now without an enormous refactor.

kOS.Safe.Execution.VariableScope has a C# Dictionary object that holds the mapping from ident name to variable value for a given scope (the nesting system is a stack of such VariableScope objects so you can walk the list looking for a hit in each Dictionary and the first hit you find will be the innermost identifier).

So I think I can fix this with just providing my own homebrew comparator to the Dictionary object that causes foo and foo* to be seen as identical, using this variant of the Dictionary constructor:

    Dictionary<TKey, TValue>(IDictionary<TKey, TValue>, IEqualityComparer<TKey>)

[chippydip]

The simplest fix might be to just add an additional check here: https://github.com/KSP-KOS/KOS/blob/c03271dd8913f17acd9361ced32893f703aa3bd7/src/kOS.Safe/Execution/CPU.cs#L524

if (localDict.Variables.ContainsKey(identifier) || (identifier.EndsWith("*") && localDict.Variables.ContainsKey(indentifier.Substring(0, indentifier.Length-1)))

There's also a check of the global scope at the end that would need a similar update. Could also cache the substringed version at the start, if applicable, || (idWithoutStar != null && localDict.Variables.ContainsKey(idWithoutStar))

[Dunbaratu]

Yeah but I'm trying to make sure the rule "$fn* and $fn are essentially the same variable" can be enforced in a more universal way across the whole mod.

Edit: It may not work so well, in which case I'll have to use a more ad-hoc solution that just addresses the one single instance where this problem cropped up. But I'd rather not spaghettify the code more than needed. It's got too many weird exceptions as it is.

[Dunbaratu]

Awwww dangit. lock steering and lock throttle are dependant on the fact that throttle and throttle* are two different variables, and steering and steering* are two different variables. Let me see if that can be changed. It's kind of an annoying misfeature, IMO that it works that way.

[chippydip]

Yeah, fixing it everywhere would obviously be the best solution but is clearly a lot more work and more error prone. From a functional standpoint it seems like the only time when the difference would matter is during variable scope resolution so patching it there should be a relatively easy and low-risk change and fixing it "the right way" could be put off as a longer-term task for a future release.

Edit: That sucks about steering and throttle. I assume user scripts can only read from the non-star variable and can only write to the star version?

(Sorry if I'm not being super helpful...I'm procrastinating a bit at work and just started digging into the code while working on a tool to optimizing KSM file size this week, so the workings of the VM are interesting to me.)

[Dunbaratu]

(Yeah the VM is super intersting).

Basically the problem is how cooked steering works: While executing C# code, we don't want to be making "calls" into "functions" that are actually composed of kerboscript code rather than running natively as C# code. Thus we can't have the SteeringManager call the expression in lock steering to expression. (Locks are really just functions that consist of a single return (expression). statement.)

So instead when you do a lock steering to whatever. what it really does is build a 'trigger' that runs every single physics tick that basically does the equivalent of this in VM instructions:

when true then {
   set steering to steering().  // call steering(), put the result in the identifier: steering.
   preserve.
}

Then the SteeringManager always tries to read the stationary "passive" variable steering, rather than calling the function steering*. Calling the function itself is done during the normal run of a batch of instructions.

So the problem is that the dependency on being able to have steering be both a function name and not a function name at the same time, depending on how it's being used, is the thing that's holding up the idea of fixing this "correctly".

I think it could be fixed by just having us pick a different name for the secret "hidden" passive version of the value so the name clash isn't there. I'm asking @hvacengi if he can look into the possibility of changing this, as he's the master of the steering code.

[Dunbaratu]

Okay I just had a really long back-and-forth with @hvacengi and it turns out that the presumption that there are two identifiers called steering, one being a function called steering* and the other being a bound variable called steering, is really really really messy to unwind and we have to deal with the implications carefully.

Because the entire point of my fix to this is to make it so that when searching for steering, steering* is considered a hit, and when searching for steering*, steering is considered a hit, that sticking point is really in the way of fixing it.

Basically, the system as it stands is already dependent on this very weird misfeature of kerboscript - that the same identifer has two different values depending on how you use it - and while we'd like to fix that, doing so is not quick and dirty. We have to refactor a bit.

We discussed several options and think we see a few that, although they would require a refactor, they wouldn't break backward compatibility except in weird edge cases people shouldn't have been taking advantage of because they're places where the way it works is just plain wrong.

Thus because it can be fixed later without a change to backward compatibility, the means it is no longer is something we have to delay 1.0.0 for, which was my original worry. (v1.0.0 is the place we intended to put backward-compatibility breaks we've been holding off on.).

So, in summary, yes I want to fix this, but it can be released in a patch after 1.0.0. It doesn't have to delay 1.0.0.

[Dunbaratu]

So in summary, this is the ugly solution I think I'll be going with. (recorded here for posterity so I don't forget what I was thinking when I pick this up a few days later)

Under the hood, we'll have two different variable names for the 4 cooked steering variables - one for the bound variable and one for the lock function that goes with it. The bound variable name will use the same name we currently use (i.e. throttle), but the lock name will get a new name like throttle^^ - an identifier that cannot be normally used in a kerboscript program. Then, when the compiler notices that a lock command is using one of the 4 magic names, like throttle, then it will re-write that as if it was throttle^^ instead of throttle, thus affecting the lock as a different name from the variable. This is a bit ugly, BUT it's no more ugly than what the compiler does now, where it already has some strange exceptions for those 4 special variable names.