Bug 485136 - Fix remaining interactions.odesign validation errors
Summary: Fix remaining interactions.odesign validation errors
Status: NEW
Alias: None
Product: Sirius
Classification: Modeling
Component: Diagram (show other bugs)
Version: 3.1.0   Edit
Hardware: PC Linux
: P4 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: triaged
Depends on: 486163 485560 486160
Blocks:
  Show dependency tree
 
Reported: 2016-01-04 07:37 EST by Esteban DUGUEPEROUX CLA
Modified: 2018-08-21 09:54 EDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Esteban DUGUEPEROUX CLA 2016-01-04 07:37:28 EST
When validating interaction.odesign I have 270 errors markers, 1 info marker and a warning in error log :

About the error markers, there are a lot of ones about "Feature XXX not found in ...", about "Couldn't find the XXX variable" and "Always true: Nothing inferred when ...".

The info marker is :

The constraint "Invalid Interpreted Expression" is disabled.  It will not be evaluated.

The warning in error log :

The constraint "org.eclipse.sirius.constraints.ValidInterpretedExpressionConstraint" is disabled.  It will not be evaluated.

because of following exception :

java.lang.IllegalStateException: Ambiguous classifier request. At least two classifiers matches Constraint : interactions.Constraint and validation.Constraint
	at org.eclipse.acceleo.query.runtime.impl.EPackageProvider.getType(EPackageProvider.java:483)
	at org.eclipse.sirius.common.acceleo.aql.business.api.TypesUtil.searchEClassifierType(TypesUtil.java:90)
	at org.eclipse.sirius.common.acceleo.aql.business.api.TypesUtil.createAQLVariableTypesFromInterpreterContext(TypesUtil.java:55)
	at org.eclipse.sirius.common.acceleo.aql.business.internal.AQLSiriusInterpreter.analyzeExpression(AQLSiriusInterpreter.java:267)
	at org.eclipse.sirius.common.tools.api.interpreter.CompoundInterpreter.analyzeExpression(CompoundInterpreter.java:1025)
	at org.eclipse.sirius.business.api.dialect.description.MultiLanguagesValidator.validateExpression(MultiLanguagesValidator.java:52)
	at org.eclipse.sirius.tools.internal.validation.description.constraints.ValidInterpretedExpressionConstraint.checkExpression(ValidInterpretedExpressionConstraint.java:81)
	at org.eclipse.sirius.tools.internal.validation.description.constraints.ValidInterpretedExpressionConstraint.validate(ValidInterpretedExpressionConstraint.java:56)
	at org.eclipse.emf.validation.internal.util.JavaConstraintParser$ConstraintAdapter.validate(JavaConstraintParser.java:80)
	at org.eclipse.emf.validation.internal.service.AbstractValidator.evaluateConstraints(AbstractValidator.java:241)
	at org.eclipse.emf.validation.internal.service.BatchValidator.validate(BatchValidator.java:264)
	at org.eclipse.emf.validation.internal.service.BatchValidator.validate(BatchValidator.java:211)
	at org.eclipse.emf.validation.internal.service.BatchValidator.doValidate(BatchValidator.java:149)
	at org.eclipse.emf.validation.internal.service.AbstractValidator.validate(AbstractValidator.java:147)
	at org.eclipse.emf.validation.internal.service.AbstractValidator.validate(AbstractValidator.java:126)
	at org.eclipse.emf.validation.internal.service.BatchValidator.validate(BatchValidator.java:117)
	at org.eclipse.sirius.tools.internal.validation.EValidatorAdapter.validate(EValidatorAdapter.java:80)
	at org.eclipse.emf.ecore.util.Diagnostician.doValidate(Diagnostician.java:171)
	at org.eclipse.emf.edit.ui.action.ValidateAction$3.doValidate(ValidateAction.java:309)
	at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:158)
	at org.eclipse.emf.ecore.util.Diagnostician.validate(Diagnostician.java:137)
	at org.eclipse.emf.edit.ui.action.ValidateAction.validate(ValidateAction.java:264)
	at org.eclipse.emf.edit.ui.action.ValidateAction$1.run(ValidateAction.java:176)
	at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:119)
