search

osate / osate2

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

Content assist should propose property constants #2073

Closed lwrage closed 4 years ago

lwrage commented 4 years ago

Summary

It would be convenient to have property constants that have the correct type proposed in content assist. Currently property constants are not proposed on Ctrl+Space.

Environment

AaronGreenhouse commented 4 years ago

Should apply in context of property associations, record fields in property associations, default value in property definition. What else?

AaronGreenhouse commented 4 years ago

Created some test property sets to see what currently happens and think about what we want to have happen.

Property set ps1

property set ps1 is
    intProp: aadlinteger applies to (all);
    stringProp: aadlstring applies to (all);
    listProp: list of aadlinteger applies to (all);
    realProp: aadlreal applies to (all);

    myTypeProp: ps1::myType applies to (all);
    myRangeProp: ps1::myRange applies to (all);
    myRecordProp: ps1::myRecord applies to (all);
    myIntProp: ps1::myInt applies to (all);

    myInt: type aadlinteger;
    myType: type aadlinteger units ps1::myUnits;
    myRange: type range of ps1::myType;
    myRecord: type record (intF: aadlinteger; myTypeF: ps1::myType; myIntF: ps1::myInt;);

    myUnits: type units (a, b => a * 2, c => b * 2);
end ps1;

I created two more property sets that contain property constant declarations. Each property set has one constant that should be compatible with a corresponding property declaration in ps1.

Property set ps2

property set ps2 is
    with ps1;

    int1: constant aadlinteger => 1;  -- does this work with myInt?
    string1: constant aadlstring => "string";
    list1: constant list of aadlinteger => (0, 1, 2);
    real1: constant aadlreal => 0.1;

    myInt1: constant ps1::myInt => 3;  -- does this work with aadlinteger?
    myType1: constant ps1::myType => 43 c;
    myRange1: constant ps1::myRange => 13 a .. 42 b;
    myRecord1: constant ps1::myRecord => [intF => 1; myTypeF => 3 a; myIntF => 0;];
end ps2;

Property set ps3

property set ps3 is
    with ps1;

    int2: constant aadlinteger => 10;  -- does this work with myInt?
    string2: constant aadlstring => "string string";
    list2: constant list of aadlinteger => (00, 10, 20);
    real2: constant aadlreal => 0.01;

    myInt2: constant ps1::myInt => 30;  -- does this work with aadlinteger?
    myType2: constant ps1::myType => 430 c;
    myRange2: constant ps1::myRange => 130 a .. 420 b;
    myRecord2: constant ps1::myRecord => [intF => 10; myTypeF => 30 a; myIntF => 100;];
end ps3;

I created a package and another property set to use as test contexts for the content assist menu:

(The exact property values and default values below are not important)

Package pkg

package pkg
public
    with ps1, ps2, ps3;

    system s
        properties
            ps1::intProp => 1;
            ps1::stringProp => "";
            ps1::listProp => ();
            ps1::realProp => 0.2;

            ps1::myIntProp => ps3::myInt2;
            ps1::myTypeProp => ps2::myType1;
            ps1::myRangeProp => ps3::myRange2;
            ps1::myRecordProp => [intF=>1; myTypeF=>ps1::myTypeProp; myIntF=>ps3::myInt2;];
    end s;
end pkg;

Property set ps4

property set ps4 is
    with ps1, ps2, ps3;

    intProp: aadlinteger => 1 applies to (all);
    stringProp: aadlstring => ""  applies to (all);
    listProp: list of aadlinteger => (1, 2) applies to (all);
    realProp: aadlreal => 0.0 applies to (all);

    myIntProp: ps1::myInt => ps3::myInt2 applies to (all);
    myTypeProp: ps1::myType => ps3::myType2 applies to (all);
    myRangeProp: ps1::myRange => ps3::myRange2 applies to (all);
    myRecordProp: ps1::myRecord => ps3::myRecord2 applies to (all);
end ps4;

To be clear, I am current testing the content assist in four contexts:

  1. property association value
  2. property constant association value
  3. default property value in a property declaration
  4. record field value when the record is in a property association or default value
AaronGreenhouse commented 4 years ago

Current behavior of the content assist with property association values:

The behavior is identical In the context of default property value.

Things are slightly different, however, the contexts of property constant association value and record field value:

AaronGreenhouse commented 4 years ago

What we need to do

Question

I don't get any errors in the declarative model. I don't see a lot of guidance on this in the AADL spec. Legality rule (L8) for property associations reads:

In a property association, the type of the evaluated property expression must match the property type of the named property.

Legality rule (L3) for property constants reads:

The property type of the property constant declaration must match the property type of the constant property value.

The legality rule (L2) for property expressions reads:

The type of a property named in a property term must be identical to the type of the property name in the property association.

But identical and matching are never defined.

AaronGreenhouse commented 4 years ago

I've looked into why aadlinteger and ps1::myInt accept each other's constant values. The validator does not do any type inference, which is not surprising because that is never defined in the standard. Instead, when testing if a property constant is acceptable to be associated with a property of a given type, the value of the property constant is recursively tested for membership of the property's type. Furthermore, named user-declared types are just aliases, and are replaced with the actual type before the type checking is performed. So, a property whose type is either aadlinteger or ps1::myInt is tested to see that the value being associated with it satisfies aadlinteger.

This tells me that for properties of type aadlinteger and ps1::myInt we do want to suggest the four property constants:

lwrage commented 4 years ago

I'd like to keep this simple, i.e. it should be enough to just propose property constants that have the same type name as what's written in the property definition. So only constants declared as aadlinteger should be proposed where the property definition has aadlinteger. The proposals don't have to be exhaustive, it's enough if we get the obvious candidates.

AaronGreenhouse commented 4 years ago

@lwrage This isn't that hard to work out. Also it occurred to me that the property definitions above are being suggested because they are property reference values. So those are legal and okay.

lwrage commented 4 years ago

It may be not hard for simple property types, but not for structured types (records, lists) because these can be nested. I don't want to waste any effort on that, in particular as the standard isn't clear what's legal and what isn't.

AaronGreenhouse commented 4 years ago

See Xtext help

AaronGreenhouse commented 4 years ago

Entry points for content assist:

AaronGreenhouse commented 4 years ago

Correction, the magic seems to occur here, in completeLiteralorReferenceTerm_NamedValue and completeConstantValue_NamedValue in PropertiesProposalProvider:

    override completeLiteralorReferenceTerm_NamedValue(EObject model, Assignment assignment, ContentAssistContext context, ICompletionProposalAcceptor acceptor) {
        lookupCrossReference(assignment.terminal as CrossReference, context, acceptor, [ 
            showCrossReference(model)
        ])
    }

    override completeConstantValue_NamedValue(EObject model, Assignment assignment, ContentAssistContext context, ICompletionProposalAcceptor acceptor) {
        lookupCrossReference(assignment.terminal as CrossReference, context, acceptor, [ 
            showCrossReference(model)
        ])
    }

First completeConstantValue_NamedValue is called and then completeLiteralorReferenceTerm_NamedValue.

AaronGreenhouse commented 4 years ago

The proposal framework is based on the grammar rules. So I need to figure out why the base types are being ignored.

AaronGreenhouse commented 4 years ago

One of two things is wrong:

  1. lookupCrossReference() is not proposing constants for the base types
  2. showCrossReference() is filtering out constants for the base types
AaronGreenhouse commented 4 years ago

I think completeConstantValue_NamedValue comes first because NumericRangeTerm comes before LiteralOrReferenceTerm in the list of possible reductions, and NumericRangeTerm term has the rule

NumericRangeTerm returns aadl2::RangeValue: minimum=NumAlt //(RealTerm|IntegerTerm| SignedConstant | ConstantValue)
'..' maximum=NumAlt//(RealTerm|IntegerTerm| SignedConstant | ConstantValue) ( 'delta' delta=NumAlt//(RealTerm|IntegerTerm| SignedConstant | ConstantValue)

)? ;

where NumAlt goes to

NumAlt returns aadl2::PropertyExpression: RealTerm|IntegerTerm| SignedConstant | ConstantValue ;

