(Possible) Breaking change: local variables references inside nested function definitions
-
Hi everybody,
I've found a really weird bug that might cause issues with nested function calls. It gets some bonus points in evilness because it only happens if
ENABLE_SCRIPTING_BREAKPOINTS
is disabled (which is usually the case in compiled plugins), so your script will run fine in HISE but fail inexplicably in the compiled plugin.It is caused when you define a function inside a inline function and reference a local variable of the (outer) inline function inside the inner function:
A simple example (and the use case where I found it) is the asynchronous execution after the user browses a file:
inline function onBrowse(component, value) { local c = component; FileSystem.browse(FileSystem.Desktop, function(f) { if(c == myExpectedControl) doSomething(); }); }
So by the time the inner function gets executed, the local variable has gone out of scope and has been resetted to
undefined
(but by havingENABLE_SCRIPTING_BREAKPOINTS
enabled, the lifetime of local variables is extended so you can inspect them in the ScriptWatchTable, which masks the bug in HISE itself).So the fix is pretty easy: don't allow local references in nested function calls. I've added a compile error on the parsing level, so your scripts will not compile (this way you don't have to test all functions like a runtime error), but this might "break" a few scripts out there (in the sense that they don't compile if they would not work in the exported plugin).
This bug can occur anywhere you are using nested function definitions - server calls, file browsing, anything asynchronous.
-
@christoph-hart Would using
reg
orvar
in place oflocal
be the way to go, or should we place the variable outside of the inline function entirely? -
@d-healey This is the same actually - if you define a
var
inside a inline function, it will get added to the global scope anyway, but sure, writing it in the global scope is a bit cleaner.inline function setX() { // ugly as f*** var x = 90; } setX(); Console.print(x);
-
@christoph-hart Would it be possible to pass arbitrary values as parameters into the nested function?
-
@d-healey Depends on the context, but if the API call expects a function with a certain amount of parameters, it's impossible. But I can understand the rationale of it to avoid polluting the global namespace with variables that hold internal information.
-
@christoph-hart said in (Possible) Breaking change: local variables references inside nested function definitions:
But I can understand the rationale of it to avoid polluting the global namespace with variables that hold internal information.
Maybe using a single global object for these things would help keep it contained.
-
Nope, doesn't always workreg
seems to work, anything wrong with that? -
@Christoph-Hart Now that we have this error message I'm aware of how often I'm doing this kind of thing :p I'm just going to put
vars
everywhere, to hell with scope, who needs it! -
Ouch! that might explain a few things indeed!
-
What happens if an
inline function
contains a nestedfunction
which calls anotherinline function
and that secondinline function
containslocal
variables? I ask because I'm getting some suspicious crashes :) -
@d-healey This shouldn't affect this example - it would if you would define that second inline function inside the first one, but that's not possible anyway.
-
I'm adding
reg nest = {};
at the top of each namespace and putting my nested function variables inside there, not ideal but keeps things tidy. -
@christoph-hart said in (Possible) Breaking change: local variables references inside nested function definitions:
So the fix is pretty easy: don't allow local references in nested function calls.
What about previously finished projects? All of them must be modified now, because you can't fix?
Sorry but the above suggestion is not a "fix", it is a prevention workaround. I think rather than telling us to not allowing local references in nested function calls, this issue should be really fixed first. You should fix it.
-
@fortune said in (Possible) Breaking change: local variables references inside nested function definitions:
Sorry but the above suggestion is not a "fix", it is a prevention workaround
Of course it is a fix. There was a problem in the language that lead to unintentional behaviour so now it throws a proper error and prevents users from facing this issue.
And if your finished project doesn't have any issues that come from this problem, you don't need to do anything, but the next time you'll update it and it start throwing these errors, you can fix them and sleep better.
-
@christoph-hart I'm getting an error in my expansion installer callback on the line "local origin = hr;". I think this was largely copied from one of the example projects provided by @Christoph-Hart.
Just so I'm crystal clear, do I want to be using something other than 'local'?
I've had some users reporting that the expansion installer gets stuck trying to install the samples - I wonder if this is related...
inline function onINSTALLEXPControl(component, value) { if (value == 1) { local msg = "SELECT EXPANSION .hr1 FILE"; Engine.showYesNoWindow("SELECT .HR1 FILE", msg, function(response) { if (response) { FileSystem.browse(FileSystem.Downloads, false, "*.hr1", function(hr) { if (hr.toString(2) == ".hr1") { local origin = hr; // Scoped copy msg = "SELECT A LOCATION FOR THE SAMPLE FILES"; Engine.showYesNoWindow("SAMPLE LOCATION", msg, function(response) { if (response) { FileSystem.browseForDirectory(FileSystem.Samples, function(dir) { expHandler.installExpansionFromPackage(origin, dir); }); } }); } }); } }); } } Content.getComponent("INSTALLEXP").setControlCallback(onINSTALLEXPControl);
-
@danh You need to declare that variable as a reg outside of the function, put it at the top of your script/namespace
-
@d-healey ok so just:
reg origin = hr; // Scoped copy inline function onINSTALLEXPControl(component, value) { if (value == 1) { local msg = "SELECT EXPANSION .hr1 FILE"; Engine.showYesNoWindow("SELECT .HR1 FILE", msg, function(response) { if (response) { FileSystem.browse(FileSystem.Downloads, false, "*.hr1", function(hr) { if (hr.toString(2) == ".hr1") { msg = "SELECT A LOCATION FOR THE SAMPLE FILES"; Engine.showYesNoWindow("SAMPLE LOCATION", msg, function(response) { if (response) { FileSystem.browseForDirectory(FileSystem.Samples, function(dir) { expHandler.installExpansionFromPackage(origin, dir); }); } }); } }); } }); } } Content.getComponent("INSTALLEXP").setControlCallback(onINSTALLEXPControl);
-
@DanH yes this is example ticks all boxes for that issue, but I can't guarantee that there is nothing else wrong :)
But there is another code smell - you're declaring the local variable inside the nested function definition, which should not be possible (and I think the recent change will also prevent that, but I'll have to check).
In your case you might get away with just changing
local
tovar
for the origin definition. -
@christoph-hart yes I changed 'local origin = hr;' to 'var origin = hr;' and the errors were gone.
EDIT: It's all going wrong when trying to install an expansion now though
-
@danh said in (Possible) Breaking change: local variables references inside nested function definitions:
if (hr.toString(2) == ".hr1")
Magic number alert!!! You should use
hr.toString(hr.OnlyExtension)