Scoping bug when defining a function from a previous scope

[bobrippling]

Minimal repro:

>{ let scope = "first"; function fn() { console.log("in fn, scope = " + scope); } }
=undefined
>scope
Uncaught ReferenceError: "scope" is not defined  // as expected, all good
 at line 1 col 1
scope
^
>fn
=function () { ... } // I believe this is incorrect - functions (specifically the assignment) shouldn't be hoisted out of block scope
>fn()
in fn, scope = first // ignoring the hoisting, this is ok - `scope` is closed-over by `fn`
=undefined
>var scope = "second"; function fn() { console.log("in second fn, scope = " + scope); }
// ^ this appears to replace `fn` with the new definition, but reuse the original fn's scope, as seen next:
=function () { ... }
>fn()
in second fn, scope = first // this should be "scope = second"
=undefined

I believe this bug is the combination of two problems:

• Replacing a function definition uses its scope and ignores the new function's scope
• Functions are hoisted out of their block scope

This is visible, for example, when the settings app loads the recorder app - both scripts reference a settings variable, and both define a showMainMenu function. The recorder app's showMainMenu then inherits the settings app's settings, causing bugs/exceptions.

---

Further details on why I believe the hoisting (of the assignment) shouldn't happen:

function f(){
  if(false){
    function g(){
      console.log("g")
    }
  }
  console.log("g:", g); // undefined
}

as it's equivalent to:

 function f(){
+  var g;
   if(false){
-    function g(){
+    g = function(){
       console.log("g")
     }
   }
   console.log("g:", g); // undefined
 }

[bobrippling]

As a minimal fix (which would resolve the settings app problem), I reckon if we updated the outer function's scope, that'd do it - provided this didn't tangle with the inner function

[gfwilliams]

Hmm - thanks! That's a tricky one.

However the functionality exists so that people can edit a function on demand via the REPL - basically replacing its code while running it in the scope it was originally defined in.

It feels like defining the same function twice in normal code is an error?

Perhaps we should be using a different codepath for redefining a function in code though - as when you do edit(...) it can use .replaceWith if the function is nontrivial.

This is visible, for example, when the settings app loads the recorder app

Oh, you mean https://github.com/espruino/BangleApps/blob/master/apps/recorder/settings.js ?

This shouldn't be an issue at all with a normal load() - have you got fastloadutils installed or something like that? It's one of the reasons I really didn't want people using it, because IMO it's only an issue because settings doesn't properly unload itself (by having defined all its functions in its own scope)?

But normally we would never fast load because settings doesn't add a remove handler in setUI (which is what we use to signal an app is capable of unloading for fast load).

[bobrippling]

Yes - I imagined replaceWith used a different technique to keep the scope, but I guess it just replaces it exactly like this? But yes I would agree, perhaps a different codepath for definition a function vs. calling replaceWith seems reasonable.

But yes - I believe this is a recorder specific issue, its settings app loads its app, which is fastloaded into (since both it and settings have Bangle.loadWidgets).

As a fix, I think we just disable fastload for settings - I already had to put in a similar fix for DST. I'll let you decide what you think about redefining functions via code / this issue

[gfwilliams]

So are you saying this happens normally? Or only with the fastload app installed?

Because I don't think we should be going around putting hacks in all the applications to deal with the fact that fastload is itself a massive hack.

Do you think we can just put exceptions into the fastload app itself? We already have appFastloadPossible in https://github.com/espruino/BangleApps/blob/master/apps/fastload/boot.js#L25 so that could just check __FILE__=="setting.app.js" && n!==undefined and then slow load? And maybe take the code out of widdst.

I think it also makes sense for all the apps that have issues with fastload to be listed within fastload itself as in future it might help with efforts to find a better way of handling things.

But really the settings app defines global functions which are going to be a memory leak, so we shouldn't be fast loading out of it at all, even if it's to a clock as it's going to leave a bunch of stuff defined (and it's a big old app to begin with).

Perhaps fastload should look in global and see if any functions/vars are defined in there apart from the usual? If they are it should maybe warn or maybe even refuse to fast load?

---

Also maybe for the function replace we should just throw an exception if we try and replace a function with another function that itself has a scope defined? Because that's almost certainly an error?

[bobrippling]

No, just with fastload - I'd expect fastload to not need an exception for settings though. @thyttan @halemmerich, what would you say if we altered fastload to only allow fastloading out of an app if the app had set a Bangle.uiRemove ?

This would cover settings and other cases where apps are unaware of fastload, at the possible pessimisation of apps that might just work with it. Alternatively we add an exception to fastload for the settings app (and possibly others as we find them).