AaronGreenhouse commented 4 years ago

In the case of proposals for intProp and myIntProp the candidates for both are the same. So the problem is not lookupCrossReference().

AaronGreenhouse commented 4 years ago

Problem is definitely showCrossReference(): It lets three items through for myIntProp and zero for intProp in the following :

        public void lookupCrossReference(IScope scope, EObject model, EReference reference,
                ICompletionProposalAcceptor acceptor, Predicate<IEObjectDescription> filter,
                Function<IEObjectDescription, ICompletionProposal> proposalFactory) {
            Function<IEObjectDescription, ICompletionProposal> wrappedFactory = getWrappedFactory(model, reference, proposalFactory);
            Iterable<IEObjectDescription> candidates = queryScope(scope, model, reference, filter);
            for (IEObjectDescription candidate : candidates) {
                if (!acceptor.canAcceptMoreProposals())
                    return;
                if (filter.apply(candidate)) {
                    acceptor.accept(wrappedFactory.apply(candidate));
                }
            }
        }

It's being called as filter.apply(...) as a closure.

AaronGreenhouse commented 4 years ago

To be clear the method is org.osate.xtext.aadl2.properties.ui.contentassist.PropertiesProposalProvider. showCrossReference()

AaronGreenhouse commented 4 years ago

The method showCrossReference() returns whether objDesc should be shown as a proposal:

    def private showCrossReference(IEObjectDescription objDesc, EObject model){
        val expectedPropertyType = 
            switch model {
                PropertyAssociation: model.property.propertyType
                BasicPropertyAssociation: model.property.propertyType
                Property: model.propertyType
                PropertyConstant: model.propertyType
            }

        switch proposedObj:  EcoreUtil.resolve(objDesc.EObjectOrProxy, model){
            EnumerationLiteral: true
            PropertyConstant: {
                expectedPropertyType == proposedObj.propertyType &&
                (   
                    switch model {
                        PropertyConstant: model != proposedObj
                        default: true
                    }
                )               
            }
            Property: {
                expectedPropertyType == proposedObj.propertyType &&
                (   
                    switch model {
                        PropertyAssociation: model.property != proposedObj
                        Property: model != proposedObj
                        default: true
                    }
                )
            }
            default: {false}
         }
    }

The problem is the two checks expectedPropertyType == proposedObj.propertyType. What I am seeing is that even though expectedPropertyType is an AadlIntegerImpl and proposedObj.propertyType is also an AadlIntegerImpl, they are different objects. Furthermore the NamedElementImpl.equals() method that is invoked by this xtend expression is 

    public boolean equals(Object arg0) {
        /*
         * if (arg0 instanceof DataPortImpl)
         * {
         * System.out.println ("equals on " + arg0);
         * }
         */
        return (this == arg0);
    }

(NamedElementImpl is an ancestor of AadlIntegerImpl).

This means the two AadlIntegerImpl objects are not considered equal.

This seems strange to me, but also this is low-level enough that I think it is dangerous to change the equals method here.

Probably we want to change how showCrossReference() is doing the comparison.

@lwrage Definitely need some guidance here.

AaronGreenhouse commented 4 years ago

Per conversation with @lwrage on Slack, if the expected type is a base type we need to check using the class of the type representation object.

AaronGreenhouse commented 4 years ago

But we need to be careful to distinguish between unnamed uses of the base types and those with names. For example,

property set ps_example is
    intProp: aadlinteger applies to (all);

    myType: type aadlinteger units ps1::myUnits;
    myUnits: type units (a, b => a * 2, c => b * 2);
    myTypeProp: ps1::myType applies to (all);

    myInt: type aadlinteger;
    myIntProp: ps1::myInt applies to (all);
end ps_exampe;

When proposing property associations for intProp, myTypeProp, and myIntProp the expected type of the context in all three cases is an AadlIntegerImpl object. But they are not the same:

So we need to have a crazy hybrid kind of analysis where the look at structure but only if the names are null. And only for the most basic of types. I think we should only try to match based on class when the type is

Anything else we force to use declared type names if they want to have content assist.

SO if we have 

property set ps_example2 is
    prop1: range of aadlinteger 1 .. 2 applies to (all);
    const1: constant range of aadlinteger 2 .. 8 => 4 .. 6;
end ps_example2;

We are not going to try to determine if we can suggest const1 as a value for a property association with property prop1, even though a casual look at the types suggests that the type of const1 is a subset of the type of prop1.

AaronGreenhouse commented 4 years ago

Added arePropertyTypesEqual() to Aadl2Util:

    /**
     * Determines if two property types are equal.  This doesn't try to be very fancy because we
     * want to use type names to compare more complicated types.  Two types are equal if
     * <ul>
     *   <li>They are both {@link ListType} and the {@link ListType#getElementType element types} are equal, or
     *   <li>They are both {@link AadlBoolean}, {@link AadlString}, {@link AadlInteger}, or
     *   {@link AadlReal} and both have {@code null} {@link NamedElement#getName() names} and equal
     *   {@link NumberType#getUnitsType() units types} as determined by <code>==</code>, or
     *   <li>Both types are equal as determined by the <code>==</code> operator.
     * </ul>
     */
    public static boolean arePropertyTypesEqual(final PropertyType type1, final PropertyType type2) {
        boolean equal = false;

        if (type1 instanceof ListType && type2 instanceof ListType) {
            equal = arePropertyTypesEqual(((ListType) type1).getElementType(), ((ListType) type2).getElementType());
        } else if (type1 instanceof AadlBoolean && type2 instanceof AadlBoolean) {
            equal = type1.getName() == null && type2.getName() == null;
        } else if (type1 instanceof AadlString && type2 instanceof AadlString) {
            equal = type1.getName() == null && type2.getName() == null;
        } else if (type1 instanceof AadlInteger && type2 instanceof AadlInteger) {
            final NumberType nt1 = (NumberType) type1;
            final NumberType nt2 = (NumberType) type2;
            equal = nt1.getName() == null && nt2.getName() == null && nt1.getUnitsType() == nt2.getUnitsType();
        } else if (type1 instanceof AadlReal && type2 instanceof AadlReal) {
            final NumberType nt1 = (NumberType) type1;
            final NumberType nt2 = (NumberType) type2;
            equal = nt1.getName() == null && nt2.getName() == null && nt1.getUnitsType() == nt2.getUnitsType();
        }

        return equal ? true : type1 == type2;
    }
AaronGreenhouse commented 4 years ago

Lutz pointed out a missed a context: proposing list element values.

Need to find the right entry point for this context.

AaronGreenhouse commented 4 years ago

This was a little more complicated than I expected because when the model argument in showCrossReference() is a ListValue we have to crawl around the parse tree to find the correct PropertyType. We have to go up the containers until we find a PropertyAssociation, BasicPropertyAssociation, Property, or PropertyConstant. But we have to keep track of how many list values we pass through (including the one we start at). Then we take the type that node and climb down list type element values the same number of times.

    def private showCrossReference(IEObjectDescription objDesc, EObject model){
        val expectedPropertyType = 
            switch model {
                PropertyAssociation: model.property.propertyType
                BasicPropertyAssociation: model.property.propertyType
                Property: model.propertyType
                PropertyConstant: model.propertyType
                ListValue: getListElementType(model)
            }
        . . .
    }
    def private PropertyType getListElementType(ListValue listValue) {
        return getListElementType(listValue, 0)
    }

    def private PropertyType getListElementType(EObject model, int listCount) {
        switch model {
            ListValue: getListElementType(model.eContainer, listCount + 1)
            PropertyAssociation: getNestedElementType(model.property.propertyType as ListType, listCount)
            BasicPropertyAssociation: getNestedElementType(model.property.propertyType as ListType, listCount)
            Property: getNestedElementType(model.propertyType as ListType, listCount)
            PropertyConstant: getNestedElementType(model.propertyType as ListType, listCount)
            default: getListElementType(model.eContainer, listCount)
        }
    }

    def private PropertyType getNestedElementType(ListType listType, int n) {
        if (n == 1) {
            listType.elementType
        } else {
            getNestedElementType(listType.elementType as ListType, n - 1)
        }
    }