osate / osate2

Open Source AADL2 Tool Environment
http://osate.org
Eclipse Public License 2.0
36 stars 8 forks source link

Error on data access in the middle of end to end flow #1124

Closed joeseibel closed 5 years ago

joeseibel commented 6 years ago

When an end to end flow refers to a data access, an error is reported complaining that data access references are only permitted at the beginning or end of an end to end flow. This validation doesn't make sense since it can't be found in the standard. The following model should be legal:

package connect_to_subcomponent
public
  system s1
  end s1;

  system implementation s1.i1
    subcomponents
      sub1: data;
      sub2: system s2.i1;
    connections
      conn1: data access sub2.f1 <-> sub1;
  end s1.i1;

  system s2
    features
      f1: requires data access;
  end s2;

  system implementation s2.i1
    subcomponents
      sub3: abstract a1;
      sub4: abstract a2;
    connections
      conn2: data access sub3.f2 -> f1;
      conn3: data access f1 -> sub4.f3;
    flows
      --Error: Illegal reference to 'f1'.  Cannot refer to a data access except for the first and last segment of an end-to-end flow.
      etef1: end to end flow sub3.fsource1 -> conn2 -> f1 -> conn3 -> sub4.fsink1;
  end s2.i1;

  abstract a1
    features
      f2: requires data access;
    flows
      fsource1: flow source f2;
  end a1;

  abstract a2
    features
      f3: requires data access;
    flows
      fsink1: flow sink f3;
  end a2;
end connect_to_subcomponent;
AaronGreenhouse commented 5 years ago

This seems like a weird model. Why are you communicating between two sibling components via the parent's feature? How did this model come about?

AaronGreenhouse commented 5 years ago

I have to agree that this situation seems is allowed by the text of the AADL spec (see below). The question is whether it is intended to be allowed.

At first I was thinking that this scenario is nonsense, because the semantics of access connections are a bit confusing, and because I keep thinking that requires access means "output" when it really means an expectation that a data object is to be provided.

If I understand things correctly, what we have is an end to end flow that describes the fact that

  1. subcomponent sub3 is pushing data into the data component that is provided to feature f1, and
  2. that data is then pulled out of that provided data component by subcomponent sub4.

This scenario would be clearer (and look less like nonsense) if a data subcomponent of s2.i1 were used instead of a data access feature. But it would still mean the same basic thing, tracking the flow of data through a "data store". The AADL standard clearly allows this based on Section 10.3 rule (N3):

The subcomponent flow identifier of an end-to-end flow declaration must name an optional flow specification in the component type of the named subcomponent or to a data component in the form of a data subcomponent, provides data access, or requires data access.

Furthermore no restrictions on the location of the data access within in the end to end flow are given.

What is strange is that the place in the verifier that is producing the error messages (Aadl2JavaValidator.typeCheckEndToEndFlowSegments()) claims to be enforcing rule (N3) above.

So in summary, so far I am convinced the checker is being too strict and the above example makes semantic sense. But I want to hear from Lutz or Peter if there is a reason this check was strengthened.

AaronGreenhouse commented 5 years ago

(Sent e-mail to Peter and Lutz and Joe about this.)

lwrage commented 5 years ago

The validator is really too strict, it is a bug.

BTW, don't expect to be able to instantiate flows that pass through data access connections, that's #643 (and a couple of related ones).

AaronGreenhouse commented 5 years ago

Aadl2JavaValidator.typeCheckEndToEndFlowSegments() was

    private void typeCheckEndToEndFlowSegments(EndToEndFlow flow) {
        for (int i = 0; i < flow.getOwnedEndToEndFlowSegments().size(); i++) {
            EndToEndFlowSegment segment = flow.getOwnedEndToEndFlowSegments().get(i);
            if ((segment.getContext() == null || !segment.getContext().eIsProxy()) && segment.getFlowElement() != null
                    && !segment.getFlowElement().eIsProxy()) {
                if (i % 2 == 0) {
                    // Checking ETESubcomponentFlow
                    if (segment.getContext() == null) {
                        if (segment.getFlowElement() instanceof Connection) {
                            error(segment, "Illegal reference to connection '" + segment.getFlowElement().getName()
                                    + "'.  Expecting subcomponent flow or end-to-end flow reference.");
                        } else if (segment.getFlowElement() instanceof FlowSpecification) {
                            error(segment, "Illegal reference to '" + segment.getFlowElement().getName()
                                    + "'.  Cannot refer to a flow specification in the local classifier's namespace.");
                        } else if (segment.getFlowElement() instanceof DataAccess && i > 0
                                && i < flow.getOwnedEndToEndFlowSegments().size() - 1) {
                            error(segment, "Illegal reference to '" + segment.getFlowElement().getName()
                                    + "'.  Cannot refer to a data access except for the first and last segment of an end-to-end flow.");
                        }
                    } else if (segment.getContext() instanceof Subcomponent) {
                        if (!(segment.getFlowElement() instanceof FlowSpecification)) {
                            error(StringExtensions.toFirstUpper(
                                    getEClassDisplayNameWithIndefiniteArticle(segment.getFlowElement().eClass()))
                                    + " in " + getEClassDisplayNameWithIndefiniteArticle(segment.getContext().eClass())
                                    + " is not a valid subcomponent flow.", segment,
                                    Aadl2Package.eINSTANCE.getEndToEndFlowSegment_FlowElement());
                        }
                    } else {
                        error("Anything in " + getEClassDisplayNameWithIndefiniteArticle(segment.getContext().eClass())
                                + " is not a valid subcomponent flow.", segment,
                                Aadl2Package.eINSTANCE.getEndToEndFlowSegment_Context());
                    }
                } else {
                    // Checking ETEConnectionFlow
                    // Because of the parser rule ETEConnectionFlow, we know
                    // that the segment.getContext() is null.
                    if (!(segment.getFlowElement() instanceof Connection)) {
                        error(segment, "Expected Connection, found "
                                + getEClassDisplayName(segment.getFlowElement().eClass()) + '.');
                    }
                }
            }
        }
    }

I removed the unnecessary check regarding DataAccess elements:

    private void typeCheckEndToEndFlowSegments(EndToEndFlow flow) {
        for (int i = 0; i < flow.getOwnedEndToEndFlowSegments().size(); i++) {
            EndToEndFlowSegment segment = flow.getOwnedEndToEndFlowSegments().get(i);
            if ((segment.getContext() == null || !segment.getContext().eIsProxy()) && segment.getFlowElement() != null
                    && !segment.getFlowElement().eIsProxy()) {
                if (i % 2 == 0) {
                    // Checking ETESubcomponentFlow
                    if (segment.getContext() == null) {
                        if (segment.getFlowElement() instanceof Connection) {
                            error(segment, "Illegal reference to connection '" + segment.getFlowElement().getName()
                                    + "'.  Expecting subcomponent flow or end-to-end flow reference.");
                        } else if (segment.getFlowElement() instanceof FlowSpecification) {
                            error(segment, "Illegal reference to '" + segment.getFlowElement().getName()
                                    + "'.  Cannot refer to a flow specification in the local classifier's namespace.");
                        }
                    } else if (segment.getContext() instanceof Subcomponent) {
                        if (!(segment.getFlowElement() instanceof FlowSpecification)) {
                            error(StringExtensions.toFirstUpper(
                                    getEClassDisplayNameWithIndefiniteArticle(segment.getFlowElement().eClass()))
                                    + " in " + getEClassDisplayNameWithIndefiniteArticle(segment.getContext().eClass())
                                    + " is not a valid subcomponent flow.", segment,
                                    Aadl2Package.eINSTANCE.getEndToEndFlowSegment_FlowElement());
                        }
                    } else {
                        error("Anything in " + getEClassDisplayNameWithIndefiniteArticle(segment.getContext().eClass())
                                + " is not a valid subcomponent flow.", segment,
                                Aadl2Package.eINSTANCE.getEndToEndFlowSegment_Context());
                    }
                } else {
                    // Checking ETEConnectionFlow
                    // Because of the parser rule ETEConnectionFlow, we know
                    // that the segment.getContext() is null.
                    if (!(segment.getFlowElement() instanceof Connection)) {
                        error(segment, "Expected Connection, found "
                                + getEClassDisplayName(segment.getFlowElement().eClass()) + '.');
                    }
                }
            }
        }
    }
lwrage commented 5 years ago

Closed by PR #1950