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

State machines with capitalized names fail when referenced within other state machines #593

Closed
Nava2 opened this Issue Aug 26, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@Nava2
Member

Nava2 commented Aug 26, 2015

Originally reported on Google Code with ID 695


What steps will reproduce the problem?

  1. Try the following umple code with two state machines - sm and Other:
class A 
{
  sm 
  {
    s1 
    {
      entry / {Other = Other.os2;}
    }
  }

  Other
  {
    os1
    {

    }
    os2
    {

    }
  }
}
Notice that entry into s1 of sm causes other to move into state os2.
  1. Generate Java code for this Umple model.
  2. Compile the Java code.

The Java compilation fails, with the following error:

A.java:60: error: cannot find symbol
      Other = Other.os2;
      ^
  symbol:   variable Other
  location: class A
1 error

This code does not fail Java compilation if the second state machine is named with
a lowercase name (ie "other") nor does it fail if the first state machine ("sm") does
not reference the second state machine (ie. if the entry action is removed).

This bug is present in the "Traffic Lights B" example on UmpleOnline.

Please use labels and text to provide additional information.


Reported by @CraigBryan on 2015-03-31 13:55:59

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 3, 2016

This is more of a bug in the example. Fix that first (easy).

The code 'Other = Other.os2;' is user-written, so further analysis ought to be done to see whether there is a simple workaround before attempting to do any changes to code generation.

In most cases we do not actually want users directly forcing state changes through code since that will bypass all of the mechanisms in state machines for triggering actions and so on. It should only be done for eventless state machines (enumerations) like in this example.

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Jan 16, 2017

What Causes the Issue

After investigating today, I found that when a state machine is used in an assignment statement, the Java code does not use the private variable that is created for the state machine. This occurs when entry, exit, and transition actions contain assignments for state machines. In the above example, for instance, a private variable "other" is created in the Java code to keep track of the "Other" state machine's state. In the generated Java code, however, the entry action for "sm" does not use the private variable to set the "Other" state machine's state. Instead, it copies the original assignment statement from the Umple code. Using lowercase "other" in the above example works because it matches the name of the private variable.

// From the generated Java code
private Other other;

public boolean setSm(Sm aSm)
{
    if (sm != Sm.s1 && aSm == Sm.s1 )
    {
      Other = Other.os2;
    }
    sm = aSm;
    return true;
}

public boolean setOther(Other aOther)
{
    other = aOther;
    return true;
}

It seems to me that if an action contains an assignment to a state machine, the Java code should call its corresponding setter method.

Solution Proposal 1

When users want to assign a state to a state machine directly in their Umple code, they must use the corresponding setter method. When I replaced "Other = Other.os2" with "setOther(Other.os2)" in the Umple code, the Umple code compiled, as well as the generated Java file. The Umple API guide lists the setter method in the state machine section http://cruise.eecs.uottawa.ca/umple/APISummary.html .

Solution Proposal 2

When an assignment to a state machine is detected in the Umple code, it should be replaced by a call to the state machine's setter function in the generated code. This solution seems more complicated than the first solution I proposed.

I am wondering if either of these solutions is on the right track? If so, I am wondering if someone could please direct me to the general area where I should be looking to make the fix and the corresponding tests? So far, I have been exploring the directories listed in https://github.com/umple/umple/wiki/UmpleArchitectureStateMachines .

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Jan 16, 2017

Currently, Umple doesn't parse the implementation code written for do activities, entry and exit actions (plus methods). To me, the code is wrong. Therefore, not a big deal.
The proposal one is what I support. Once Umple parses the implementation code, we could get rid of this.
I think @TimLethbridge also pointed out this.

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 16, 2017

Yes, Simply fix the example to call the set method. That is the established practice.

I can see as a future effort the possibity of scanning all code blocks (methods, actions, activities etc.) looking for direct access to variables and warning to replace with set/get methods. But that is complex and would be a separate issue if we decide to do it.

So for now, simply fix the example

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Jan 17, 2017

@TimLethbridge
My PR #955 containing the fix for the traffic light example has been accepted. While I was working on this fix, I did not see any tests that checked if the code generated by the UmpleOnline examples was correct (ex. no .java.txt files). I am wondering if there is a reason that these tests have not been included?

Also, would you like me to open a separate issue regarding displaying a warning to users to use Umple's set and get methods instead of directly accessing a variable?

May this issue be closed now?

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Jan 17, 2017

showing such warnings needs parsing actions.

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Jan 18, 2017

@vahdat-ab would you like me to open a separate issue for the warnings? and also should this issue be closed?

@jblang94 jblang94 closed this Jan 18, 2017

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Jan 18, 2017

No need to that.
I keep this closed because when Umple starts parsing manual code, it will be dealt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment