Skip to content

Commit

Permalink
Fix negation of null values. (#16101)
Browse files Browse the repository at this point in the history
* Add tests for binding negation operator.

Expected results come from 11.0.x branch.

* Unset and null need to be distinct.

Also remove `ExpressionNode.Reset` as it was doing unneeded stuff and should just be the same as `SetSource(AvaloniaProperty.UnsetValue, null);`.

* Make ExpressionNode.OnSourceChanged accept null.

The previous design assumed that a source of `null` was an invalid input to all types of expression nodes. It turned out that there was a single exception to this rule: the `!` operator can in fact operate on a null value. With this new design we instead have to explicitly check for a null value in every override of `OnSourceChanged ` except in `LogicalNotNode`.

Due to this change, we also have to distinguish between `null` and `(unset)` in `ExpressionNode.SetSource` as well.

Fixes #16071

* Fix comment.
  • Loading branch information
grokys committed Jun 26, 2024
1 parent eb14afe commit 8744c64
Show file tree
Hide file tree
Showing 22 changed files with 214 additions and 49 deletions.
2 changes: 1 addition & 1 deletion src/Avalonia.Base/Data/BindingNotification.cs
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ public BindingNotification(Exception error, BindingErrorType errorType, object?
/// to <paramref name="value"/>. If <paramref name="value"/> is a
/// <see cref="BindingNotification"/> then the value will first be extracted.
/// </remarks>
public static object? UpdateValue(object o, object value)
public static object? UpdateValue(object? o, object value)
{
if (o is BindingNotification n)
{
Expand Down
14 changes: 5 additions & 9 deletions src/Avalonia.Base/Data/Core/BindingExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public override void UpdateTarget()
var source = _nodes[0].Source;

for (var i = 0; i < _nodes.Count; ++i)
_nodes[i].SetSource(null, null);
_nodes[i].SetSource(AvaloniaProperty.UnsetValue, null);

_nodes[0].SetSource(source, null);
}
Expand Down Expand Up @@ -253,10 +253,6 @@ internal void OnNodeValueChanged(int nodeIndex, object? value, Exception? dataVa
_nodes[nodeIndex + 1].SetSource(value, dataValidationError);
WriteTargetValueToSource();
}
else if (value is null)
{
OnNodeError(nodeIndex, "Value is null.");
}
else
{
_nodes[nodeIndex + 1].SetSource(value, dataValidationError);
Expand All @@ -273,11 +269,11 @@ internal void OnNodeValueChanged(int nodeIndex, object? value, Exception? dataVa
/// <param name="error">The error message.</param>
internal void OnNodeError(int nodeIndex, string error)
{
// Set the source of all nodes after the one that errored to null. This needs to be done
// for each node individually because setting the source to null will not result in
// Set the source of all nodes after the one that errored to unset. This needs to be done
// for each node individually because setting the source to unset will not result in
// OnNodeValueChanged or OnNodeError being called.
for (var i = nodeIndex + 1; i < _nodes.Count; ++i)
_nodes[i].SetSource(null, null);
_nodes[i].SetSource(AvaloniaProperty.UnsetValue, null);

if (_mode == BindingMode.OneWayToSource)
return;
Expand Down Expand Up @@ -394,7 +390,7 @@ protected override void StartCore()
protected override void StopCore()
{
foreach (var node in _nodes)
node.Reset();
node.SetSource(AvaloniaProperty.UnsetValue, null);

if (_mode is BindingMode.TwoWay or BindingMode.OneWayToSource &&
TryGetTarget(out var target))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,11 @@ public bool WriteValueToSource(object? value, IReadOnlyList<ExpressionNode> node
return false;
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is Array array)
SetValue(array.GetValue(_indexes));
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ public bool WriteValueToSource(object? value, IReadOnlyList<ExpressionNode> node

protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is AvaloniaObject newObject)
{
WeakEvents.AvaloniaPropertyChanged.Subscribe(newObject, this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@ void IWeakEventSubscriber<PropertyChangedEventArgs>.OnEvent(object? sender, Weak
UpdateValueOrSetError(sender);
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

Subscribe(source);
UpdateValue(source);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ namespace Avalonia.Data.Core.ExpressionNodes;

internal sealed class DataContextNode : DataContextNodeBase
{
protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is IDataContextProvider && source is AvaloniaObject ao)
{
ao.PropertyChanged += OnPropertyChanged;
Expand Down
49 changes: 29 additions & 20 deletions src/Avalonia.Base/Data/Core/ExpressionNodes/ExpressionNode.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Text;

Expand Down Expand Up @@ -60,16 +61,6 @@ public virtual void BuildString(StringBuilder builder, IReadOnlyList<ExpressionN
BuildString(builder);
}

/// <summary>
/// Resets the node to its uninitialized state when the <see cref="Owner"/> is unsubscribed.
/// </summary>
public void Reset()
{
SetSource(null, null);
_source = null;
_value = AvaloniaProperty.UnsetValue;
}

/// <summary>
/// Sets the owner binding.
/// </summary>
Expand Down Expand Up @@ -101,28 +92,26 @@ public void SetOwner(BindingExpression owner, int index)
/// </param>
public void SetSource(object? source, Exception? dataValidationError)
{
var oldSource = Source;

if (source == AvaloniaProperty.UnsetValue)
source = null;
if (_source?.TryGetTarget(out var oldSource) != true)
oldSource = AvaloniaProperty.UnsetValue;

if (source == oldSource)
return;

if (oldSource is not null)
if (oldSource is not null && oldSource != AvaloniaProperty.UnsetValue)
Unsubscribe(oldSource);

_source = new(source);

if (source is null)
if (source == AvaloniaProperty.UnsetValue)
{
// If the source is null then the value is null. We explicitly do not want to call
// If the source is unset then the value is unset. We explicitly do not want to call
// OnSourceChanged as we don't want to raise errors for subsequent nodes in the
// binding change.
_source = null;
_value = AvaloniaProperty.UnsetValue;
}
else
{
_source = new(source);
try { OnSourceChanged(source, dataValidationError); }
catch (Exception e) { SetError(e); }
}
Expand Down Expand Up @@ -242,6 +231,26 @@ protected void SetValue(object? value, Exception? dataValidationError = null)
}
}

/// <summary>
/// Called from <see cref="OnSourceChanged(object?, Exception?)"/> to validate that the source
/// is non-null and raise a node error if it is not.
/// </summary>
/// <param name="source">The expression node source.</param>
/// <returns>
/// True if the source is non-null; otherwise, false.
/// </returns>
protected bool ValidateNonNullSource([NotNullWhen(true)] object? source)
{
if (source is null)
{
Owner?.OnNodeError(Index - 1, "Value is null.");
_value = null;
return false;
}

return true;
}

/// <summary>
/// When implemented in a derived class, subscribes to the new source, and updates the current
/// <see cref="Value"/>.
Expand All @@ -250,7 +259,7 @@ protected void SetValue(object? value, Exception? dataValidationError = null)
/// <param name="dataValidationError">
/// Any data validation error reported by the previous expression node.
/// </param>
protected abstract void OnSourceChanged(object source, Exception? dataValidationError);
protected abstract void OnSourceChanged(object? source, Exception? dataValidationError);

/// <summary>
/// When implemented in a derived class, unsubscribes from the previous source.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@ public override void BuildString(StringBuilder builder)
// We don't have enough information to add anything here.
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

SetValue(_transform(source));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,11 @@ public override bool ShouldLogErrors(object target)
return target is ILogical logical && logical.IsAttachedToLogicalTree;
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is ILogical logical)
{
var locator = ControlLocator.Track(logical, _ancestorLevel, _ancestorType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,12 @@ public bool WriteValueToSource(object? value, IReadOnlyList<ExpressionNode> node
return false;
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
var v = BindingNotification.ExtractValue(source);

if (TryConvert(v, out var value))
{
SetValue(BindingNotification.UpdateValue(source, !value), dataValidationError);
}
else
SetError($"Unable to convert '{source}' to bool.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ public override void BuildString(StringBuilder builder)
builder.Append("()");
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is INotifyPropertyChanged newInpc)
WeakEvents.ThreadSafePropertyChanged.Subscribe(newInpc, this);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ public override bool ShouldLogErrors(object target)
return target is not ILogical logical || logical.IsAttachedToLogicalTree;
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (_nameScope.TryGetTarget(out var scope))
_subscription = NameScopeLocator.Track(scope, _name).Subscribe(SetValue);
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ internal sealed class ParentDataContextNode : DataContextNodeBase
private static readonly AvaloniaObject s_unset = new();
private AvaloniaObject? _parent = s_unset;

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is AvaloniaObject newElement)
newElement.PropertyChanged += OnPropertyChanged;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,11 @@ public bool WriteValueToSource(object? value, IReadOnlyList<ExpressionNode> node
}

[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = "<Pending>")]
protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

var reference = new WeakReference<object?>(source);

if (_plugin.Start(reference, PropertyName) is { } accessor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,11 @@ public bool WriteValueToSource(object? value, IReadOnlyList<ExpressionNode> node
return _accessor?.SetValue(value, BindingPriority.LocalValue) ?? false;
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

var reference = new WeakReference<object?>(source);

if (GetPlugin(source) is { } plugin &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@ override public void BuildString(StringBuilder builder)
builder.Append('^');
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

var reference = new WeakReference<object?>(source);

if (GetPlugin(reference) is { } plugin &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,11 @@ public bool WriteValueToSource(object? value, IReadOnlyList<ExpressionNode> node
return true;
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

_indexes = null;

if (GetIndexer(source.GetType(), out _getter, out _setter))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ public override void BuildString(StringBuilder builder)
builder.Append(')');
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (_targetType.IsInstanceOfType(source))
SetValue(source);
else
Expand Down
5 changes: 4 additions & 1 deletion src/Avalonia.Base/Data/Core/ExpressionNodes/StreamNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@ public override void BuildString(StringBuilder builder)
void IObserver<object?>.OnError(Exception error) { }
void IObserver<object?>.OnNext(object? value) => SetValue(value);

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (_plugin.Start(new(source)) is { } accessor)
{
_subscription = accessor.Subscribe(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@ public override void BuildString(StringBuilder builder)
throw new InvalidOperationException("Cannot find a StyledElement to get a TemplatedParent.");
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is StyledElement newElement)
{
newElement.PropertyChanged += OnPropertyChanged;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,11 @@ public override bool ShouldLogErrors(object target)
return target is Visual visual && visual.IsAttachedToVisualTree;
}

protected override void OnSourceChanged(object source, Exception? dataValidationError)
protected override void OnSourceChanged(object? source, Exception? dataValidationError)
{
if (!ValidateNonNullSource(source))
return;

if (source is Visual visual)
{
var locator = VisualLocator.Track(visual, _ancestorLevel, _ancestorType);
Expand Down
Loading

0 comments on commit 8744c64

Please sign in to comment.