conflict between auto-generated constructor and hard-coded constructor #958

Open
vahdat-ab opened this Issue Jan 17, 2017 · 19 comments

Projects

None yet

4 participants

@vahdat-ab
Member

Please consider the following example:

class X{
 
  X(){
  }  
}

Now, in the generated code we have two constructors

public class X
{

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

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

  public X()
  {}

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

  public void delete()
  {}
  // line 5 "model.ump"
  public  X(){
    
  }
}

Umple must detect this case and shows the proper reaction.

@vahdat-ab
Member

Thanks to @amidzak for pointing this out

@vahdat-ab
Member

Please pay attention to the concept we have in Umple related adding functionality to the constructor.
We use the following syntax.

after constructor() {
//the code
}

You need to inform developers to use the above structure.

@AdamBJ AdamBJ self-assigned this Jan 20, 2017
@AdamBJ
Contributor
AdamBJ commented Jan 21, 2017

@TimLethbridge I've discussed this issue with @vahdat-ab, and believe that if we encounter this issue we should issue an error (rather than a warning), since a warning might just be ignored. Do you have any thoughts?

@TimLethbridge
Member

Option 1: Ignore the added constructor that exactly matches the generated one (don't output it), and issue a warning (because the resulting Java code will then compile).

Option 2: issue an error causing the compiler to bail out.

Option 1: seems more reasonable. Warnings can be ignored, sure, but it is the developer's responsibility to deal with them nonetheless.

@Nava2
Member
Nava2 commented Jan 21, 2017 edited

I agree with issuing a warning, perhaps adding a suggested fix of adding the AOP-style change of adding the after constructor(). I know I ran into this when writing umple models.

@AdamBJ
Contributor
AdamBJ commented Jan 22, 2017

My thinking is that if we're going to get rid of the explicit constructor anyways, we may as well make sure the user is aware that it's being removed (by forcing them to remove it themselves) and why it's being removed (by directing them to a manual page that explains the potential dangers of explicitly declaring a constructor and how using an explicit constructor goes against the Umple "philosophy").

Here's a first-pass at a manual page (I put it together before I saw the comments from you guys):

image

If you guys still think a warning would be better I'll get rid of the explicit constructor and make it a warning. Please let me know what you think.

@TimLethbridge
Member

I think you will find a few explicit constructors in the compiler. This warning may therefore need to be suppressed when compiling the compiler (there is a 'strictness' directive for this); alternatively it may be able to fix by using 'after constructor' in those cases.

@vahdat-ab
Member

to fix by using 'after constructor' in those cases.

Yes, we need to be consistent with the rules related to the constructor.

@AdamBJ
Contributor
AdamBJ commented Jan 28, 2017 edited

@vahdat-ab My plan to fix this issue is to check that a method we're about to add to the model does not have the same as the class that contains it. If it does, I block the addition and issue a warning. I considered trying to do this in the parser, but it seems like the parser is more concerned with detecting syntactic errors rather than semantic errors like this one.

@TimLethbridge You're right, there are a number of uses of explicit constructors in the Umple compiler. I'm going to suppress these using the strictness directive you mentioned.

@AdamBJ
Contributor
AdamBJ commented Jan 30, 2017 edited

@TimLethbridge @vahdat-ab I've fixed this issue for Java. However, there's a slight wrinkle for Php. Php 5.3.0-5.3.2 has Java-style constructors (i.e. they're a function with the name of the class with no return type). However, in Php 5.3.3+ constructors always have the name __construct(). The constructors Umple currently outputs conform to the latter standard.

When I'm checking for explicitly specified constructors, should I only remove constructors of the form __construct()? Or should I remove old-style constructors too? I think it'd be best to just remove __construct() constructors, as in the new Php standards the old-style constructors are treated as regular functions.

There are two problems with this. First, I can't see a way to check the target language in UmpleInternalParser where I do the constructor check (I prevent the method from being added to the model if the check succeeded). Second, what if the user requests Java and Php to be outputted and does something like explicitly including a function named __constructor? We'd need to remove constructor from the Php output, but not the Java output. However, since currently Java and Php share a model, I can't see an easy way to do this. It might be easier once Katherine has finished her project. What do you suggest I do for now? Maybe there's an existing "language conflict" error I could throw?

@TimLethbridge
Member

@AdamBJ - you should target for the most recent PhP.

Also, all parsing and analysis should be independent of what is going to be generated.

@vahdat-ab
Member

There are two problems with this. First, I can't see a way to check the target language in UmpleInternalParser where I do the constructor check (I prevent the method from being added to the model if the check succeeded). Second, what if the user requests Java and Php to be outputted and does something like explicitly including a function named __constructor?

Interesting point. I think we need to adopt one way to recognize constructors. So far, we followed what has been used for Java and Cpp. We can adopt it. For PHP issue, we can deal with that at the code generation level. It means if there is a method __constructor, we get it at modeling level, but we do check method names when we generate code. This checking at the code generation level is going to happen for languages that have different way to define constructors than Java and Cpp.
Another way is to make target assumption at the modeling level, which Tim said doesn't make sense. I agree with that.
@TimLethbridge any idea about this?

@AdamBJ
Contributor
AdamBJ commented Jan 30, 2017

Dealing with everything (I.e. Java and C++) except for PhP at the model level and PhP at the code generation level would be easiest. The only problem is that we'd be striping Java-style constructors from generated PhP code if we did it that way, which we shouldn't really do.

The alternative would be to duplicate the check at the code generation level for all three languages, with a slightly different check for PhP.

@vahdat-ab
Member

As I said, we need to accept some rules to Umple. I those methods are going to be deleted users will be noticed about this. In that case, they can change it. I think it's not a big deal or PHP users to have it. Not having this causes many issues to us.

@AdamBJ
Contributor
AdamBJ commented Jan 31, 2017

After discussing this issue with @vahdat-ab, there are two options:

  1. Keep the check at the model level, and add a check for __constructor at the PhP generation level. This is the easiest way to go since it means we only have to do one check at code generation. However, it will also incorrectly remove Java-style constructors for generated PhP code.

  2. Push all checks to the code generation level. This way is not as clean since we'd need to repeat the check for each of the languages Umple generates (at least we'd need a check for Java, C++ and PhP). However, it would solve the Java-style constructor in PhP issue mentioned above.

I'm going to look into option 2.

@vahdat-ab
Member

@TimLethbridge there is a need for more thought

@AdamBJ
Contributor
AdamBJ commented Feb 5, 2017

@TimLethbridge @vahdat-ab

If the language target is Java, we should remove any methods the user includes that share a name with the class that contains the method. If it's PhP, we shouldn't do this because a function named myClass that's contained in class myClass is legal.

The solution to this problem is to do the check at code generation time. However, we don't have access to any of the strictness information at code generation time (e.g. which error/warning messages to allow). So we have no way to know if we should actually generate the warning or not (i.e. whether or not the message is being ignored).

I think the best solution to address these issues is to make the strictness stuff a member attribute of the model rather than the UmpleInternalParser. That way we'll have access to the error messages at code generation time (which could be useful in the future if we encounter more target-language dependent messages). What do you guys think?

@TimLethbridge
Member

As I understand it, we don't issue any warnings at code generation time. This issue needs more analysis.

@AdamBJ
Contributor
AdamBJ commented Feb 15, 2017

I'm going to un-assign myself from this issue in order to focus on my term project. Maybe someone from the next round of UCOSP can pick up where this discussion left off.

@AdamBJ AdamBJ removed their assignment Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment