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

Not having return types for the methods #969

Closed
vahdat-ab opened this Issue Jan 22, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@vahdat-ab
Member

vahdat-ab commented Jan 22, 2017

The current version of Umple allows having methods without a return type. It also generates a method without a return type. It makes the generated code not being complied. Please consider that just constructors of classes in Java, for example, can have no return types.
The first option is to modify the Umple grammar to force all methods to have a return type. This is correct because we don't allow modelers to define a constructor in Umple models.
The second option is to allow it, but consider the return type as a string. This philosophy was used for attributes.
This issue is connected to the issue #958. The reason is that if we detect those methods that don't have return types the issue 958 won't happen.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Jan 22, 2017

@TimLethbridge any idea?

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 23, 2017

I don't want to at this point change the grammar to force a return type. I think all along the idea is that there would be a default return type generated, that ought to be void in languages that support void.

@AdamBJ

This comment has been minimized.

Contributor

AdamBJ commented Jan 23, 2017

So to fix this issue I should change the code generation code to either add "void" (or something equivalent) for languages that support such a return type, or throw an error for languages that don't support such a feature. And if the user tries to explicitly specify a constructor, regardless of the return type or parameters they specify, I'll throw a warning as discussed in #958. @vahdat-ab I guess this issue and 958 are separate after all.

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Jan 23, 2017

In Java return void. In Php you don't have to do anything (null is returned in such cases). Ruby also doesn't need a return type specificed. In C++ I leave it to @ahmedvc to ensure that user-defined functions with no umple-specified return type are in fact void functions. So all @AdamBJ has to do is deal with the Java case

@Nava2 Nava2 changed the title from Not having return types for the methdos to Not having return types for the methods Jan 24, 2017

@AdamBJ

This comment has been minimized.

Contributor

AdamBJ commented Feb 1, 2017

I've got a fix for this, and when I run JUnit I'm now getting a single failure. The failure is due to a template comparison mismatch between the actual output generated from ClassTemplateTest_Generated.ump and the contents of ClassTemplateTest_Generated.java.txt. The method only has the public, static modifiers in ClassTemplateTest_Generated.java.txt, but after implementing the 969 fix, the actual output also had the void modifier.

Having public static void main() in a function seems to trigger Umple to generate some thread initialization code in main's function body that isn't there when void is omitted. This added code is causing the template test to fail.

The obvious solution is to include the thread code in the expected output file. However, some parts of the generated threading code have parameters that change from run to run. Also, the original test intentionally omitted the threading code in the expected file, so including it might not be relevant.

I see three solutions to this issue.

  1. Change the name of the function from main to foo. So long as the function name isn't main the threading code isn't added in the function body.

  2. Try to include the threading code in the expected output file. I'm not sure how to do this at this point though since it's a moving target.

  3. Leave the name as-is, but delete all references to threading code. This means the expected output would look something like this:

/*PLEASE DO NOT EDIT THIS CODE*/
/*This code was generated using the UMPLE @UMPLE_VERSION@ modeling language!*/

package example;

public class Mentor
{

  //------------------------
  // MEMBER VARIABLES
  //------------------------

  //------------------------
  // CONSTRUCTOR
  //------------------------

  public Mentor()
  {}

  //------------------------
  // INTERFACE
  //------------------------

  public void delete()
  {}

   public static   main(){

Obviously, the best choice depends on what the test was originally intended to validate. However, there aren't any comments to provide clues, and the file hasn't changed much since it was added by @aforward in 2011 as part of a much larger commit.

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Feb 1, 2017

It must be a bug that the threading code is included only when void is absent.

It is likely that the .txt file was put there and has remained unchanged since before the threading concepts were ever added to Umple elsewhere?

Are there no main methods anywhere in the template tests? Do all of them have the 'void', hence avoiding the threading code?

The threading code must be there for some purpose? (State machine do activities come to mind). Find out the cases where it is needed ... determine where it is generated and why by examining the code.

You say "some parts of the generated threading code have parameters that change from run to run." That is also strange. Surely there must be tests that detect the threading code.

In other words, investigate the broader issue here. Perhaps raise a separate issue and solve that first before finishing this issue.

@Nava2

This comment has been minimized.

Member

Nava2 commented Feb 1, 2017

Oh, something I just noticed while reading through this, the Umple threading code should likely only trigger on public static void main(String[]). I believe that was the intention, and while it is bad "practice" to use main() as a function name, it is allowed by all accounts. Thus, there's probably a check for a public static void main but the parameters are not being checked, which is important for discerning a "main execution" function.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 1, 2017

Your solution has a catch. If you set void to unspecified return types, it becomes part of methods and there is no way to detect later if it's defined by users or Umple itself.
In a simple form, if a different language than Java is going to be generated, the generator will consider that the return type defined by users and it is void. It won't be possible for it to detect that it's been added by Umple compiler.
What Tim wanted
In Java return void. In Php you don't have to do anything (null is returned in such cases). Ruby also doesn't need a return type specified. In C++ I leave it to @ahmedvc to ensure that user-defined functions with no umple-specified return type are in fact void functions
can be covered at the code generation level.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 1, 2017

Regarding the example, it is partially correct. There is a bug in the code generator.
If the method's signature is public static void main (String[] ), then the code related to the thread must be generated and its initialization code must be inside the main method.

   public static  void main(){
    Thread.currentThread().setUncaughtExceptionHandler(new UmpleExceptionHandler());
    Thread.setDefaultUncaughtExceptionHandler(new UmpleExceptionHandler());
  }

Otherwise, the thread must not be generated.
What happens with the example,

class Mentor 
{
  public static main()
  {}
}

Is that it just checks the name of the method and then generates the thread code. However, it cannot find the right method to initialize the thread in. It means it generates the following code:

   public static   main(){
    
  }

it's correct not to have that initialization code, but it's wrong not to have the return type, which must be void
You need to fix those parts in the code generator to avoid these cases.

@AdamBJ

This comment has been minimized.

Contributor

AdamBJ commented Feb 1, 2017

@vahdat-ab I'm actually doing the check at the code generation level (and only for Java as Tim suggested). I was considering doing a check at the parsing level for issue 958, but am working on moving that solution to the code generation level too.

I've added this check at line 30 of UmpleToJava/UmpleTLTemplates/class_MethodDeclaration.ump:

// Fix issue 969
		if (!methodName.equals(uClass.getName()))
		{
			// If this is not a constructor, this method should return "void"
			methodType = methodType.equals("") ? "void" : methodType;
		}

When you say

If you set void to unspecified return types, it becomes part of methods and there is no way to detect later if it's defined by users or Umple itself.

You mean it would be a problem if I do the check at the parser level, right?

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 1, 2017

You mean it would be a problem if I do the check at the parser level, right?

Yes, if you do it at the code generation level, you're fine.

@AdamBJ

This comment has been minimized.

Contributor

AdamBJ commented Feb 1, 2017

Ok, thanks everybody for the feedback. Here's what I'm going to do:

  1. Make sure void is being added to generated Java (non-constructor) functions if no return type is specified in the .ump file.

This is already done

  1. Investigate the threading issue

I need to tighten up the main check so that the threading code is only generated if the the user specifies public static void main(String[]). Right now it's being triggered even if void is missing.

I also need to do a more general investigation of why the threading code is generated, and how we've tested it in other cases to figure out if/how it will be tested in this case.

As @TimLethbridge suggested, I'm going to split issue 2 into a separate issue. Once it's been addressed I'll come back and close 969 issue, hopefully I can do that with a single PR.

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