Windowscommunitytoolkit: The ! operator for ExpressionBuilder BooleanNodes always throws

[arcadiogarcia]

Describe the bug

Trying to use the BooleanNode ! operator, e.g.:

    var visual = Windows.UI.Xaml.Hosting.ElementCompositionPreview.GetElementVisual(this);
    var a = !(visual.GetReference().Offset.X > 10);
    visual.StartAnimation("Offset.Y", ExpressionFunctions.Conditional(a, 1,2));

always results in the following exception being thrown:

Can't have an operator that doesn't have 2 exactly params

I looked at the implementation and

• ExpressionNodeType.Not is defined as a OperationType.Operator in ExpressionFunctions.cs
• ExpressionNode.ToExpressionStringInternal has code to check that every node of type OperationType.Operator has exactly 2 params
• The implementation of the ! operator only sends 1 param:

        public static BooleanNode operator !(BooleanNode value)
        {
            return ExpressionFunctions.Function<BooleanNode>(ExpressionNodeType.Not, value);
        }

I don't see any place in the whole solution or the samples using this operator, and this is the only operator that takes only 1 param, so I was thinking maybe it got overlooked when the 2-params check was introduced?

Or am I somehow 'holding it wrong'? I don't see any way to create a Not node with two children nor I understand why would we need a second one.

Steps to Reproduce

Write any expression node using the ! operator (BooleanNode not) and pass it to StartAnimation so ExpressionNode.ToExpressionStringInternal gets invoked.

Expected behavior

I would expect the operator to work with only one param.

[msftbot[bot]]

Hello arcadiogarcia, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[arcadiogarcia]

(If this is indeed a bug and we need to add different OperationType values for Operations of different arity so we can do the appropriate check in each case, or just need to add a special case for this operator, I'm happy to send the fix once I get confirmation)

[Kyaa-dost]

@arcadiogarcia This is an interesting scenario. I am going to open the discusssion with the team and if anyone can confirm you can then proceed with the fix 🚀 If not then hopefully we have a possible solution.

[michael-hawker]

Yeah, I'm not too familiar with this part of the library. Looks like @skendrot brought it over originally?

@JustinXinLiu have you used these before? @Sergio0694 does this relate to things you've been doing in your animation stuff as well?

[Sergio0694]

@michael-hawker Do you mean in the animation APIs in the UICompositionAnimations library? If so, then no that's not really related as I'm not using expression animations there - my APIs are just wrappers for either XAML (Storyboard-based) or composition animations. As for the other animation I've added in the toolkit (the one for the ScrollViewer manipulation set), that's using expression animations that are manually built, so I'm not interacting with this expression builder system there either.

@arcadiogarcia Just to double check - that expression in your example can still be achieved by just removing the ! and inverting the condition, right? As in, for your specific scenario this issue can be worked around and it's not breaking, correct? 🤔

[arcadiogarcia]

@arcadiogarcia Just to double check - that expression in your example can still be achieved by just removing the ! and inverting the condition, right? As in, for your specific scenario this issue can be worked around and it's not breaking, correct? 🤔

Yes, in that example inverting the condition is a valid workaround. That was just a simple repro though, in real scenarios like mine where the input is a preexisting boolean node you need to be a bit more creative and do things like:

var booleanNode = ...
var negatedBooleanNode = ExpressionFunctions.Conditional(booleanNode , 1, 0) == 0

Which basically reimplements the ! operator in a less-than-ideal way using the available operations.

[michael-hawker]

@arcadiogarcia to clarify, I believe you're hitting this exception code, correct?

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/5e030db32da16715696d1b18bf54127760b288e3/Microsoft.Toolkit.Uwp.UI.Animations/Expressions/ExpressionNodes/ExpressionNode.cs#L496-L500

Is this just a case where Not was improperly defined as an Operator instead of a Function?

[arcadiogarcia]

@michael-hawker Yes, that is the exception I'm hitting.

At a quick glance it seems that if we declare Not as a Function then ToExpressionStringInternal would do the right thing, but I think that is fortuitous and only happens because both functions and the Not operator use a prefix notation, and the extra parentheses that the "function call" adds do not cause any side-effects.

Maybe adding a new UnaryOperator type that looks like this would be more appropiate?

 case OperationType.UnaryOperator:

                    if (Children.Count != 1)
                    {
                         throw new Exception("Can't have an unary operator that doesn't have exactly one params");
                    }

                     ret = $"( {GetOperationString()} {Children[0].ToExpressionStringInternal()} )";
                    break;