Any failures will be caught. They're not logged, but they also weren't
before. Error handling can be improved at a future date, this series of
changes is primarily intending to unblock a new inspection.
A null there indicates a deserialisation error and therefore due to the
catch block immediately succeeding the changed line everything will
continue to work as intended.
They're mostly used in extensibility scenarios, so everything happens in
runtime. There is no better resolution than to crash with a null
reference exception.
There are seemingly no C#-side compile-time guarantees that it is safe,
but if the task's state is `Faulted` (as is checked right before), the
exception cannot be null as per the documentation.
If the null-checks were tripped, the test would crash anyway. It is not
possible to call `.Any()` and get a valid result instead of an exception
on a null reference.
`InternalChildren` can't viably be `null`, and if it were, we have
bigger problems. The removed null-check was triggering false-positive
inspections further down.
As it turns out, C# 8 provides an attribute that allows annotating that
an `out` parameter's nullability depends on the method's return value,
which is exactly what is desired here.
Uses the usual pattern of two `ReferenceEquals` checks against `this`
and `null` before proceeding to inspect field values. Doing this causes
the compiler to infer that at the point that field values are checked,
`other` can no longer viably be `null`.
`SerializationReader` is not written in a form that would support
turning nullability checking on for the entire class. The biggest
problem there is the inner `DynamicDeserializer` static class, whose
members are initialised via an `initialize()` method, which the compiler
knows nothing about.
For this reason, just opt to suppress the single inspection about
returning a `null` from a method with a return type of `string` (rider
expects `string?`). It would have been also viable to enable nullability
checking for this one method, but that's pretty much the same thing and
adds no safety anyways, so just disable the warning to minimise
surprise.
The previous implementation was checking if the `x0` or `x2` anchors
were selected to decide on which way to transfer the drawable's scale,
but that check actually ends up being always true for corner anchors. To
visualise, this is how the corner anchors correspond to `Anchor` flags:
x0 x1 x2
| | |
y0 -O---O---O-
| | |
y1 -O---+---O-
| | |
y2 -O---O---O-
| | |
The Os indicate where the reference anchors are on a selection box.
The first conditional eliminates the middle ones, which makes sense.
But after excluding them from further deliberations (marking via X):
x0 x1 x2
| | |
y0 -O---X---O-
| | |
y1 -X---+---X-
| | |
y2 -O---X---O-
| | |
The remaining anchors always have `x0` or `x2` set. So to avoid
confusion, just always transfer one way for now. At some point this
should be torn out in favour of an actual implementation of the desired
behaviour.
