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

Generated property getter methods don't handle the case when the property doesn't apply #2593

Closed
AaronGreenhouse opened this issue Feb 24, 2021 · 3 comments · Fixed by #2736 or #2789
Closed

Comments

@AaronGreenhouse
Copy link
Contributor

Summary

When updating the FlowLatencyAnalysisSwitch to use the new generated property getter methods, I discovered that the generated property set code doesn't nicely handle the case where the property doesn't to apply to an AADL element. The underlying property lookup code throws a PropertyDoesNotApplyToHolderException. This is allowed to propagate to the caller. This would be fine if there were an easy way to test if the property applies, but there isn't one. You really just have to try it and see what happens. This exception should not be propagated, but caught internally to take advantage of the Optional class.

Expected behavior

I expected the generated property getter method to return an empty Optional instance in this case, same as when the property value is undefined.

Actual behavior

The underlying PropertyDoesNotApplyToHolderException propagates to the caller of the method.

** Possible fix **

I fixed this by updating CodeGenUtil.lookupProperty() on my local branch to be

	public static PropertyExpression lookupProperty(Property property, NamedElement lookupContext,
			Optional<Mode> mode) {
		try {
			Optional<PropertyExpression> modalValue = mode.map(m -> {
				PropertyAssociation association = lookupContext.getPropertyValue(property).first();
				if (association == null) {
					PropertyExpression defaultValue = property.getDefaultValue();
					if (defaultValue == null) {
						throw new PropertyNotPresentException(lookupContext, property, "No property value");
					} else {
						return defaultValue;
					}
				} else {
					return association.valueInMode(m);
				}
			});
			return modalValue.orElseGet(() -> lookupContext.getNonModalPropertyValue(property));
		} catch (final PropertyDoesNotApplyToHolderException e) {
			throw new PropertyNotPresentException(lookupContext, property, "Property does not apply");
		}
	}

This seems to do what I want, but I'm not sure if there are other places I need to catch the exception in the code used by the generated method.

@AaronGreenhouse
Copy link
Contributor Author

Discussed this with @lwrage . Probably we don't want to make this change, but we might want to add acceptsXXX() where xxx is a property name methods. Need to try to use these generated classes more.

@lwrage
Copy link
Contributor

lwrage commented Mar 21, 2022

I now think we DO want the empty option if a property doesn't apply. In an analysis it is inconvenient to have to check first if a property applies and then get the value or hard code what is in the applies to clause. This is relevant in bulk operations such as getting all bindings in a model.

@lwrage lwrage reopened this Mar 21, 2022
@jjhugues
Copy link
Contributor

I understand the use case as different from one a typical user would do. But I would recommend having a safe (with exception raised) and unsafe (without exception) variants for this method. The rationale is that it may help spotting inconsistent model traversal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment