Ziggy changes #81
Ziggy changes #81
Conversation
Merge to get the integration test update
There was a problem hiding this comment.
Hi @zlattala,
I've finished going through the set of changes. I have some questions about the changes in the grammar of RoboChart, whose changes may impact the RoboSim plug-ins.
I also have a few concerns regarding the changes in the validator. The most pressing are those that have been swapped to use 'contains' or 'containsAll'. We cannot, in general, use Java methods of collections to do these checks, because the notion of equality for Java does not match our notion (based on names of eg. variables/types). It's likely that our examples may not be sufficient to show all potential issues with these changes.
Thanks,
Pedro
| IntegerExp: value=INT; | ||
| StringExp: value=STRING; |
There was a problem hiding this comment.
@zlattala why are these necessary here, given they're declared within Atomic?
There was a problem hiding this comment.
I believe we discussed this, and it was fine to leave, but just to clarify, it is because ANNParameters requires a StringExp explicitly.
| SeqExp: | ||
| {SeqExp} | ||
| '<' ( | ||
| values+=PlusMinus (',' values+=PlusMinus)* | ||
| )? '>'; // need this to avoid conflict between greater than and closing sequences |
There was a problem hiding this comment.
Why has this changed? Before a SeqExp could also be defined by anothe Expression or SetExp. I can't rember why we had that format, but perhaps @alvarohm may be able to comment on that.
There was a problem hiding this comment.
I am not sure about this, but I have not changed it for now ^
| 'requires' rInterfaces+=[Interface|QualifiedName] | | ||
| variableList+=VariableList | | ||
| events+=Event | | ||
| clocks+=Clock |
There was a problem hiding this comment.
Why are clocks included? Controllers cannot have clocks.
There was a problem hiding this comment.
I have removed this now, thanks.
| //Similar definition to RoboSim but no InputContext and OutputContext class. | ||
| InputContext returns Context: | ||
| {InputContext} | ||
| 'inputcontext' '{' | ||
| ( | ||
| 'uses' interfaces+=[Interface|QualifiedName] | | ||
| events+=Event | ||
| )* | ||
| '}' | ||
| ; | ||
|
|
||
| OutputContext returns Context: | ||
| {OutputContext} | ||
| 'outputcontext' '{' | ||
| ( | ||
| 'uses' interfaces+=[Interface|QualifiedName] | | ||
| events+=Event | ||
| )* | ||
| '}' | ||
| ; |
There was a problem hiding this comment.
There's a potential problem here for RoboSim.xtext (which inherits from this) because it also has rules with the same name, and classes in the metamodel of the same name. I'll need to check RoboSim can reuse these before merging this into master.
There was a problem hiding this comment.
RoboSim also uses the syntax 'input' 'context' rather than 'inputcontext', and similarly for outputs. This is a minor detail, perhaps could make it consistent.
| /* C3 */ | ||
| val rVars = getRVars(s) | ||
| if (!rVars.forall[x|pVars.exists[v|x.name == v.name && ((v.type === null && x.type === null) || typeCompatible(v.type,x.type))]]) { | ||
| if (!pVars.containsAll(rVars)) { |
There was a problem hiding this comment.
I'm not sure this change is sound. The previous implementation checked variable names and their types. I'm not sure if the type-checker deals with this. May be worth checking we have examples that cover such cases.
There was a problem hiding this comment.
This is fixed, and with the new version, robochart success test13 catches this error.
There was a problem hiding this comment.
I have made this change, and added a test case for it ^
| var vs = "" | ||
| var started = false | ||
| for (v : rVars) { | ||
| if (!pVars.exists[x|x.name == v.name && ((v.type === null && x.type === null) || typeCompatible(v.type,x.type))]) { | ||
| if (!pVars.contains(v)) { |
There was a problem hiding this comment.
I don't think this is a sound change either. We cannot use the builtin Java methods otherwise we'll be comparing containment based on Java objects, whose equality notion is different from that of the RoboChart metamodel.
There was a problem hiding this comment.
I have made this change ^
| val calls = c.eAllContents.filter(Call) | ||
| val allOps = calls.map[ | ||
| val sig = operation | ||
| opDef(parent.LOperations.findFirst[OpEqual(sig, it)]) |
There was a problem hiding this comment.
This may be ok, but see problem above with changes in implementation of OpEquals, which are likely not sound.
| var vs = "" | ||
| var started = false | ||
| for (v : rVars) { | ||
| if (!pVars.exists[x|x.name == v.name && ((v.type === null && x.type === null) || typeCompatible(v.type,x.type))]) { | ||
| if (!pVars.contains(v)) { |
There was a problem hiding this comment.
I have made this change ^
| @@ -2385,7 +2477,7 @@ https://github.com/UoY-RoboStar/robochart-csp-gen/issues/39', | |||
|
|
|||
| // recursively collects outputs of a node container, avoiding operations already checked | |||
| // it is required by WFC O2 that operations not be recursive, but we can't know that has been checked beforehand | |||
| private def HashSet<Event> stmOutputSetInContextRecursive(StateMachineBody stm, Controller context, HashSet<OperationSig> alreadyChecked, StateMachineBody outerstm) { | |||
| /*private def HashSet<Event> stmOutputSetInContextRecursive(StateMachineBody stm, Controller context, HashSet<OperationSig> alreadyChecked, StateMachineBody outerstm) { | |||
There was a problem hiding this comment.
Why is this, and the following, commented out? Where is the replacement implementation for this?
There was a problem hiding this comment.
I have uncommented this, and removed the replacement code, as discussed.
| RoboChartPackage.Literals.CONNECTION__TO, | ||
| index | ||
| ) | ||
| if (ncOutputSet(stmDef(c.to as StateMachine)).contains(c.eto)) { |
There was a problem hiding this comment.
Why has this been changed?
There was a problem hiding this comment.
I have changed this back.
StringExp. This seems required, otherwise extensions of the grammar, eg. in robosim-textual cannot produce valid parsers for some unknown reason. I suspect it was because of the duplication of action/rules having similar names.
Creating a pull request so it's easier to track changes or add comments on them.