Skip to content

Prevent clobbering built-ins.#3016

Merged
Dunbaratu merged 6 commits intoKSP-KOS:developfrom
Dunbaratu:prevent_builtin_masking
Feb 13, 2022
Merged

Prevent clobbering built-ins.#3016
Dunbaratu merged 6 commits intoKSP-KOS:developfrom
Dunbaratu:prevent_builtin_masking

Conversation

@Dunbaratu
Copy link
Member

@Dunbaratu Dunbaratu commented Sep 13, 2021

(TODO: Read through the issues backlog and find issues this is connected to and put the reference number here so this will close them. There's possibly more than one such issue.)

This implements a compile-time check that makes it illegal for a script (or the interpreter) to create an identifier that clashes with any of the following:

  • The built-in variable names (SHIP, NORTH, ALTITUDE, VELOCITY, etc)
  • The system locks (THROTTLE, STEERING, etc)
  • The built-in function names (PRINT(), HEADING(), V(), R(), etc)

Note that this is a compile-time check, so it detects the problem and complains even if the offending line of script code is never reached and executed.

The need for this PR is this:

Once one of these built-in identifiers gets clobbered by the script, it often cannot be accessible again for the rest of that program. (or the entire interpreter context).

The danger with this PR is this:

Because this was never enforced before, There's probably lots of scripts out there that may have accidentally used a name that masked a built-in, like the example below:

// R() is the built-in rotation constructor, and this next line
// replaces it with a new user-defined R() "function":
lock r to 1.

// But if the script never tries to use R() from here on,
// it might have worked anyway, blissfully unaware that it wiped out
// a built-in function.

This enforcement in this PR, while better practice, would break existing scripts that have things like the situation above, and require having them rename such variables. If it's a script the user is actively working on, that's no big deal. If it's a script the user found out on the internet, it is.

Thus there's an option to turn the old behavior back on again:
There's going to be two ways to toggle whether this rule is being enforced:

The default global behavior comes from: CONFIG:CLOBBERBUILTINS

There will also be a way to override it per-file with the compiler directive @CLOBBERBUILTINS on., which isn't implemented yet in the current state of this PR.

[x] Add @CLOBBERBUILTIN directive, which requires some
grammar file changes which is why I didn't tackle it yet.

[x] Add user-facing documentation about this, which means
mentioning it in several place through the docs, and
mentioning how older scripts you find on the internet
might break because of it unless you change the setting,
etc, etc, etc.

Fixes KSP-KOS#3005 - The discussion of the problem is long, see the issue KSP-KOS#3005.

There was logic in the compiler that handled this situation:

When in the context of compiling the lefthand side of a SET assignment,
if that lefthand side is a function name, it should suppress the "invoke
even when no parentheses are present" logic, like so:

```
  set x to { return 1. }. // x is now a delegate function.
  print x. // Should call x() the function, even with the () missing.
  set x to 3. // Should NOT call x() when replacing x with a 3.
```
The problem was that this same logic that tries to handle that was
partly firing off when the 'x' in question was followed by a suffix,
when that should not be happening in that case:
```
  set x to { return ship. }.
  set x:name to "hello". // Here x() DOES need to be called, to return
                         // SHIP, which then gets its suffix :name set.

```
It was partially skipping half the work of setting up for the function
call because it failed to realize one was coming.  The part it skipped
was the part that inserts the KOSArgMarker on the stack before the call
happens.
The eventual plan is to be able to toggle this feature off
in two ways - one is the CONFIG:CLOBBERBUILTINS setting for the
default, and the other is a compiler directive @CLOBBERBUILTINS
that can override the default on a per-file basis.  That compiler
directive isn't there yet in this PR, but you can test turning
the feature on and off via the CONFIG option.

What's needed for me to make the PR merge-ready:

[ ] Add @CLOBBERBUILTIN directive, which requires some
grammar file changes which is why I didn't tackle it yet.

[ ] Add user-facing documentation about this, which means
mentioning it in several place through the docs, and
mentioning how older scripts you find on the internet
might break because of it unless you change the setting,
etc, etc, etc.
@Dunbaratu Dunbaratu added Pending Documentation Author is warning Reviewer not to merge this PR yet. User doc changes aren't in the PR yet. Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. labels Sep 13, 2021
@Dunbaratu
Copy link
Member Author

Note: one problem to try to fix - the exceptions are coming out saying the line number and column but not the offending filename. It's being reported in a different format than usual because it's coming from the compiler and not using the normal compiler exception handling - I need to fix that too.

When Compler.cs throws KOSCompileExceptions, it tends to
print the filename/line/col now.

However, KOSNumberParseException, which is also thrown
from the compiler, still doesn't show the info.  That's
because it's not derived from KOSCompileException, and
gets thrown by StringValue.ToNumber(), which also gets
called from contexts that aren't the compiler, so it
can't contain the file/line/col info.  Fixing that is
a bit more complex, but what this commit does is still an
improvement over what it was doing before, so it's still
worth checking in.
@Dunbaratu Dunbaratu removed Pending Documentation Author is warning Reviewer not to merge this PR yet. User doc changes aren't in the PR yet. Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. labels Sep 16, 2021
@Dunbaratu
Copy link
Member Author

Problem found: Today I was playing with this and tried to do this:

lock heading to 3.

This (correctly) issued the error message telling me that I can't lock the heading because that would clobber the built-in heading().
BUT, it (incorrectly) still broke the heading() function anyway, despite the error message telling me it caught the problem and claimed it refused to do it. from then on, I could not lock steering to heading(90,0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant