ModernUO/Projects/UOContent/Commands/Generic/Extensions/Compilers/PropertyExpressions.cs
Kamron Batman c9831b9680
fix: TextDefinition was uneditable in the props gump, and where parsed constants its own way (#2624)
## Pressing `>` on a TextDefinition did nothing useful

`#1217` moved `TextDefinition` into the Server project for serialization support and added `[PropertyObject]` along the way. `PropsGump` checks that attribute **before** its parsable fallback, so from that commit on the button either drilled into a read-only `Number`/`String` page (both get-only since `#1221`, so no `>` buttons — a dead end) or, when the value was null, re-sent the same page and looked inert.

Worth being clear that `#765` — which replaced the hand-maintained type list with a generic `IsParsable` branch — was **not** the regression. At that commit `TextDefinition` was `[Parsable]` only, fell through to the new branch, and the editor worked. Only the later attribute hijacked the routing.

`TextDefinition` is now caught ahead of the `[PropertyObject]` branch and routed to `SetGump`, which has had `TextDefinition`-specific code at line 35 all along.

## `0` and `"0"` could not be told apart

`Commands.Split` strips quotes before any parser runs, so this was never fixable in `TextDefinition.Parse` alone — the information is gone two layers earlier. The markers now travel inside the value:

| Input | Result |
|---|---|
| `1060847` | cliloc (unchanged) |
| `#1060847` | cliloc, explicit — the form `ToString()` already writes |
| `@"1060847"` | the literal string |
| `hello` | string (unchanged) |
| `(-null-)` | null (unchanged) |

`GetValue()` quotes a string that would not survive the trip back. That check is a real round trip through the parser rather than a pattern match, so it cannot drift from it. `Types.TryParse` decodes the same escape for plain strings — which is what `[get` has always written for the literal `"null"` without `[set` ever reading it back.

**Saved data is untouched.** Serialization reads a discriminated flag plus int/string (`IGenericReader.cs:175`) and the JSON converter switches on token type; neither calls `Parse`, so a stored string `"1060847"` stays a string and spawner JSON is unchanged. The new syntax stays in the text and command layer, where it reaches `[set`, `[add`, spawner props, the props gump and Advanced Search.

## `where` still parsed its constants its own way

`#2625` carried the hand-rolled constant parser across to `PropertyExpressions.Parse` unchanged. It looks for a static `Parse` overload on the property type and gives up if there is none — and `Type` and `IEntity` have neither:

```
where Subject Kind  = Static     ->  Unable to convert string "Static" into type 'System.Type'.
where Subject Owner = 0x1        ->  Unable to convert string "0x1" into type 'Server.Mobile'.
```

Both resolve fine for every other command. Routing through `Types.TryParse` picks up type-name lookup, entity resolution by serial, and the `@"..."` literal convention, so a value that works in one place now works everywhere. It also deletes the duplicate parser (−56 lines in `PropertyExpressions`).

Two things are load-bearing and preserved. `where` has always spelled a null constant as a bare `null`, where `[set` uses `(-null-)`; the shared parser reads a bare `null` as the text, so the null case is handled ahead of it and `= null` keeps meaning null. And nullable targets still unwrap first, so `where <int? prop> = 5` is unaffected.

Bare and hex integers, enums, bools, strings, `Map`, and TextDefinition's `#` / `@"..."` markers all resolve exactly as before — 13 of the 15 tests in `WhereConstantParsingTests` pin that and passed before the change as well as after.

## Documentation

The generic command system had no documentation anywhere in the repo: scopes, `where` and its operators, dot notation, `order by` / `distinct` / `limit`, the value and quoting syntax, `[interface` and `[batch` were all learn-by-reading-the-source. `dev-docs/generic-commands.md` covers them, linked from `CLAUDE.md` and `commands-targeting.md`, and points at <https://muo.gg/commands> for the per-command list rather than duplicating it.

Two behaviours it records that were news to me while writing it:

- `Contained` honours conditions on the normal command path but never sets `SupportsConditionals`, and `[batch` is the only place that reads the flag — so the same condition works typed directly and is refused under batch.
- `Multi`, `Single`, `Self` and `Serial` do not parse modifiers at all, so a `where` clause there is passed to the command as ordinary arguments rather than rejected.

Comments in the changed code were trimmed to what is not evident from the code itself; the rationale they carried is either in the doc now or in the commit that introduced it.

## Behavior changes worth a reviewer's attention

Two things go beyond strict bug-fixing, both deliberate:

- **Hex now parses into a TextDefinition.** Consolidating the three `Parse` overloads onto one span codec means `[set Message 0x102CE7` is cliloc 1060847 rather than the string. Previously only the 1-arg `Parse(string)` did hex and nothing called it. This makes the props gump's own `1060847 (0x102CE7)` display typeable.
- **`@"..."` decodes generally for strings**, not only the exact token `@"null"`. So `[set Name @"hello"` sets `hello`. A half-working escape seemed more surprising than a general one, and the `where` path already decoded it — but narrowing it back is a one-line change if preferred.

## Testing

40 tests, each written first and watched fail for the right reason. They cover the props gump routing (populated and null), cliloc/string/quoted parsing and `GetValue` round trips, `where` constant resolution for Type- and entity-valued properties, and the bare-`null` vs `@"null"` distinction that must not drift.

One sort case is added that #2625 left uncovered: ordering a value-typed chain whose intermediate is null, which reads as `default(int)` rather than throwing. Two other tests from the pre-rebase branch were dropped as duplicates of `ConditionalCompilerEdgeTests`.

Rebased onto `535a09899`. `dotnet build` clean, 0 warnings. **902 UOContent** and **869 Server** tests pass, 0 failures.

## Known gaps

- `Nullable<T>` comparisons work via #2625's lifted operators; nothing here changes that.
- Spawner **Params** are split on plain spaces (`BaseSpawner.cs:1035`) rather than through `Commands.Split`, so a constructor argument still cannot contain one. Untouched here — a tokenizer issue, not a TextDefinition one.
- `Types.ParseStringNumericParamTypes` is now unreferenced. Left in place because it is public and shards may use it.
2026-09-10 19:36:30 -07:00

355 lines
13 KiB
C#

using System;
using System.Globalization;
using System.Linq.Expressions;
using System.Reflection;
namespace Server.Commands.Generic;
/// <summary>
/// Expression-tree fragments over a bound <see cref="Property" /> chain, shared by the
/// conditional, sort and distinct compilers. Everything here builds an <see cref="Expression" />;
/// the compilers assemble those into a lambda and hand <c>Compile()</c> the codegen.
/// </summary>
public static class PropertyExpressions
{
private static readonly MethodInfo _objectEquals = typeof(object).GetMethod(
nameof(object.Equals),
BindingFlags.Public | BindingFlags.Static,
[typeof(object), typeof(object)]
)!;
/// <summary>
/// Walks a property binding. A binding of more than one property (<c>Message.Number</c>)
/// dereferences each link in turn, and any link but the last can legitimately be null -- an
/// unset TextDefinition, an unparented item. Each reference-typed intermediate link is stored
/// once and null-checked; a null there yields <paramref name="whenUnreadable" /> in place of
/// whatever <paramref name="onValue" /> would have built from the final link.
/// </summary>
public static Expression Chain(
Expression target,
Property prop,
Func<Expression, Expression> onValue,
Expression whenUnreadable
) => ChainFrom(target, prop.Chain, 0, onValue, whenUnreadable);
private static Expression ChainFrom(
Expression current,
PropertyInfo[] chain,
int index,
Func<Expression, Expression> onValue,
Expression whenUnreadable
)
{
var link = Expression.Property(current, chain[index]);
// The last link is the value being tested, so a null there is the caller's business.
if (index == chain.Length - 1)
{
return onValue(link);
}
if (link.Type.IsValueType)
{
return ChainFrom(link, chain, index + 1, onValue, whenUnreadable);
}
var local = Expression.Variable(link.Type, chain[index].Name);
return Expression.Block(
[local],
Expression.Assign(local, link),
Expression.Condition(
Expression.ReferenceNotEqual(local, Expression.Constant(null, local.Type)),
ChainFrom(local, chain, index + 1, onValue, whenUnreadable),
whenUnreadable
)
);
}
/// <summary>
/// Walks a binding for a caller that has no way to express "no match" -- ordering and
/// grouping, where the value itself is the answer rather than a yes or no. An unreadable
/// link yields <c>default(T)</c>, which is what a null link along the way amounts to; the
/// comparers these feed already handle a null value.
/// </summary>
public static Expression ChainOrDefault(Expression target, Property prop) =>
Chain(target, prop, static value => value, Expression.Default(prop.Type));
/// <summary>
/// Equality for the "not comparable" path, which supports only == and !=. Reference equality
/// would miss a type whose equality is by value -- <see cref="TextDefinition" /> among them --
/// so this is static <c>object.Equals</c>, which honors the override and is null-safe on
/// either side. Value types box; they only reach here when they have no <c>CompareTo</c>.
/// </summary>
public static Expression ValueEquals(Expression a, Expression b) =>
Expression.Call(_objectEquals, Box(a), Box(b));
private static Expression Box(Expression e) =>
e.Type == typeof(object) ? e : Expression.Convert(e, typeof(object));
/// <summary>
/// A boolean test of <paramref name="a" /> against <paramref name="b" />. Integral primitives
/// and enums compare with the operator itself -- on the unsigned types as unsigned; nothing
/// here widens to a signed type. Everything else goes through <see cref="TryCompare" /> and
/// tests the sign of the result. <c>Nullable&lt;T&gt;</c> is lifted either way, as C# lifts it:
/// two nulls are equal, a null and a value are unequal, and a null satisfies no relation.
/// (A null <em>reference</em> keeps the ordering <see cref="TryCompare" /> gives it.) False
/// when the type has no <c>CompareTo</c> at all, in which case only equality is meaningful.
/// </summary>
public static bool TryRelational(Expression a, Expression b, ComparisonOperator op, out Expression test)
{
var type = a.Type;
var underlying = Nullable.GetUnderlyingType(type);
var nonNullable = underlying ?? type;
if (underlying != null && !underlying.IsEnum && !IsIntegral(underlying))
{
return TryLiftedRelational(a, b, op, out test);
}
if (nonNullable.IsEnum)
{
// Equal/NotEqual are defined on enums; the relational operators are not, so those
// read the underlying integer. Convert lifts over Nullable<E> on its own.
if (op is not (ComparisonOperator.Equal or ComparisonOperator.NotEqual))
{
var integer = Enum.GetUnderlyingType(nonNullable);
if (underlying != null)
{
integer = typeof(Nullable<>).MakeGenericType(integer);
}
a = Expression.Convert(a, integer);
b = Expression.Convert(b, integer);
}
test = Relational(a, b, op);
return true;
}
if (IsIntegral(nonNullable))
{
test = Relational(a, b, op);
return true;
}
if (!TryCompare(a, b, 1, out var comparison))
{
test = null;
return false;
}
test = Relational(comparison, Expression.Constant(0), op);
return true;
}
// Nullable<T> over a type that compares through CompareTo: the values compare when both are
// present, and the HasValue flags decide otherwise, the way the language lifts an operator.
private static bool TryLiftedRelational(Expression a, Expression b, ComparisonOperator op, out Expression test)
{
var left = Expression.Variable(a.Type, "left");
var right = Expression.Variable(a.Type, "right");
var couldCompare = TryCompareValues(
Expression.Property(left, "Value"),
Expression.Property(right, "Value"),
1,
out var comparison
);
if (!couldCompare)
{
test = null;
return false;
}
var leftHasValue = Expression.Property(left, "HasValue");
var rightHasValue = Expression.Property(right, "HasValue");
var both = Expression.AndAlso(leftHasValue, rightHasValue);
var relation = Relational(comparison, Expression.Constant(0), op);
Expression lifted = op switch
{
ComparisonOperator.Equal => Expression.Condition(both, relation, Expression.Equal(leftHasValue, rightHasValue)),
ComparisonOperator.NotEqual => Expression.Condition(both, relation, Expression.NotEqual(leftHasValue, rightHasValue)),
_ => Expression.AndAlso(both, relation)
};
test = Expression.Block(
[left, right],
Expression.Assign(left, a),
Expression.Assign(right, b),
lifted
);
return true;
}
private static Expression Relational(Expression a, Expression b, ComparisonOperator op) =>
op switch
{
ComparisonOperator.Equal => Expression.Equal(a, b),
ComparisonOperator.NotEqual => Expression.NotEqual(a, b),
ComparisonOperator.Greater => Expression.GreaterThan(a, b),
ComparisonOperator.GreaterEqual => Expression.GreaterThanOrEqual(a, b),
ComparisonOperator.Lesser => Expression.LessThan(a, b),
ComparisonOperator.LesserEqual => Expression.LessThanOrEqual(a, b),
_ => throw new InvalidOperationException("Invalid comparison operator.")
};
private static bool IsIntegral(Type type) =>
type == typeof(int) || type == typeof(long) || type == typeof(uint) || type == typeof(ulong)
|| type == typeof(short) || type == typeof(ushort) || type == typeof(byte) || type == typeof(sbyte);
/// <summary>
/// An <c>int</c>-valued comparison of <paramref name="a" /> against <paramref name="b" />
/// with <c>CompareTo</c> semantics, multiplied by <paramref name="sign" />. A null on either
/// side of a reference or nullable type is handled here rather than in the callee:
/// <c>null.CompareTo(null) = 0</c>, <c>real.CompareTo(null) = -sign</c>,
/// <c>null.CompareTo(real) = +sign</c>. False when the type has no <c>CompareTo</c>.
/// </summary>
public static bool TryCompare(Expression a, Expression b, int sign, out Expression comparison)
{
var type = a.Type;
// Both sides are read more than once below; pin them so a chained binding is walked once.
var left = Expression.Variable(type, "left");
var right = Expression.Variable(type, "right");
if (!TryCompareValues(left, right, sign, out var body))
{
comparison = null;
return false;
}
comparison = Expression.Block(
[left, right],
Expression.Assign(left, a),
Expression.Assign(right, b),
body
);
return true;
}
private static bool TryCompareValues(Expression a, Expression b, int sign, out Expression comparison)
{
var type = a.Type;
var underlying = Nullable.GetUnderlyingType(type);
if (underlying != null)
{
if (!TryCompareValues(Expression.Property(a, "Value"), Expression.Property(b, "Value"), sign, out var inner))
{
comparison = null;
return false;
}
comparison = NullAware(Expression.Property(a, "HasValue"), Expression.Property(b, "HasValue"), inner, sign);
return true;
}
if (type.IsEnum)
{
var integer = Enum.GetUnderlyingType(type);
return TryCompareValues(Expression.Convert(a, integer), Expression.Convert(b, integer), sign, out comparison);
}
var compareTo = FindCompareTo(type);
if (compareTo == null)
{
comparison = null;
return false;
}
var parameterType = compareTo.GetParameters()[0].ParameterType;
var argument = parameterType == type ? b : Expression.Convert(b, parameterType);
Expression call = Expression.Call(a, compareTo, argument);
if (sign == -1)
{
call = Expression.Negate(call);
}
if (type.IsValueType)
{
comparison = call;
return true;
}
var nil = Expression.Constant(null, type);
comparison = NullAware(Expression.ReferenceNotEqual(a, nil), Expression.ReferenceNotEqual(b, nil), call, sign);
return true;
}
private static Expression NullAware(Expression aHasValue, Expression bHasValue, Expression compare, int sign) =>
Expression.Condition(
aHasValue,
Expression.Condition(bHasValue, compare, Expression.Constant(-sign)),
Expression.Condition(bHasValue, Expression.Constant(sign), Expression.Constant(0))
);
private static MethodInfo FindCompareTo(Type type)
{
var compareTo = type.GetMethod("CompareTo", [type]);
if (compareTo != null)
{
return compareTo;
}
/* There's a scenario where we might be trying to use CompareTo on an interface
* which, while it doesn't explicitly implement CompareTo itself, is said to
* extend IComparable indirectly. The implementation is implicitly passed off
* to implementers, so the interface's own GetMethod("CompareTo") returns null.
*/
var ifaces = type.FindInterfaces(
static (iface, _) => iface.IsGenericType && iface.GetGenericTypeDefinition() == typeof(IComparable<>),
null
);
for (var i = 0; i < ifaces.Length; ++i)
{
if (ifaces[i].GetGenericArguments()[0].IsAssignableFrom(type))
{
return ifaces[i].GetMethod("CompareTo", [type]);
}
}
return typeof(IComparable).IsAssignableFrom(type)
? typeof(IComparable).GetMethod("CompareTo", [typeof(object)])
: null;
}
/// <summary>
/// The right-hand side of a condition as a typed constant, resolved by the same parser behind
/// <c>[set</c> and <c>[add</c>. See <c>dev-docs/generic-commands.md</c>.
/// </summary>
public static ConstantExpression Constant(Type type, object value)
{
if (value is string text)
{
value = Parse(type, text);
}
return Expression.Constant(value, type);
}
private static object Parse(Type type, string text)
{
var underlying = Nullable.GetUnderlyingType(type);
// `where` spells null as a bare `null`, not [set's (-null-), so it precedes the parser.
if (text == "null" && (underlying != null || !type.IsValueType))
{
return null;
}
return Types.ParseOrThrow(underlying ?? type, text);
}
}