suppress type validation errors

[abelhegedus]

I have a feeling that this will be seen as a joke, but I will submit it nonetheless.

With the current type validator, any error (intentional or otherwise) will block any code generation and will prohibit the use of the corresponding patterns.

My problem:

• there are "Shape" and a "Color" EClass with no common supertypes (apart from the implicit EObject)
• there are "Red Circle" and "Green Rectangle" EClasses which are both subtypes of both previous EClasses
• there is a "Paper" EClass with a normal EReference "colors" that referes to Colors and a derived EReference "coloredShapes" that should be query-backed and refers to Shapes
• the definition of this reference would go something like this:

pattern coloredShapes(This : Paper, Target : Shape){
  Paper.colors(This, Target);
}

• this will result in an "Inconsistent variable type definitions: [Shape, Color]" error
• however, it is a completely good query, that should match each EObject that is an instance of Shape and is in the colors list of Paper.

I can imagine, that even if I explicitely declare, that Target will be a Shape, I could be wrong (i.e. the type validator would be helpful in warning me). However, I would want to be able to tell the tool that I am fully aware that the query might not make sense to it, but it is correct (e.g. Target : !Shape)

It would also help if you could use EObject/Object/Top or something to tell the type validator to bugger off and trrouble someone else :D

Bonus question: Is my metamodeling method flawed and if yes, how would you solve it?

(hint: there may be Shapes that are not Colors and vice versa)
(hint2: obviously, the real case where this came up is different, but the question still applies)

[ujhelyiz]

During the last two meetings this exact issue came forth, and we did not have any real scenario like this. If I remember correctly, you were there when we concluded this.

However, if there is a known subtype, the type checker should not report error, only a warning, if I remember correctly. @okrosa Can you confirm this?

About the bonus question: your metamodeling method is allowed in EMF, but without understanding the correct domain I cannot determine whether there is a cleaner solution for your problem. However, most cases of multiple inheritance can be avoided by using less inheritance and more composition during modeling.

[okrosa]

Yes, we postponed this in the last meeting, as no one came across any real
scenario which needed this feature.
The final decision was to give error always. I see no difference between
the two cases, whether there is a good subtype or not, the currently
defined variable has two types, which at the moment is considered a faulty
behavior. Maybe the error message should list out the possible good
subtypes!

At the end neither java nor xbase are ready for multiple inheritance, so
the solution is not straightforward if we want to support this feature.

2012/9/27 Zoltán Ujhelyi notifications@github.com

During the last two meetings this exact issue came forth, and we did not
have any real scenario like this. If I remember correctly, you were there
when we concluded this.

However, if there is a known subtype, the type checker should not
report error, only a warning, if I remember correctly. @okrosahttps://github.com/okrosaCan you confirm this?

About the bonus question: your metamodeling method is allowed in EMF, but
without understanding the correct domain I cannot determine whether there
is a cleaner solution for your problem. However, most cases of multiple
inheritance can be avoided by using less inheritance and more composition
during modeling.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/309#issuecomment-8947227.

[ujhelyiz]

Ok, we have talked with @abelhegedus about this issue, and we concluded that this feature will not be supported in 0.6.5 because of the short deadline.

On the other hand, as this is required, we should revisit this issue in the short term.

[bergmanngabor]

If I recall that specific meeting correctly, we concluded that the ideal solution would be this:

• the type of Target in that body is (Shape INTERSECT Color)
• this is compatible with both the path constraint and the explicit parameter type

Unfortunately, we can't have this. The problem seems to be that Xtext forces us to compute a single type for each variable. Due to reasons unknown to me (why was this again? @okrosa, @ujhelyiz, I suggest you write it down somewhere so that it is left for posterity), it is not feasible to do the proper type inference in the background, and then merely expose a single type towards Xtext (which is then used for Xbase in case of local variables and matcher API generation in case of parameter variables).

Therefore currently all our validation rules have to use Xtext's type instead of the properly inferred composite type, which would cause a conflict at one of the two constraints (depending which type the variable will eventually be coerced upon). Instead of risking such a nondeterministic conflict, the type checker currently reports the ambiguity upfront.

[ujhelyiz]

@bergmanngabor Not entirely true. There are some validation rules that use the more detailed structure. If you remain on the level of the EMF model, we have an intermediate service that can return the detailed EMF types.

However, for Xtext we have to return a single type, because the Xtext API wants something like this - and Xtext does not guarantee to recall us if he thinks he got the type just now - alltogether for every variable we have to specify a single type, not for every variable reference. The basic problem with this approach is that if we have an Xbase expression, we need to a) have the most specific type information available to be able to refer to the additional constraints, or b) have to ask the user the write manual type casts to this specific types.

[bergmanngabor]

@ujhelyiz well, in that case, I see no reason why we can't easily solve this issue in its current form (in 0.7 of course). We will only have a problem if there is such type ambiguity AND an Xbase expression, in which case we can still guess, and in the worst case it should still be resolvable by an (as) cast.

[ujhelyiz]

Instead of suppressing, we should extend the type checker:

• parameter values should be used as a stronger types in case of ambiguous type information - in this case it should only be a warning; if no parameter type is defined, it is an error
• optionally a new keyword might be introduced for local variables for similar type definitions

[okrosa]

After the commits, Ábel found one more issue. Here is an example:

pattern testBad(parameter : EClassifier) = {
EDataType(parameter);
}

pattern testGood(parameter : EClassifier) = {
EDataType(parameter);
} or {
EClass(parameter);
}

pattern testGood2(parameter) = {
EDataType(parameter);
EClassifier(parameter);
}

The testGood has no error message as the parameter's type is defined as EClassifier, and the type resolution comes out with EClassifier as well (because it selects a common supertype of the EDataType and EClass). However testBad ha an error message now. The definition is the same EClassifier, but the resulotion comes out with EDataType. This is the current error message: "Inconsistent parameter type definition, should be EDataType based
on the pattern definition".

Given the fact the that Good2 pattern has no warning/error at all and not really different from the Bad pattern, I recommend to change this error to a warning.

[bergmanngabor]

I think we could distinguish the cases where the inferred type and the parameter type are really inconsistent from cases where the parameter type is simply not as strict as it could be. The latter is clearly not an error, a warning at most.

[okrosa]

Fixed: changed from error to warning.

[abelhegedus]

Okay, the next corner case to be discussed. TypeOne and TypeTwo are not in any supertype relationship and do not have common supertype, however, there is a TypeOneTwo that is a subtype of both. The following patterns will give an error on Between in the testCombined pattern ("Inconsistent variable type defintions: [TypeOne, TypeTwo], type cannot be selected.").

pattern testCombined(This, Target) {
    find testOne(Between, This);
    find testTwo(Target, Between);
}

pattern testOne(Between : TypeOne, Other){
    //...
}

pattern testTwo(Other, Between : TypeTwo) {
    //...
}

The error is valid, however, it if I have several subtypes that are both TypeOne and TypeTwo, I will not be able to further constraint the type, since I only now that both interfaces are implemented.

Possible ways to suppress to warning (none of them work yet):

• add "TypeOne(Between); TypeTwo(Between);" to testCombined to indicate that the Between is both.
• if there is a subtype that is both (in the packages imported in the EIQ file).
• both must be true (add explicit type constraint and have a valid subtype)

Which one would you be OK with?

[okrosa]

Just some quick remarks:

• First this is still an error, but actually the last error which the type validator can produce, all other cases are warnings now. This also can be stepped down to a warning, if there is no feasible workaround in a special/important use case. In this case adding the TypeOneTwo declaration to Between wouldn't be a good solution? Correct me if I'm wrong. This would only be problematic if there are more TypeOneTwos, and you don't want to select between them.
• Add "TypeOne(Between); TypeTwo(Between);" to testCombined to indicate that the Between is both. >> This would make no difference in the current type inference. A find which gives the type information is just as good as a declaration, only the parameter has a "higher value".
• The type validator currently does not check if there is a common subtype that would make the case correct. We can add this feature, if it voted to be important.

[abelhegedus]

• My use case is exactly the one where I cannot select between TypeOneTwo and TypeTwoOne.
• "in the current type inference" meaning that such a check could be implemented, if we agree that an explicitly declared type constraint is can be stronger than the inferred one.
• my vote has already been cast :)

[abelhegedus]

Quick update: I have managed to get the example to work by using the existing parameter-based override. Since one of my patterns were private anyway and only used in this pattern, I have changed the parameter type in testTwo to TypeOne and now instead of an error in testCombined, I have a warning in testTwo.

pattern testCombined(This, Target) {
    find testOne(Between, This);
    find testTwo(Target, Between);
}

pattern testOne(Between : TypeOne, Other){
    //...
}

pattern testTwo(Other, Between : TypeOne) {
    SomeType.typeTwoFeature(Other,Between);
    //...
}

[ujhelyiz]

Sorry for restarting the conversation about this issue, but I think, the type inference warnings have been weakened too much. E.g. consider the following pattern over the UML domain:

pattern typeError(Cl : Class, Op : Operation) {
    find hasOperation(Op, Cl);
}

The hasOperation pattern is called with parameters in a wrong order. The type checker reports a warning
"Ambiguous variable type defintions: [Operation, Class], the parameter type (Operation) is used now.", but I think this clearly error.

I am not sure whether it is worth adding the extra effort to support both cases (e.g. by checking whether there is a known common subtype or not), but I think I wanted to point out this issue.

[istvanrath]

+1

[okrosa]

Zoli I don't really understand what do you mean by this: "e.g. by checking whether there is a known common subtype or not", in relation to this last issue.

I tried to come up with a correct solution in the last 15 minutes, but all of the seems to lead other problems. If we drop that the parameter is stronger than the other type informations we will be back to where we started. I'll try to come up with something meaningful.

[ujhelyiz]

I wanted to say that we could check whether there is any known common subtype of the referenced Ecore elements - if there is, then it is a multiple inheritance scenarios -> might be intended, so it should be only a warning; if not, then it could be an error instead of a warning.

[okrosa]

Ok, that will be good for this case, but from another viewpoint these two
topics seem completely unrelated. I imagine it would look completely weird
in a year's time, if we look back :).

We might need to tackle this issue from a different perspective. Zoli, why
are we using an extended XbaseTypeProvider at all? It provides the
ctrl+space, the hover functionality, but what else? The two current type
validators use it as well, but I'm thinking about refactoring the code and
rethinking the current validators once more. It might be better if they
would use a custom type inference directly, without the xtext restraints.
//But I'm just thinking out loud in writing, we might not earn much from
changing these parts.

2012/10/31 Zoltán Ujhelyi notifications@github.com

I wanted to say that we could check whether there is any known common
subtype of the referenced Ecore elements - if there is, then it is a
multiple inheritance scenarios -> might be intended, so it should be only a
warning; if not, then it could be an error instead of a warning.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/309#issuecomment-9944174.

[ujhelyiz]

If I am not mistaken, the code generator also uses this type provider (and possibly other Xtext services as well).

Alltogether, I have already stated that I have no trouble with having two type inference interfaces: one for custom reasons and one for Xtext services; however, unless we are absolutely sure we cover all eventualities (including possible future Xtext services :) ), I would argue against rewriting the entire service.

Additionally, in Xtext 2.4 they have introduced major changes in type system handling - possibly they are helping us as well (not sure, I have not checked what is changed exactly). That is another reason I would argue against rewriting it for now.

[ujhelyiz]

Extend validator with the a feature that adds an error if no common subtype is known at compile time. See #309 (comment)

[okrosa]

Ok, I made some minor refactoring as well, some comments are still missing, but I commited in the extended validator for a start (restart) on this ticket. The previous warning now is an error if there is no valid common subtype in the metamodels.

The end result now is that this is just a warning if there is a Red-Circle in the metamodel:

pattern warning(parameter : Red) = {
    Circle(parameter);
}

Parameter in this case is considered Red for the xtext, and is deterministic, because the parameter definition worth more than the other type definitions/inferences.
What might be weird now, that it is still an error if you define it like this:

pattern error(parameter) = {
    Red(parameter)
    Circle(parameter);
}

I was tempted to change this error to a warning in the last 15 minutes, if there is a common subtype. In these case we would select a random type from Red and Circle for the xtext. But we might not need this at all, and it might lead to problems, as the selected type for xtext would be indeterministic. Please vote if it is enough, or should we reconsidered this last case once more.

[okrosa]

(One more, the second case can be made detereministic as well, if we would select between the types based on the alfabetical order. This might sound weird, but this type is for xtext, so it would ensure that the error messages, etc would be deterministic at least.)

[abelhegedus]

If that's not much trouble we could have a specific error message for the last case saying: "Ambiguous type (X, Y)... Please specify the one to be used as the parameter type by adding it to the parameter definition." Which could lead the user to reach the above warning state.

[istvanrath]

A big +1!

On Wednesday, November 21, 2012, Ábel Hegedüs wrote:

If that's not much trouble we could have a specific error message for the
last case saying: "Ambigous type (X, Y)... Please specify the one to be
used as the parameter type by adding it to the parameter definition." Which
could lead the user to reach the above warning state.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/309#issuecomment-10587407.

István

[okrosa]

I added the special error message for the undefined parameter types as discussed.
Note, there is one last case which is still an error with the normal error message:

pattern error(parameter : SomethingElse) = {
    Red(variable)
    Circle(variable);
}

[abelhegedus]

Would it be too much trouble to add this feature to the maintenance branch? This has now come up with an outside user, not only in internal corner cases.

[okrosa]

The lifecycle of this ticket was so long that I'm afraid cherrypicking it will just create new errors. Nevertheless if we do a bugfix release I'll try to refactor the current validator implementation to work in the maintenance branch as well.

[okrosa]

The master validator is refactored into the 0.6.10 branch, this issue should be fixed.