---

@gfwilliams yes, r.e. the function replacing, I think we should throw an exception or fully replace the previous function, to avoid the existing behaviour which takes a bit of tracking down (but we preserve replaceWith's behaviour - I've used it once when debugging event handlers).

[thyttan]

what would you say if we altered fastload to only allow fastloading out of an app if the app had set a Bangle.uiRemove ?

I thought this was already part of the logic?

This [Fastload utils] allows fast loading of all apps with two conditions:

• Loaded app contains Bangle.loadWidgets. This is needed to prevent problems with apps not expecting widgets to be already loaded.
• Current app can be removed completely from RAM.

But I'll leave the technicalities to @halemmerich since he's the brains on this. ;)

[gfwilliams]

Current app can be removed completely from RAM.

I think this is the issue - fastload doesn't attempt to check if this is the case, or do anything to try and unload (like clearing the global scope).

The thing is any app can call Bangle.load() to fast load another app now, and if setUI({remove:...}) has been done then it will fast load.

It's a very easy change to make and can be done to any app once it's been checked that the app can totally unload itself (as is loading into an app with widgets), and then it'll run fast for everyone.

So fastload basically exists to force apps that were never intended to have fast load (or were explicitly designed to use load rather than Bangle.load because they don't work) to fast load.

Personally I think it shouldn't exist. If we took all the time we spent tracking down fastload related issues, and instead spent it just changing load() to Bangle.load() in apps that supported it (or tweaking them), Bangle.js would be running fast and reliable for everyone, instead of being in this state where people install fastload and it basically makes their device unreliable and gives it memory leaks.

[thyttan]

If we took all the time we spent tracking down fastload related issues, and instead spent it just changing load() to Bangle.load()

I'm somewhat sympathetic towards this stance. But one thing that I think is not straight forward is, with fastload utils you have it sit in the middle and check weather the next app uses widgets and in that case it allows a fastload (if the current app has a remove handler). So you have for every app going out from the launcher the best possible loading speed.

Maybe that logic could be put into Bangle.load though - but I suspect you're maybe not the biggest fan of that @gfwilliams ? 😅😇

And also there's the App history feature in fastload utils that I'm personally quite fond of 😛

[thyttan]

Maybe one could argue all apps should just load widgets and if not wanted just hide them? Then it would maybe be OK to hardcode Bangle.load-ing out of launchers into apps.

[gfwilliams]

Thanks - yes, that's a good point. So the way I see it right now:

• When loading an app from a launcher which you do 99% of the time, the launcher is usually designed to unload itself and fastload is basically safe and provides a great speed boost
• When used from any other app (apart from a fastload-capable clock) fastload is almost certainly a disaster, but then this is quite a small proportion of loads that are done as a lot of times you long-press to go back to the clock.

Do you think that's about right?

So, we could disable fastload (by default, maybe via an option?) for anything that's not a clock or a launcher?

OR, we currently have this magic 'launch cache' code that's duplicated in every main launcher: https://github.com/espruino/BangleApps/blob/master/apps/launch/app.js#L23-L39

And that's felt like a bad idea for a while (especially as they all use the same cache file!) - so how about we make a module for it, and also include the fastload check for loadWidgets in there, which we could do when populating the cache in the first place? And then provide a function in that module for launching apps which takes that into account?

I feel like that'd be a good solution which is safe(ish) and works for everyone?

The other benefit is of course the issue with fastload is it's a whole bunch of code that gets loaded on boot whenever you don't fast-load an app, so it actually makes the non-fastload apps even slower to load. If you were to only load that code from the launcher you're not slowing any of the other apps down, but are getting the speed improvement when you can use it.

For the small percentage of apps that do load other apps (and where speed is an issue) they could be modified manually to do Bangle.load (but of course they have to know about the widgets). Going back to the clock is probably the main one and you can use Bangle.showClock for that (which does fast load).

[gfwilliams]

Just to add I guess settings is one of the most-used apps and that could be made fast-load capable anyway by doing the usual (adding {...} around the outside and defining functions/vars with let), then using Bangle.showClock() to go back rather than load()

[thyttan]

When loading an app from a launcher which you do 99% of the time, the launcher is usually designed to unload itself and fastload is basically safe and provides a great speed boost

Yes, if the next app uses widgets.

When used from any other app (apart from a fastload-capable clock) fastload is almost certainly a disaster, but then this is quite a small proportion of loads that are done as a lot of times you long-press to go back to the clock.

Yes, but doesn't this solve itself by apps other than clocks and launchers not having a remove handler set? (bar e.g. spotrem which I've broken the rules with and added a remove handler so that if fastload utils is installed it fastloads the clock on exit - I don't think this should be a consideration now maybe, and I could revert that in spotrem if we want.)

So, we could disable fastload (by default, maybe via an option?) for anything that's not a clock or a launcher?

As I said abouve I think this is already, virtually at least, the case? But maybe it's a good idea for further flexibility. I don't get an immediate feeling either way.

OR, we currently have this magic 'launch cache' code that's duplicated in every main launcher: https://github.com/espruino/BangleApps/blob/master/apps/launch/app.js#L23-L39

And that's felt like a bad idea for a while (especially as they all use the same cache file!) - so how about we make a module for it, and also include the fastload check for loadWidgets in there, which we could do when populating the cache in the first place? And then provide a function in that module for launching apps which takes that into account?

I feel like that'd be a good solution which is safe(ish) and works for everyone?

Yes, without thinking about it very much that feels like a great idea to me!

The other benefit is of course the issue with fastload is it's a whole bunch of code that gets loaded on boot whenever you don't fast-load an app, so it actually makes the non-fastload apps even slower to load. If you were to only load that code from the launcher you're not slowing any of the other apps down, but are getting the speed improvement when you can use it.

Yes, I agree it's a shame to slow down non-fastloadable apps even further.

For the small percentage of apps that do load other apps (and where speed is an issue) they could be modified manually to do Bangle.load (but of course they have to know about the widgets). Going back to the clock is probably the main one and you can use Bangle.showClock for that (which does fast load).

This sounds like a good idea to me and I'd probably like to do that for e.g. spotrem which I go back and forth to from the watch via quicklaunch quite often.

Just to add I guess settings is one of the most-used apps and that could be made fast-load capable anyway by doing the usual (adding {...} around the outside and defining functions/vars with let), then using Bangle.showClock() to go back rather than load()

OK - I always assumed the the full bootcycle was needed to rewrite the settings. But thinking about it that's probably not the case?

[gfwilliams]

Yes, but doesn't this solve itself by apps other than clocks and launchers not having a remove handler set?

I don't think fastload cares? It doesn't check Bangle.uiRemove in appFastloadPossible as far as I can see.

As I said abouve I think this is already, virtually at least, the case?

Pretty sure not - this whole issue wouldn't be a problem if it did. setttings doesn't define a remove handler afaik so it shouldn't fastload. But it does and so it breaks.

This sounds like a good idea to me and I'd probably like to do that for e.g. spotrem which I go back and forth to from the watch via quicklaunch quite often.

Cool, yes. I think if you did it now, you'd at minimum get a fast swap to the clock from spotrem, and then maybe later even a fastload without the fastload app.

OK - I always assumed the the full bootcycle was needed to rewrite the settings. But thinking about it that's probably not the case?

Hmm - good point. How often do you go into settings and then not change anything? I guess maybe that means it's not so important to make fast loadable.

[thyttan]

Yes, but doesn't this solve itself by apps other than clocks and launchers not having a remove handler set?

I don't think fastload cares? It doesn't check Bangle.uiRemove in appFastloadPossible as far as I can see.

As I said abouve I think this is already, virtually at least, the case?

Pretty sure not - this whole issue wouldn't be a problem if it did. setttings doesn't define a remove handler afaik so it shouldn't fastload. But it does and so it breaks.

I see your point now - yes. But that sounds to me like it's a bug rather then the supposed behaviour of Fastload Utils - e.g. as described by the readme and what I recall from the discussions around when it was conceived?

Maybe we give it a day or two and give @halemmerich a chance to weigh in?

[halemmerich]

It has been a few more days, but I had a look at fastboot. The idea was to only fastload into incompatible apps, never fastload out of them. That would be as @gfwilliams correctly said completely bonkers.
For that the implementation checks if the target app has widgets (if the current app has them too) and then tires to fastload into the new app. I think there is no actual check necessary (in fastload itself) for the uiRemove, since the the original Bangle.load() actually checks for that and cleans up before evaling the new app. If there is no uiRemove, it won't do that and load normally.
Fastload replaces the global load to prevent the full reset by calling Bangle.load instead.
It replaces Bangle.load only to facilitate other features, not the loading itself (loading screen, app history, autoload launcher). If those extra features are not active it just calls the wrapped, original Bangle.load.

I did not yet have a look at why settings could fastload the recorder app, but it absolutely should not. I do not see settings set uiRemove at all. Maybe there is some remnant remove handler from some other app which should have been cleaned up?

It actually does not fastload on my watch when going from settings to the recorder app.