After/Before Statements generate code with methods incorrectly #1002

Closed
a3994288 opened this Issue Feb 19, 2017 · 8 comments

Comments

@a3994288
Contributor

a3994288 commented Feb 19, 2017

Brief Description

When try to insert codes before/after a method, it either generate an error or generate Java code incorrectly.

Minimum Steps to Reproduce

Consider the following code:

class Student
{
  String textdata(){
    
  }
  before textdata { 
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}

Expected Result

no error

Actual Result

Warning on line 6 : Method 'textdata' cannot be found. Injection was ignored.. More information (1012)

@a3994288

This comment has been minimized.

Show comment
Hide comment
@a3994288

a3994288 Feb 19, 2017

Contributor

Minimum Steps to Reproduce

class Student
{
String textdata( Integer i ){
}

before !textdata( Integer i ){
if ( DEBUG) { System.out.println("accessing the name"); }
}
}

Expected Result (Generate Java Code)

public class Student
{
  public Student()
  {}
  public void delete()
  { if ( DEBUG) { System.out.println("accessing the name"); }
  }
  public String textdata(Integer i){
  }
}

Actual Result

public class Student
{
  public Student()
  {}
  public void delete()
  {}
  public String textdata(Integer i){
  }
  public before !textdata(Integer i){
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}
Contributor

a3994288 commented Feb 19, 2017

Minimum Steps to Reproduce

class Student
{
String textdata( Integer i ){
}

before !textdata( Integer i ){
if ( DEBUG) { System.out.println("accessing the name"); }
}
}

Expected Result (Generate Java Code)

public class Student
{
  public Student()
  {}
  public void delete()
  { if ( DEBUG) { System.out.println("accessing the name"); }
  }
  public String textdata(Integer i){
  }
}

Actual Result

public class Student
{
  public Student()
  {}
  public void delete()
  {}
  public String textdata(Integer i){
  }
  public before !textdata(Integer i){
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}
@a3994288

This comment has been minimized.

Show comment
Hide comment
@a3994288

a3994288 Feb 19, 2017

Contributor

However, this will give a correct result (there is no exclamation mark (!) after before), however, it might just be a coincidence

class Student
{
  String textdata( Integer i ){
  }
  before textdata( Integer i ){ 
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}   
Contributor

a3994288 commented Feb 19, 2017

However, this will give a correct result (there is no exclamation mark (!) after before), however, it might just be a coincidence

class Student
{
  String textdata( Integer i ){
  }
  before textdata( Integer i ){ 
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}   
@vahdat-ab

This comment has been minimized.

Show comment
Hide comment
@vahdat-ab

vahdat-ab Feb 19, 2017

Member

The person who is going to take this issue in the future must first do an extensive exploration on what exists in the current version. I also recommend that person gets a basic knowledge about the AOP (aspect-oriented programming). The AOP features have developed incrementally so the compiler might have several duplication.

Member

vahdat-ab commented Feb 19, 2017

The person who is going to take this issue in the future must first do an extensive exploration on what exists in the current version. I also recommend that person gets a basic knowledge about the AOP (aspect-oriented programming). The AOP features have developed incrementally so the compiler might have several duplication.

@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Dec 8, 2017

Member

It seems that if the custom method has a parameter, and the before statement matches the parameter exactly (same type and name), the injection is done.

If there are no brackets in the before statement the issue arises.

Ideally matching with * should work.

If there are parentheses but parameter specified in the before statement as in the example below even stranger messages are presented.

class Student
{
  public String m(int i){
    
  }
  before m () { 
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}

results in

Warning on line 6 : Method 'm' cannot be found. Injection was ignored.. More information (1012)
Warning on line 6 : Parameter specification does not apply to code injections with the 'generated' keyword. The injection was applied to all generated methods.. More information (1013)
Member

TimLethbridge commented Dec 8, 2017

It seems that if the custom method has a parameter, and the before statement matches the parameter exactly (same type and name), the injection is done.

If there are no brackets in the before statement the issue arises.

Ideally matching with * should work.

If there are parentheses but parameter specified in the before statement as in the example below even stranger messages are presented.

class Student
{
  public String m(int i){
    
  }
  before m () { 
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}

results in

Warning on line 6 : Method 'm' cannot be found. Injection was ignored.. More information (1012)
Warning on line 6 : Parameter specification does not apply to code injections with the 'generated' keyword. The injection was applied to all generated methods.. More information (1013)

@TimLethbridge TimLethbridge added this to the April 2018 for release 1.28 milestone Dec 8, 2017

@hugessen hugessen self-assigned this Feb 10, 2018

@hugessen

This comment has been minimized.

Show comment
Hide comment
@hugessen

hugessen Feb 21, 2018

Contributor

I deleted my previous comment as I realized I had made a mistake. The error is being thrown in checkCodeInjectionValidity (UmpleInternalParser.java: 5138) when it tries to verify that the user-generated textdata() method actually exists on the class. It doesn't see it there, and so it fails.

The problem is that getAllMethodNames(uClass) appears to be programmed to not return any user-generated methods; it only returns auto-generated methods from attributes, associations and state machines.

Further discussion will be needed in order to fix this, as I'm sure there's a good reason for why it behaves the way it does. In the meantime, I will continue to look into this issue and try to find a good solution.

Contributor

hugessen commented Feb 21, 2018

I deleted my previous comment as I realized I had made a mistake. The error is being thrown in checkCodeInjectionValidity (UmpleInternalParser.java: 5138) when it tries to verify that the user-generated textdata() method actually exists on the class. It doesn't see it there, and so it fails.

The problem is that getAllMethodNames(uClass) appears to be programmed to not return any user-generated methods; it only returns auto-generated methods from attributes, associations and state machines.

Further discussion will be needed in order to fix this, as I'm sure there's a good reason for why it behaves the way it does. In the meantime, I will continue to look into this issue and try to find a good solution.

@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Feb 26, 2018

Member

We have agreed that getAllMethods() either a) needs to be updated to include methods provided by the user, or else b) needs to be renamed (to getAllGeneratedMethods(). Case a would be the answer if all users are clearly expecting not just generating methods. In case b, we would need to create a new getAllMethods() that would have both kinds.

Member

TimLethbridge commented Feb 26, 2018

We have agreed that getAllMethods() either a) needs to be updated to include methods provided by the user, or else b) needs to be renamed (to getAllGeneratedMethods(). Case a would be the answer if all users are clearly expecting not just generating methods. In case b, we would need to create a new getAllMethods() that would have both kinds.

@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Mar 7, 2018

Member

I did a 40 minutes of analysis. The code looks pretty bad, hence it s not surprising you are having some problems.

I am basing my analysis on

class X
{
  d;
  String textdata(){

  }
  before textdata {
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}

I started by finding all places where error 1012 is created using grep on all source. THere are three places. Then I instrumented each of these so that in addition to the method in question, the parameter has a string i.e. +"AAA", +"BBB" and so on to allow me to see which line is actually generating the error.

Working in UmpleInternalParser_CodeClass.ump, we get the following pseudocode.

Line 630 checkCodeInjections is being called
Depending on what getOperationSource() returns, one of three possible methods is called
checkCodeInjectionValidityAll() - line 3378
checkCodeInjectionValidityCustom() - line 3349
checkCodeInjectionValidity() - line 3421
In the above code it is the latter that is being called,
// This seems wrong, as this is custom code!!!
// Also all three of these methods are too similar, so should ideally be
// refactored into a single method, but that might be another issue
// the checkCodeInjectionValidityAll() is separately looking at
// getMethods and getAllMethods but isn't being used here.

so going back to line 630 we have to understand
getOperationSource().
operationSource is somehow being set to 'custom', 'all', 'generated' or perhaps blank

I do a grep on 'custom' and realize the source line is
umple_patterns.grammar:35
So actually 'custom', 'all' and 'generated' are keywords!

Aha. So if I change the line to 'before custom textdata' it works!
Same with 'before all textdata'.

The question then is why doesn't leaving it blank work. Aha! I see that it defaults to 'generated' (I see three comparisons that do this). So why? This must have been a design decision.

Is it a valid decision? Try defaulting it to all and see what test cases break.

Also, look back in the history of git commits (use Github on the lines that do this) and see what issue resulted in this decision to default to 'generated'.

Whatever happens ... we need to add the 'custom', 'all' and 'generated' keywords to the examples in the user manual (actually a new page is needed). And we need test cases for these. Maybe there are some test cases for them actually, perhaps you can locate them.

Member

TimLethbridge commented Mar 7, 2018

I did a 40 minutes of analysis. The code looks pretty bad, hence it s not surprising you are having some problems.

I am basing my analysis on

class X
{
  d;
  String textdata(){

  }
  before textdata {
    if ( DEBUG) { System.out.println("accessing the name"); }
  }
}

I started by finding all places where error 1012 is created using grep on all source. THere are three places. Then I instrumented each of these so that in addition to the method in question, the parameter has a string i.e. +"AAA", +"BBB" and so on to allow me to see which line is actually generating the error.

Working in UmpleInternalParser_CodeClass.ump, we get the following pseudocode.

Line 630 checkCodeInjections is being called
Depending on what getOperationSource() returns, one of three possible methods is called
checkCodeInjectionValidityAll() - line 3378
checkCodeInjectionValidityCustom() - line 3349
checkCodeInjectionValidity() - line 3421
In the above code it is the latter that is being called,
// This seems wrong, as this is custom code!!!
// Also all three of these methods are too similar, so should ideally be
// refactored into a single method, but that might be another issue
// the checkCodeInjectionValidityAll() is separately looking at
// getMethods and getAllMethods but isn't being used here.

so going back to line 630 we have to understand
getOperationSource().
operationSource is somehow being set to 'custom', 'all', 'generated' or perhaps blank

I do a grep on 'custom' and realize the source line is
umple_patterns.grammar:35
So actually 'custom', 'all' and 'generated' are keywords!

Aha. So if I change the line to 'before custom textdata' it works!
Same with 'before all textdata'.

The question then is why doesn't leaving it blank work. Aha! I see that it defaults to 'generated' (I see three comparisons that do this). So why? This must have been a design decision.

Is it a valid decision? Try defaulting it to all and see what test cases break.

Also, look back in the history of git commits (use Github on the lines that do this) and see what issue resulted in this decision to default to 'generated'.

Whatever happens ... we need to add the 'custom', 'all' and 'generated' keywords to the examples in the user manual (actually a new page is needed). And we need test cases for these. Maybe there are some test cases for them actually, perhaps you can locate them.

TimLethbridge added a commit that referenced this issue Mar 9, 2018

Merge pull request #1234 from umple/issue_1002
Fix Issue #1002 and Refactor Methods
@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Mar 9, 2018

Member

This was solved by #1234

Member

TimLethbridge commented Mar 9, 2018

This was solved by #1234

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