Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

function hoisting #274

Open
gfwilliams opened this issue Mar 29, 2014 · 13 comments
Open

function hoisting #274

gfwilliams opened this issue Mar 29, 2014 · 13 comments

Comments

@gfwilliams
Copy link
Member

When parsing function decls, parse and remove other function decls inside them.

This should have the affect of hoisting, and will speed up execution too!

@gfwilliams
Copy link
Member Author

Could be more difficult when converting functions back into strings though

@KeithJRome
Copy link

Converting functions back into source strings (using dump() for example) already doesn't work for nontrivial code. A simple example:

var foo_module = (function(){
  var foo = "hello, world from a secret place!";
  var bar = function() { console.log(foo); };
  return { 'bar': bar };
})();

This instantiates an object named foo_module using the revealing module pattern - which really just uses closures to simulate "private" members. In this example, "foo" is a private field of "foo_module", and "bar" is a public member that proves "foo" is still there, even though you cannot directly reach "foo". This is a very common pattern in real-world javascript code, almost necessary for projects of appreciable size.

>foo_module.bar()
hello, world from a secret place!
=undefined
>dump()
var foo_module = {
  "bar":function () { console.log(foo); }
};
=undefined
>foo_module.foo
=undefined
> 

Note that the last statement there actually adds a public undefined "foo" member to "foo_module", which is surely a bug. It looks like during the process of looking up a member, espruino is actually creating the member if it doesn't already exist:

>dump()
var foo_module = {
  "bar":function () { console.log(foo); },
  "foo":undefined};
=undefined
>

@rwaldron
Copy link
Contributor

Not sure I follow the explanation given, re: parsing and removing? Can you clarify? Functions don't hoist beyond the function scope (or global scope) that they are declared in, maybe I'm not following correctly?

@gfwilliams
Copy link
Member Author

@KeithJRome That's already covered on these two (6 month old) bugs: #35 and #63. Both of them are really easy to find, so please search next time. If you want to fix them, pull requests are always appreciated ;)

@rwaldron yeah - it's more a note for me :) The idea is to turn this (approximation of Espruino's symbol table) :

{
  a : {
   code : "function b() { print('hello'); } print('bar'); b();"
  }
 }

into:

{
  a : {
   b : { code : "print('hello');" },
   code : "print('bar'); b();"
  }
 }

Obviously then you have an issue when converting that function back into a string, for example for edit() or dump() - but for now, the declared functions could just be dumped right at the top.

@rwaldron
Copy link
Contributor

@gfwilliams ah, got it.

@KeithJRome
Copy link

I had actually seen those before; I wasn't attempting to report new bugs. I was simply pointing out that your concern ("Could be more difficult when converting functions back into strings though") didn't really matter until/if the other was dealt with first. The second bug mentioned (accessing a nonexistent member) was brought up simply because anyone who tries that code snippet I posted would likely notice it and I didn't want that to overshadow what my actual point was.

@KeithJRome
Copy link

Your point is taken though. I'll try to make some time to contribute an actual pull request or two. I've just not had the bandwidth for it yet due to an extremely heavy workload at the moment.

@bartmichu
Copy link
Contributor

I was looking for a reason why this example code fails and it looks like it's the right place.

function test(){
  function f() {
    return "not very good";
  }
  return f();
  function f() {
    return "yay";
  }
}

It fails with:

>test()
at line 6 col 3
  function f() {
   ^
in function "test" called from line 1 col 6
>

@gfwilliams
Copy link
Member Author

Hmm. This looks like a slightly different bug though - while it would get the wrong result, it shouldn't error. I've just added a new one for it.

@gfwilliams
Copy link
Member Author

When implementing this: There's a kind of variable hoisting too - it just hoists undefined:

var foo = 2;
function a() {
  return foo;
  var foo = 4;
}
console.log(a());
// undefined

this could probably be implemented in almost the same way:

  • keeping the original variable decl
  • define a variable inside the function
  • when prepping an execution context (parsing arguments into local variables), make sure we create a new undefined variable

@rm-rf-etc
Copy link

rm-rf-etc commented Nov 15, 2016

Wow, was really surprised to see function hoisting isn't working. This will break any JS modules that use hoisting, which I assume is a lot. Wanted to provide a code sample and the error I see.

var UI = require('morainterface');
var board = new UI();
board.addButton(B3, makeCallback(B3));
board.addButton(B4, makeCallback(B4));
board.addButton(B5, makeCallback(B5));

function makeCallback(name){

    name = name.toString();
    return function(e) {
        console.log(name, e.state);
    };
}
Uncaught ReferenceError: "makeCallback" is not defined
 at line 1 col 21
board.addButton(B3, makeCallback(B3));
                    ^
Uncaught ReferenceError: "makeCallback" is not defined
 at line 1 col 21
board.addButton(B4, makeCallback(B4));
                    ^
Uncaught ReferenceError: "makeCallback" is not defined
 at line 1 col 21
board.addButton(B5, makeCallback(B5));
                    ^
=undefined

@gfwilliams
Copy link
Member Author

It's surprisingly few modules, because you only hit it if you execute the code at the root scope of the module (so it executes as soon as the module is loaded), which seems like bad practice anyway. If you put your initialisation in a function - which, as you're making a UI, you might want to do after the screen is initialised - you wouldn't have the problem.

From the errors you're showing, you're not uploading a module, but normal JS code from the Web IDE?

In that case, it's impossible to fix. Each command is executed in sequence, as soon as it's loaded into Espruino - it's done so that you can load a text file that is bigger than might otherwise fit in Espruino's memory if it were loaded as one chunk... But that means that when Espruino executes board.addButton(B3, makeCallback(B3)); it has never even received the characters for makeCallback and has no idea what it is.

@trusktr
Copy link

trusktr commented Jun 2, 2017

Interesting. I stumbled on this too. But I can appreciate why it works the way it does while scanning the code in one go.

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

No branches or pull requests

6 participants