TelluIoT / ThingML

The ThingML modelling language
https://github.com/TelluIoT/ThingML
Apache License 2.0
101 stars 32 forks source link

[JavaScript] Messages can't have the same names anymore #294

Closed fungiboletus closed 4 years ago

fungiboletus commented 4 years ago

The messages in different things with the same names will override each other.

import "datatypes.thingml" from stl

thing ThingA {
    message hello(m: String)

    provided port input {
        receives hello
    }
}

thing ThingB {
    message hello(canard: Integer)

    provided port input {
        receives hello
    }
}

configuration BugJs {
    instance a:ThingA
    instance b:ThingB
}

This will generate the following events.js code:

var Hello = /** @class */ (function () {
  function Hello(port,...params) {
    this.type = 'hello';
    this.port = port;
    this.canard = params[0];
  }

  Hello.prototype.is = function (type) {
    return this.type === type;
  };

  Hello.prototype.toString = function () {
    return 'event ' + this.type + '?' + this.port + '(' + canard + ')';
  };

  return Hello;
}());
exports.Hello = Hello;

var Hello = /** @class */ (function () {
  function Hello(port,...params) {
    this.type = 'hello';
    this.port = port;
    this.m = params[0];
  }

  Hello.prototype.is = function (type) {
    return this.type === type;
  };

  Hello.prototype.toString = function () {
    return 'event ' + this.type + '?' + this.port + '(' + m + ')';
  };

  return Hello;
}());
exports.Hello = Hello;
fungiboletus commented 4 years ago

@brice-morin I'm a bit stuck with this problem, but you are the one who wrote the code generator.

I think we can't use simply the message name as a classname but what can we use instead ?

fungiboletus commented 4 years ago

It probably appeared in 7db8763a6ab0be99eb570eb915ef9b64e43ec3fa related to #283

brice-morin commented 4 years ago

Should be easy to fix. Append the name of the thing where messages are defined. There should be a helper to find the containing thing. Alternatively, it should be safe to take myMsg.eContainer() and cast it as a Thing.

brice-morin commented 4 years ago

I am in a taxi right now, but I can have a look later on

fungiboletus commented 4 years ago

I'm working on a fix with the name of the thing added, but I think it will still be possible to break stuff with some unlucky names.

brice-morin commented 4 years ago

OK, good. I will write a test meanwhile.

fungiboletus commented 4 years ago

I tried something in 26c092a30a3fefdb8378e52a8ab72c848469ea5a but it looks like it was not as easy as I thought. Guards and probably actions are broken now.

brice-morin commented 4 years ago

OK, I'll have a look and run the tests

brice-morin commented 4 years ago

See my comment on your commit, fix, and then I can run the tests :-)

brice-morin commented 4 years ago

Note that I do not expect problems on guards and transitions, which use the port and type attributes of the classes generated from the messages.

fungiboletus commented 4 years ago

I did change the type attribute to also have the name of the thing at the end. I don't know whether it's required.

Guards generate broken code like this in my branch :

SafariMadnessWorkaround_null_Safari_Buffering_WaitBuffering.to(SafariMadnessWorkaround_null_Safari_Buffering_ProcessBuffering).on(Event.Timer_timeout_TimerMsgs).when((timer_timeout_TimerMsgs) => {
        return timer_timeout_TimerMsgs.port === 'timer' && timer_timeout_TimerMsgs.type === 'timer_timeout_TimerMsgs' && (timer_timeout.id === this.SafariMadnessWorkaround_waitBufferingTimerId_var);
    });
brice-morin commented 4 years ago

Yes, true, you did change the type attribute.....

I would say it is not necessary, as a given thing should not be able, AFAIK, to include and/or define two messages with the same name. I would expect the checker to complain, and if not, the parser to yield funny results in some cases... (I just check, and it seems the cheker is happy, but the parser indeed loses its mind)

But two different things should indeed be able to define a message each, with the same name.

So, IMHO, revert the type back to its simpler form. And be careful when you have two messages with the same name: keep them in separate things!

brice-morin commented 4 years ago

Well, I have been too fast in my previous comment. There is some cases where it can work, like having two things fragments, each defining a message m. If this message m is in a port p1 in the first fragment, and p2 in the second fragment, then you can safely include both fragments in a given thing, and the parser will work as expected. Nevertheless, the existing logic in JS using type and port should also work. So, again, revert the type back, as it will save you some changes in guards, transitions, etc :-)

fungiboletus commented 4 years ago

type has been reverted :-)

brice-morin commented 4 years ago

OK, seems like there is no regression in the tests. You can pull request and merge yourself :-)