Comment 1 Cedric Brun CLA 2016-01-04 07:50:52 EST
I would say the .odesign is not referencing the metamodel (or the reference cannot be resolved in your environment)

Can you confirm that ?
Comment 2 Esteban DUGUEPEROUX CLA 2016-01-04 08:24:06 EST
The odesign references http://www.eclipse.org/sirius/sample/interactions#/ and http://www.eclipse.org/emf/2002/Ecore#/ which are correctly deployed in runtime.
Comment 3 Cedric Brun CLA 2016-01-04 08:51:21 EST
Ok, I think I get it.

So we have interactions.Constraint and  validation.Constraint (from Sirius itself)

And so during type analysis an exception is thrown because it needs to be disambiguated in some way.

I would say there are two things to fix here :
1- org.eclipse.sirius.constraints.ValidInterpretedExpressionConstraint should never ever throw an exception or it will be disabled up to the next Eclipse restart, at worst catch it and build a status from it.
2- the hardcoded declaration of the "validation" package could probably be removed under the rational that the name "Constraint" might be fairly common in other Ecores and writting queries within a .odesign which are navigating on some of the validation package types is not something we've seen so far.
(see org.eclipse.sirius.business.internal.dialect.ViewpointMetamodelsProvider.provides(Collection<Viewpoint>))
Comment 4 Cedric Brun CLA 2016-01-05 06:00:46 EST
(In reply to Cedric Brun from comment #3)
> Ok, I think I get it.
> 
> So we have interactions.Constraint and  validation.Constraint (from Sirius
> itself)
> 
> And so during type analysis an exception is thrown because it needs to be
> disambiguated in some way.
> 
> I would say there are two things to fix here :
> 1- org.eclipse.sirius.constraints.ValidInterpretedExpressionConstraint
> should never ever throw an exception or it will be disabled up to the next
> Eclipse restart, at worst catch it and build a status from it.
> 2- the hardcoded declaration of the "validation" package could probably be
> removed under the rational that the name "Constraint" might be fairly common
> in other Ecores and writting queries within a .odesign which are navigating
> on some of the validation package types is not something we've seen so far.
> (see
> org.eclipse.sirius.business.internal.dialect.ViewpointMetamodelsProvider.
> provides(Collection<Viewpoint>))

Besides the exception which disable the validation rule and browsing through the errors the "Always true XXX" is triggered by the expression : 
aql:not self.oclIsKindOf(interactions::MixEnd) or (self.oclIsKindOf(interactions::MixEnd) and self.execution = null) which also present the message "Feature execution not found in EClass MessageEnd)"

MessageEnd is the type of self given by Sirius.

But in this case, couldn't AQL be smarter and figure out that as we are in a "and" branch with a  self.oclIsKindOf(interactions::MixEnd) condition we *know* the type is going to be at least a MixEnd ?
Comment 5 Eclipse Genie CLA 2016-01-08 09:06:34 EST
New Gerrit change created: https://git.eclipse.org/r/63849
Comment 6 Yvan Lussaud CLA 2016-01-08 09:07:46 EST
The problem was in AQL see the patch above.
Comment 7 Eclipse Genie CLA 2016-01-11 10:25:32 EST
New Gerrit change created: https://git.eclipse.org/r/64003
Comment 8 Cedric Brun CLA 2016-01-11 10:37:10 EST
> 1- org.eclipse.sirius.constraints.ValidInterpretedExpressionConstraint
> should never ever throw an exception or it will be disabled up to the next
> Eclipse restart, at worst catch it and build a status from it.

Has a fix proposition through : https://git.eclipse.org/r/#/c/64003/

Note that once the name clash has been fixed, interaction.odesign reveal other classes of problems
a°) Sirius does not use the current context to infer some variable types, I tracked Bug 485560 for this.
b°) It looks like interactions.odesign has many xxx.currentInteraction.yyy calls which should be replaced by xxx.currentInteraction().yyy 
c°) Sirius fail to declare some variables or their type in some of the .odesign constructions.

With the patchsets https://git.eclipse.org/r/#/c/64003/ and https://git.eclipse.org/r/#/c/64004/ we are down to 138 validation errors which, at first sight should lead to a number of bugzilla in Sirius itself to correctly retrieve the type information.
Comment 9 Cedric Brun CLA 2016-01-11 10:40:41 EST
> b°) It looks like interactions.odesign has many xxx.currentInteraction.yyy
> calls which should be replaced by xxx.currentInteraction().yyy 

Sorry, my mistake, this is currentParticipant which should be replaced by currentParticipant() and this is at least partialy done with the proposed patchset migrating the swtbot tests

https://git.eclipse.org/r/#/c/63469/
Comment 12 Eclipse Genie CLA 2016-01-19 12:01:58 EST
New Gerrit change created: https://git.eclipse.org/r/64675
Comment 13 Cedric Brun CLA 2016-01-19 12:16:02 EST
A small breakdown of the remaining errors :
22 "Couldn't find the 'element' variable" which are refering to the precondition of generic tools. 
13 "Couldn't find the 'elementView' variable in preconditions of generic tools
2 - feature eOperation not found in EClass EClass which are looking like a bug in the validation of a SetValue task.

For these two classes of problem I'm not sure who is right and who is wrong. It is not clear to me whether these variables are valued or not at runtime.

The following are looking like SequenceDiagram specific issues :

15 "Couldn't find the 'endBefore' variable"
8 "Couldn't find the 'newOperand' variable' within a tool body.
36 "Feature semanticEnd not found in EClass EObject" which are denoting a lack of type information given by Sirius. In particular these are reference to the variable "startingEndPredecessor" which exists but has no type inference.
Comment 14 Cedric Brun CLA 2016-01-20 04:57:10 EST
(In reply to Cedric Brun from comment #13)
> A small breakdown of the remaining errors :
> 22 "Couldn't find the 'element' variable" which are refering to the
> precondition of generic tools. 
> 13 "Couldn't find the 'elementView' variable in preconditions of generic
> tools

=> this is actually a bug in the validation code. The variables are there @runtime. See Bug 486160
Comment 16 Pierre-Charles David CLA 2016-03-14 05:39:01 EDT
On current master I get: 70 errors, 17 warnings.

For the errors:
- "Couldn't find the computeCombinedFragmentDepth(EClassifier=EObject) service" and "Couldn't find the computeExecutionDepth(EClassifier=EObject) service" (one each), on interpolated colors, defined in palettes outside any Viewpoint (where service classes are registered). Validation code does not know that in the context these colors will be used, the services they require will be present. I guess it should be possible to know which elements in the VSM references the colors, and deduce from that which services should be considered available for validation purpose.
- 1 instance of "Couldn't find the elementview variable" on a generic tool, which looks like a typo (should be elementView). Patch incoming.
- 15 instances of "Couldn't find the endBefore variable" which looks like it is not properly declared in the Sirius code.
- 2 instances of "Couldn't find the instance variable" for sub-operations of "Create Instance" task. Both are bugs in the VSM, as the "Create Instance" explicitly defines an alternate variable name for the newly created instance. Patch incoming.
- 5 instances of "Couldn't find the newOperand variable". Two are bugs similar to the one above (alternate name set in the parent CreateInstance). The 3 others are referenced inside the parameters of an ExternalJavaAction invocation, which appears as a sibling of the CreateInstance. It should probably be *inside* the CreateInstance, but it's still a validation bug because we do not "unset" the variables created by a CreateInstance when executing its successor (non-contained) operations, so it is valid to assume the variable will be present at runtime.
- 2 instances of "Feature eOperation not found in EClass EClass", which look like typos (should be eOperations). Patch incoming.
- 36 instances of "Feature semanticEnd not found in EClass EObject". Cause by the lack of precise type declaration for several sequence-specific variables (e.g. finishingEndPredecessor)
- 5 instances of "The current context does not contains variable named: newOperand". A mix of customized variable name in "CreateInstance" task which were not reflected in subsequent expressions (patch incoming), and expressions which come *after* a CreateInstance (and thus correctly assume the variable will be present) but *outside* the CreateInstance (where the validation code seems to assume it will be out of view).
- 1 instance of "The current element interactions::Interaction does not have the feature named: ends". The VSM seems correct, and the validation code seems to known the correct type. Further investigation in FeatureInterpreter.analyzeExpression() is needed.
- 1 instance of "The required feature 'instanceRolesOrdering' of 'Sequence Diagram on Interaction' must be set". Indeed, it is marked as a required attribute in the metamodel, and our sample modeler does not set a value...

For the warnings:
- 4 instances of "Always true:
Nothing inferred when self (EClassifier=MixEnd) is not kind of EClassifierLiteral=MixEnd" and 1 instance of "Always true:
Nothing inferred when self (EClassifier=Participant) is not kind of EClassifierLiteral=Participant" Not sure exactly how to interpret that. AQL seems to want to tell us that a test in an "if" is unneeded, but the wording of the message is not very clear.
- 12 instances of "Feature XXX not found in EClass YYY". There are several different concrete cases, but for all of them the completion code actually sees the "XXX" feature and proposes it, so completion and validation do not agree.
Comment 17 Pierre-Charles David CLA 2016-03-14 05:52:21 EDT
(In reply to Pierre-Charles David from comment #16)
> - 2 instances of "Couldn't find the instance variable" for sub-operations of
> "Create Instance" task. Both are bugs in the VSM, as the "Create Instance"
> explicitly defines an alternate variable name for the newly created
> instance. Patch incoming.
> - 5 instances of "Couldn't find the newOperand variable". Two are bugs
> similar to the one above (alternate name set in the parent CreateInstance).
> The 3 others are referenced inside the parameters of an ExternalJavaAction
> invocation, which appears as a sibling of the CreateInstance. It should
> probably be *inside* the CreateInstance, but it's still a validation bug
> because we do not "unset" the variables created by a CreateInstance when
> executing its successor (non-contained) operations, so it is valid to assume
> the variable will be present at runtime.
> - 5 instances of "The current context does not contains variable named:
> newOperand". A mix of customized variable name in "CreateInstance" task
> which were not reflected in subsequent expressions (patch incoming), and
> expressions which come *after* a CreateInstance (and thus correctly assume
> the variable will be present) but *outside* the CreateInstance (where the
> validation code seems to assume it will be out of view).

Actually it looks like most (all?) of these are cases of "expressions which come *after* a CreateInstance (and thus correctly assume the variable will be present) but *outside* the CreateInstance (where the validation code seems to assume it will be out of view)". What I took for mistakes in reflecting "Variable Name" changes in the directly enclosing "Create Instance" was voluntary references to variables defined by earlier "Create Instances" which are not in our lexical scope.
Comment 18 Pierre-Charles David CLA 2016-03-14 06:27:22 EDT
(In reply to Pierre-Charles David from comment #16)
> On current master I get: 70 errors, 17 warnings.
> 
> For the errors:
> - "Couldn't find the computeCombinedFragmentDepth(EClassifier=EObject)
> service" and "Couldn't find the computeExecutionDepth(EClassifier=EObject)
> service" (one each), on interpolated colors, defined in palettes outside any
> Viewpoint (where service classes are registered). Validation code does not
> know that in the context these colors will be used, the services they
> require will be present. I guess it should be possible to know which
> elements in the VSM references the colors, and deduce from that which
> services should be considered available for validation purpose.

This one is already tracked as bug #486163.