Skip to content
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

Aadl2JavaValidator doesn't check CLASSIFIER_MATCH correctly #2344

Closed
AaronGreenhouse opened this issue May 26, 2020 · 18 comments · Fixed by #2345
Closed

Aadl2JavaValidator doesn't check CLASSIFIER_MATCH correctly #2344

AaronGreenhouse opened this issue May 26, 2020 · 18 comments · Fixed by #2345

Comments

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented May 26, 2020

Aadl2JavaValidator doesn't check Classifier_Match sematnics correctly in the method testClassifierMatchRule(). According to the standard:

If the destination classifier is a component type, then any implementation of the source matches.

But the method also allows the src to be a type and the dest to the an implementation of the type. This is well known to be unsafe.

Also, the method currently generates warnings when one classifier is a type and the other is an implementation and the implementation is not an implementation of that type. This should be an error.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented May 28, 2020

Check that bidirectional connections are checked correctly.

Add unit tests

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented May 28, 2020

Errors vs warnings (per @lwrage):

  • if one end has a classifier and the other has none then it is a warning

that's fine, the model may be incomplete.

  • If the classifiers are completely incompatible then it is an error

yes, should be an error

  • one is a type and the other is an implementation, and the implementation is not an implementation of that type then it is a warning

that should be an error, instead. Please fix.

@lwrage lwrage reopened this Jun 13, 2020
@lwrage
Copy link
Contributor

lwrage commented Jun 13, 2020

@AaronGreenhouse This does the wrong thing for access connections. There it's not the connection direction but provides/requires that determines "source" and "destination" for the validation. see #2362

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 15, 2020

Well, it's a bit more complicated than that. The type can become more general towards the ultimate requires. So when checking a connection instance, one end is a requires and the other end is either a provides or a component instance. So the original rule

If the destination classifier is a component type, then any implementation of the source matches.

should be interpreted in this case as

If the classifier on the requires features is a component type, then any implementation of the provides or component matches.

Easy enough.

Things get trickier when checking the declarative model. We must also consider which way is traveling toward the ultimate requires feature. The classifier type information flows in the following directions:

  • sub -> sub.r (Subcomponent to requires feature of a sibling subcomponent)
  • sub -> p (Subcomponent to a provides feature of the containing component)
  • sub.p1 -> p2 (provides feature of a subcomponent to a provides feature of the containing component)
  • sub.p -> sub.r (provides feature of a subcomponent to a requires features of a sibling subcomponent)
  • r -> sub.r (requires feature of a component to a requires feature of a subcomponent)

These are distinct from the declared directions of the connections, which indicate whether the shared component is written to or read from. So both endpoints of a declarative connection need to be looked at to determine which one should be consider the "source" and which one should be considered the "destination" for the type checking.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 16, 2020

Declarative checks are in and have unit tests.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 16, 2020

Connections in the instance model aren't so straightforward either because we allow instantiation of incomplete models. Thus, we still have to go through some gymnastics to figure out which end of the connection belongs to the provider of shared access, and which end is the requirer.

The best case is when one end of the connection is a ComponentInstance: that end is the provider.

When both ends are FeatureInstances then we have to do the same general tests that we have above for declarative models. Only it's much more inconvenient to figure out if a FeatureInstance is a requires or provides.

@lwrage
Copy link
Contributor

lwrage commented Jun 16, 2020

@AaronGreenhouse The fix for #758 should help for the instance model case

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 16, 2020

It's a little easier than the declarative case.

If one end is a requires and the other a provides, that is good. Either the components that the features belong to are siblings, or cousins; neither contains the other.

If both ends are requires or both ends are provides, then one of the components contains the other.

  • If the features are provides then the feature of the contained component is the provider.

  • If the features are requires then the feature of the containing component is the provider.

@lwrage
Copy link
Contributor

lwrage commented Jun 16, 2020

Connection instances that only go up (or down) the containment hierarchy should only be created if one end feature is at the instance model root component. All other connection instances should contain a segment that connects two sibling components.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 16, 2020

Checked in change to ValidateConnectionsSwitch to check the access connection instances. Looks like it works, but I need to run more extensive tests.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 17, 2020

Don't forget to deal with inverse of feature groups.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 18, 2020

Fixed declarative checking to handle feature groups.

Updated JUnit tests.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 18, 2020

Testing inverse feature groups on the instantiated models. I haven't changed any code yet.

