espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.75k stars 741 forks source link

`new A()` should ignore return value of `A` if it is not an object #848

Open ceremcem opened 8 years ago

ceremcem commented 8 years ago

Hi,

I'm using Livescript to write code, then compile it into Javascript. class functions that Livescript supports are throwing an error. Here is a Livescript version:

class x
    ~>
        @a = \hi

test = x!
test.a

...and here is the compiled Javascript:

var x, test;
x = (function(){
  x.displayName = 'x';
  var prototype = x.prototype, constructor = x;
  function x(){
    var this$ = this instanceof ctor$ ? this : new ctor$;
    this$.a = 'hi';
    return this$;
  } function ctor$(){} ctor$.prototype = prototype;
  return x;
}());
test = x();
test.a;

Output of Node.js:

'hi'

Output of Espruino:

Uncaught Error: Field or method "displayName" does not already exist, and can't create it on undefined
 at line 1 col 2
x.displayName = 'x';
 ^
in function called from line 10 col 3
}());
  ^
Uncaught Error: Function "x" not found!
 at line 1 col 8
test = x();

What might the problem be?

ceremcem commented 8 years ago

Here is a much simple class definition:

var x;
function Led(pin){
  this.pin = pin;
  return this.mode = 'turn-off';
}
Led.prototype.blink = function(){
  return function lo(op){
    if (this.mode === 'blink') {
      this.write(true);
    }
    return sleep(1000, function(){
      if (this.mode === 'blink') {
        this.write(false);
      }
      return sleep(1000, function(){
        if (this.mode === 'blink') {
          return lo(op);
        }
      });
    });
  }(function(){});
};
Led.prototype.write = function(val){
  return digitalWrite(this.pin, val);
};
({
  turnOff: Led.prototype[function(){
    this.mode = 'turn-off';
    return this.write(false);
  }]
});
({
  turnOn: Led.prototype[function(){
    this.mode = 'turn-on';
    return this.write(true);
  }]
});
x = new Led(12);
x.mode;

Here is the output:

>x = new Led(12)
="turn-off"
>x.blink()
Uncaught Error: Field or method "blink" does not already exist, and can't create it on String
 at line 1 col 2
x.blink()
 ^
>x.mode
Uncaught Error: Field or method "mode" does not already exist, and can't create it on String
 at line 1 col 2
x.mode
 ^
>x
="turn-off"
>
gfwilliams commented 8 years ago

The problem is your function Led is returning the string 'turn-off'. Literally just remove the return:

function Led(pin){
  this.pin = pin;
  this.mode = 'turn-off';
}

It looks like Espruino should detect this and just throw away the result rather than returning it if it's not an object:

The object returned by the constructor function becomes the result of the whole new expression. If the constructor function doesn't explicitly return an object, the object created in step 1 is used instead. (Normally constructors don't return a value, but they can choose to do so if they want to override the normal object creation process.)

But IMO this is broken JS code - you should not be returning a string, because it will just be ignored.

gfwilliams commented 8 years ago

Oh, and your initial error is because Espruino doesn't do function hoisting - but honestly it's pretty lazy code produced by the compiler... It could easily have put the function decl at the top of the block, making the code way more readable.

ceremcem commented 8 years ago

Thank you. When I omitted the return value , it simply worked. I had to make some adoptions and here is the last working version of the code fyi:

!function Led pin
    self = this
    @pin = pin
    pin-mode @pin, \output
    @mode = \turn-off

    @write = (val) ->
        digital-write @pin, val

    @blink = ->
        self.mode = \blink
        <- :lo(op) ->
            self.write on if self.mode is \blink
            <- sleep 1000ms
            self.write off if self.mode is \blink
            <- sleep 1000ms
            return lo(op) if self.mode is \blink
            return op!
        console.log "blink ended..."

    @turn-off = ->
        @mode = \turn-off
        @write off

    @turn-on = ->
        @mode = \turn-on
        @write on

data232 = new Led 13

...and here is the compiled Javascript:

function Led(pin){
  var self;
  self = this;
  this.pin = pin;
  pinMode(this.pin, 'output');
  this.mode = 'turn-off';
  this.write = function(val){
    return digitalWrite(this.pin, val);
  };
  this.blink = function(){
    self.mode = 'blink';
    return function lo(op){
      if (self.mode === 'blink') {
        self.write(true);
      }
      return sleep(1000, function(){
        if (self.mode === 'blink') {
          self.write(false);
        }
        return sleep(1000, function(){
          if (self.mode === 'blink') {
            return lo(op);
          }
          return op();
        });
      });
    }(function(){
      return console.log("blink ended...");
    });
  };
  this.turnOff = function(){
    this.mode = 'turn-off';
    return this.write(false);
  };
  this.turnOn = function(){
    this.mode = 'turn-on';
    return this.write(true);
  };
}
data232 = new Led(13);
gfwilliams commented 8 years ago
  /* FIXME: we should ignore return values that aren't objects (bug #848), but then we need
   * to be aware of `new String()` and `new Uint8Array()`. Ideally we'd let through
   * arrays/etc, and then String/etc should return 'boxed' values.
   *
   * But they don't return boxed values at the moment, so let's just
   * pass the return value through. If you try and return a string from
   * a function it's broken JS code anyway.
   */