Prevent clobbering built-ins.#3016
Merged
Dunbaratu merged 6 commits intoKSP-KOS:developfrom Feb 13, 2022
Merged
Conversation
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.
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.
Member
Author
|
Problem found: Today I was playing with this and tried to do this: This (correctly) issued the error message telling me that I can't lock the heading because that would clobber the built-in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(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:
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:
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:CLOBBERBUILTINSThere 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.