The following tests works correctly:

  • Sub_FG_inverse_to_provides.comp_*
  • Sub_FG_inverse_to_provides2.comp_*
  • Sub_FG_inverse_to_provides3.comp_*

In this case we do not check the direction of the feature in the feature group because we know the source of the connection is a subcomponent.

  • Sub_FG_inverse_to_provides2.sub_*
  • FG_inverse_to_To_sub_requires2.*

These work fine. These use an inverse feature group type that explicitly redeclares the features. These work because the features are going to have the correct direction because the are explicitly redeclared.

The following do not work. The connection instances do not get created: Instead there are errors about the connections not having valid connections.

  • Sub_FG_inverse_to_provides.sub_*
  • Sub_FG_inverse_to_provides3.sub_*
  • FG_inverse_to_To_sub_requires.*
  • FG_inverse_to_To_sub_requires3.*

NB. The unnumbered case uses an inverse feature group without explicit features. The (3) case uses inverse of in the feature declaration.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 18, 2020

Need to fix CreateConnectionsSwitch first and then try again.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 18, 2020

Urgh. Check I added for issue #582 in ConnectionInfo.addSegment() is broken and doesn't consider inverse feature groups.

		/*
		 * Issue 582 -- This does not catch all the bad things that can happen. NOT testing for
		 * subcomponents being connected to requires (goingup) or provides (goingdon).
		 */
		// XXX: the argument below, "this.src", may not be correct, but I'm not really sure what is the correct thing
		final ConnectionInstanceEnd resolvedSrc = resolveFeatureInstance(this.src, srcFi);
		// XXX: the argument below, "this.src", may not be correct, but I'm not really sure what is the correct thing
		final ConnectionInstanceEnd resolvedDst = resolveFeatureInstance(this.src, dstFi);
		if (resolvedSrc instanceof FeatureInstance) {
			final Feature srcF = ((FeatureInstance) resolvedSrc).getFeature();
			if (resolvedDst instanceof FeatureInstance) {
				final Feature dstF = ((FeatureInstance) resolvedDst).getFeature();
				if (srcF instanceof DataAccess && dstF instanceof DataAccess) {
					if (goingUp || goingDown) {
						valid &= ((DataAccess) srcF).getKind() == ((DataAccess) dstF).getKind();
					} else {
						valid &= ((DataAccess) srcF).getKind().getInverseType() == ((DataAccess) dstF).getKind();
					}
				}
			}
		} else {
			// TODO ComponentInstance -- Should check connections between components and access features here
		}

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 18, 2020

Updated addSegment() to just use the instance model and not fall back to inspecting the declarative model:

		/*
		 * Issue 582 -- This does not catch all the bad things that can happen. NOT testing for
		 * subcomponents being connected to requires (goingup) or provides (goingdon).
		 */
		// XXX: the argument below, "this.src", may not be correct, but I'm not really sure what is the correct thing
		final ConnectionInstanceEnd resolvedSrc = resolveFeatureInstance(this.src, srcFi);
		// XXX: the argument below, "this.src", may not be correct, but I'm not really sure what is the correct thing
		final ConnectionInstanceEnd resolvedDst = resolveFeatureInstance(this.src, dstFi);
		if (resolvedSrc instanceof FeatureInstance) {
			if (resolvedDst instanceof FeatureInstance) {
				final FeatureInstance resolvedSrcFI = (FeatureInstance) resolvedSrc;
				final FeatureInstance resolvedDstFI = (FeatureInstance) resolvedDst;
				if (resolvedSrcFI.getCategory() == FeatureCategory.DATA_ACCESS
						&& resolvedDstFI.getCategory() == FeatureCategory.DATA_ACCESS) {
					if (goingUp || goingDown) {
						valid &= resolvedSrcFI.getDirection() == resolvedDstFI.getDirection();
					} else {
						valid &= resolvedSrcFI.getDirection().getInverseDirection() == resolvedDstFI.getDirection();
					}
				}
			}
		} else {
			// TODO ComponentInstance -- Should check connections between components and access features here
		}

This fixes the connection problem (above). The featurea groups are then checked correctly on the instance model.

Updated the JUnit tests for the instance model.

Seems to be working.

@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Jun 18, 2020

All the unit tests pass.

@lwrage
Copy link
Contributor

lwrage commented Jun 23, 2020

Fixed by merging #2370

@lwrage lwrage closed this as completed Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants