Rules DSL: Support arithmetic operations directly on Number Item's State - #5618
Rules DSL: Support arithmetic operations directly on Number Item's State#5618jimtng wants to merge 1 commit into
Conversation
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
|
If the arguments are QuantityType, you need to do QuantityType arithmetic. I think you run into trouble with this if the units don't match. And arithmetic with QuantityType should return a QuantityType unless the arguments are first converted explicitly to Number, in which case it is the responsibility of the script to use the proper unit. |
|
I can't understand that you can't already do this. public static BigDecimal operator_plus(Number x, Number y) {
BigDecimal xValue = numberToBigDecimal(x);
BigDecimal yValue = numberToBigDecimal(y);
if (xValue == null) {
return yValue;
} else if (yValue == null) {
return xValue;
} else {
return xValue.add(yValue);
}
}If both I can only guess at how Xtend "matches" types, but if it matches by "specificity", it might consider
|
|
I've done a bit of testing, and I've found that the reason it doesn't work with the existing |
|
The reason why questions are raised on the forum is lack of examples in the openHAB documentation (and users not understanding the openHAB Java API, including all sub-classes of State). The procedure to enhance the documentation is currenlty very cumbersome: it takes too much time to verify if some proposed text change is correct, even if verifying the changes would be trivial; written text in github, as part of proposed text rewording, must be deleted, before the proposed change make progress. And updates, for which it is clear that on their own they are correct, are not integrated for more than a year or more than half a year. All this leads to a situation, where nobody wants to propose changes to the documentation, because simply said - currently changes to the documentation are proposed faster, that they are handled. As a matter of fact https://www.openhab.org/docs/configuration/rules-dsl.html contains this example: but the example is not correct. In an openHAB 5.2. test: prints So The current change does not address comparing state with unit to a number. All that said, instead of changing how the system currently works, it should be described in the documentation how the system currently works. Otherwise - before and after this change - there is still no documentation how the system works - so nothing changes. This change can cause also further surprizes. For all the above reasons, I am against the current changes. I think the right first step is to update the documentation and explain there how numbers and states are supposed to be used in Xbase/openHAB DSL Rules/Scripts/Transformations/UI-Rule-bodies. |
Did you read further down the page, specifically this section: https://www.openhab.org/docs/configuration/rules-dsl.html#number-item The example is correct if the item is a Number type without dimension (carries a DecimalType and not a QuantityType). In the section I point to, this is explained with appropriate examples. I would say Temperature as an Item name for the first example is an unlucky choice, as it may imply a dimension, but it is not wrong by itself. And it is properly documented on the page. |
Some of this is a problem with openHAB that isn't limited to documentation. I've also found that if you touch somewhat advanced topics, there might not be anybody that wants to approve or reject it because they don't feel that they themselves have the required knowledge. Yes, I know it's frustrating, but it's also somewhat understandable. It can help if you explain the topic so that it's easier for people to understand the problem. But, there's another problem that I've encountered with some of the changes you want to do to the documentation: If you find an inconsistency/a problem, you want to document that - when really, the problem should instead be solved. When something is documented, it kind of becomes "the official way it's supposed to work". This doesn't always fit with the problems you find, in which case the effort should be on fixing the design, not documenting the flaws. |
|
This PR is still a very early draft, but I appreciate all the feedback. |
|
The text at https://www.openhab.org/docs/configuration/rules-dsl.html#number-item
could be understood as:
Irrespective of this, at the same time for a number item of QuantityType (without dimension) |
In fact currently is sufficient to write: var a = MyItem.state as Number - MyOtherItem.stateor (probably what is meant): var b = MyItem.state as QuantityType - MyOtherItem.stateIn the first case var c = MyItem.state as QuantityType<javax.measure.quantity.Temperature> - MyOtherItem.statethere is no warning. |
|
Looking at this again, currently there are two minus operators for two parameters:
When MyItem.state is a Number and a QuantityType at the same time, my guess is, that the expression Likewise for multiplication these operators exist in NumberExtensions:
The expression The discussion here should be, whether this ambiguity should stay, so that users must use casts to be explicit on which operator exactly to call.
What can be tried, and I have no idea if it will work, is when the user explicitly provides the destination type, to call the desired val Number a = MyItem.state - MyOtherItem.state // calls operator_minus(Number, …)
var b = MyItem.state - MyOtherItem.state // calls operator_minus(QuantityType, …)This is how Collection Literals work: val a = #['Hello','World'] // a is LIST, almost same expression as next line
val String[] b = #['Hello','World'] // b is ARRAY, because the destination type is set explicitly. |
I'm not saying that this is wrong, I don't know enough about how Xtext/Xtend works, but from what I read, it should behave in the same manner as standard Java does. For standard Java the rule is clear: Pick the version with the highest "specificity", which means that
I think it could (and should), but it would require refactoring of .../src/main/java/org/openhab/core/library/types/QuantityType.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bundles/org.openhab.core/src/main/java/org/openhab/core/library/types/QuantityType.java b/bundles/org.openhab.core/src/main/java/org/openhab/core/library/types/QuantityType.java
index fc0f7d17ed..c4026beb7e 100644
--- a/bundles/org.openhab.core/src/main/java/org/openhab/core/library/types/QuantityType.java
+++ b/bundles/org.openhab.core/src/main/java/org/openhab/core/library/types/QuantityType.java
@@ -688,7 +688,7 @@ public class QuantityType<T extends Quantity<T>> extends Number
* @param value the value this {@link QuantityType} should be multiplied with.
* @return the product of the given value with this {@link QuantityType}.
*/
- public QuantityType<?> multiply(BigDecimal value) {
+ public QuantityType<T> multiply(BigDecimal value) {
Quantity<T> quantity = Quantities.getQuantity(this.quantity.getValue(), this.quantity.getUnit(),
Scale.ABSOLUTE);
return new QuantityType<>(quantity.multiply(value));Similar changes would need to be made throughout the class. The thing is that, as soon as something returns It's long bothered me that it seems to be impossible to deal with But, there are other problems here too, in public static QuantityType<?> operator_multiply(Number x, QuantityType<?> y) {
BigDecimal xValue = numberToBigDecimal(x);
if (xValue == null) {
return QuantityType.ZERO;
} else if (y == null) {
return QuantityType.ZERO;
} else {
return y.multiply(xValue);
}
}This is just wrong and can never work, it actually returns I think returning zero for But, it's probably way too late to correct the design at this time, I can only imagine how many rules would break if it started returning |
|
I also think that it makes no sense for an extension method to handle the case, when its first parameter is prints so I think the |
Yeah, I keep forgetting that it's the declared type that decides, not the actual type. So it is possible. But, I still don't think it's "correct" to translate |
I do not see how the first sentence could be interpreted as the second. And the second sentence is wrong. If you have a Note,
Unless the erorr is suppressed now thorugh some change, Xbase explicitly reports when there is and error when it encounters and ambiguous method call. I used to have to fight with that all the time in Rules DSL. So if there is no error reported cliaming an ambiguous function call, I doubt that is the case here. However, the cases you point out indeed used to generate that type of error in a DSL rule. So maybe the reporting of that error has indeed been supporessed somehow.
Try it as
In short, if you put the constant first, you don't need to cast the state. If you put the state first, you must cast the state to at least a I'm not sure what this means but it's a data point which might inform the issue. I don't think the problem is with the underlying classes so much as it's that Xtend seems to only try to coerce the second operand's type and not both operands. |
I'm not aware of any recent changes that have led to more suppression of errors - changes have been in the opposite direction, if anything. But I doubt this is an "ambiguous situation", because they differ in specificity. If these situations were deemed ambiguous, there would be ambiguity "everywhere". I think ambiguity occurs when it lacks any "rule" for which one to pick, if they have equal "priority"/specificity.
By looking at the existing methods in It doesn't matter if the Regarding |
I think the first sentence, taken from https://www.openhab.org/docs/configuration/rules-dsl.html, allows this interpretation (which I emphasized by rewriting). . And then the problem is, that this interpretation is wrong. |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
This is an AI-assisted review performed before manual maintainer review.
The goal of making arithmetic with Number Item states less cumbersome makes sense. Item.state is statically typed as State, so Rules DSL currently requires casts even though Number Item states are represented by numeric types at runtime.
However, the current approach introduces some potentially surprising changes to arithmetic semantics and overload resolution that should be addressed before this is merged.
In particular:
- The new
Stateoperators returnNumber, even when the runtime result is aQuantityType. Because Rules DSL/Xbase resolves subsequent operators using the static type, chained expressions can therefore lose quantity semantics. QuantityType + NumberandQuantityType - Numberare now accepted implicitly but fall back to the ordinaryNumberoperators, which convert the quantity to a numeric value and discard its unit. Previously this required an explicit cast toNumber.- The casts added to the existing Java tests show that the new overloads introduce source-level ambiguity for classes such as
DecimalTypethat implement bothStateandNumber.org.openhab.core.model.script.libis an exported package, so this is not necessarily limited to internal test code. - The new tests call
NumberExtensionsdirectly and therefore do not exercise the Rules DSL type resolution that this PR is intended to change.
No direct NumberExtensions.operator_* callers were found in the other main openHAB repositories, so there does not appear to be an immediate Java build break there. The larger cross-repository impact is documentation: openhab-docs, openhab-addons, and openhab-vscode contain examples that currently teach explicit state-to-Number conversions. Those can be updated once the intended arithmetic semantics are settled.
The usability improvement is worth pursuing, but it would be good to make sure direct state arithmetic retains the same unit safety and predictable type behavior as the existing explicit operations.
A human maintainer review is still required after the AI-assisted review.
|
|
||
| // Calculation operators for states | ||
|
|
||
| public static Number operator_plus(State x, State y) { |
There was a problem hiding this comment.
The Number return type can lose the fact that the runtime result is a QuantityType when this expression is used as part of another expression.
For example, with two dimensional Number Items:
(LengthA.state - LengthB.state) * 2the first operation can correctly produce a QuantityType at runtime, but its static result type is Number. Xbase will therefore resolve the following multiplication as a Number operation rather than a QuantityType operation, which converts the quantity to a BigDecimal and loses the unit.
The same issue occurs when such a result is assigned to an inferred variable and used later.
Could this be implemented without reducing a quantity result to Number in the DSL type system? Otherwise simple direct state arithmetic may work while equivalent chained arithmetic silently changes semantics.
| return plus(stateToNumber(x), stateToNumber(y)); | ||
| } | ||
|
|
||
| public static Number operator_plus(State x, Number y) { |
There was a problem hiding this comment.
This makes expressions such as:
Temperature.state + 1valid, but when the state is a QuantityType, plus() only chooses the quantity-specific operator when both operands are QuantityType. A quantity plus a plain number therefore falls through to operator_plus(Number, Number), which converts the quantity to its system-unit numerical value and returns a BigDecimal.
The same problem applies to State - Number.
That seems particularly risky because previously discarding the unit required an explicit as Number conversion. With this change an apparently unit-aware Item expression can discard the unit implicitly.
Could mixed quantity/plain-number addition and subtraction either be rejected or given explicitly defined unit-safe semantics instead?
| @Test | ||
| public void operatorPlusNumberNumber() { | ||
| assertThat(NumberExtensions.operator_plus(DECIMAL1, DECIMAL2), is(BigDecimal.valueOf(3))); | ||
| assertThat(NumberExtensions.operator_plus((Number) DECIMAL1, (Number) DECIMAL2), is(BigDecimal.valueOf(3))); |
There was a problem hiding this comment.
The need to add these casts to previously unambiguous calls looks like an API compatibility regression caused by the new overloads.
DecimalType implements both Number and State, so after adding both:
operator_plus(Number, Number)
operator_plus(State, State)neither overload is more specific for a DecimalType argument and Java requires an explicit cast.
This package is exported by org.openhab.core.model.script, so external Java code directly using these extension methods could also stop compiling after updating openHAB, even though existing binaries would continue to work.
It would be preferable to avoid introducing this ambiguity rather than adapting the existing tests to it. It would also be useful to verify that equivalent Rules DSL expressions with variables statically typed as DecimalType, PercentType, etc. do not become ambiguous for the same reason.
| } | ||
|
|
||
| @Test | ||
| public void operatorMinusStateState() { |
There was a problem hiding this comment.
These tests verify the runtime dispatch helpers, but they bypass the Rules DSL/Xbase operator resolution that this PR is intended to change by explicitly calling NumberExtensions.operator_*.
Could this also be covered by Rules DSL-level tests that parse/type-resolve and execute representative expressions?
In particular, it would be useful to cover:
NumberItem.state - OtherNumberItem.state
NumberItem.state * 1.5
QuantityItem.state - OtherQuantityItem.state
(QuantityItem.state - OtherQuantityItem.state) * 2as well as explicitly typed numeric variables, NULL/UNDEF, and a nonnumeric Item state.
That would catch both overload-resolution regressions and cases where the static type of an intermediate expression differs from its runtime numeric type.
7 participants
Currently to perform numeric calculations in Rules DSL / DSL script, one has to do this:
This PR makes it easier by allowing: