Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

Address null checking patterns and specific usage

Open dodexahedron opened this issue 1 year ago • 2 comments

(Intended as a future work item)

This is partially a formatting work item (great for inclusion in a project in that vein).

It also is deeper than just switching the operators, though.

There are places where there are a lot of repeated checks against null or the same other values (or worse - other reference types), which present a couple of problems that simply replacing an == null with is { } does not address:

  1. Those situations are hard to read and maintain and thus potential bug and regression farms.
  2. They present thread-safety issues, if the reference being validated/checked is not, itself, guaranteed thread-safe, both for the reference itself and for the instance of the object the reference points to.
    • For example, those sequential checks are not atomic and are susceptible to race conditions, dirty reads, spurious null reference exceptions, and other fun problems.
  3. Some of these constructs also result in additional unnecessary nesting/indentation, eating up horizontal space and unintentionally spaghettifying the code.
  4. Many of these are replaceable by recursive object patterns and/or null propagation, to condense the conditions.
  5. Many of these are also replaceable by switch statements or switch expressions, not exclusive of the above constructs, which typically have better runtime performance and are a lot more compact and typically clearer.
    • Guard clauses, if used and not carefully designed, can reduce the benefit of the switch, but still are typically better.

dodexahedron avatar Jan 25 '24 18:01 dodexahedron

I'm pretty sure that most of this one will actually be handled by #3212, when the rules are applied, as they include the null checking pattern specification.

However, it will only apply to places where there is literally a == null usage, and will not go farther than that. That's fine as it takes care of the actual theoretical bugs that explicit equality to null creates, but it does still leave plenty of other opportunities for further cleanup and simplification, such as null conditional access (?.), null coalescing (??), and further-simplified recursive pattern matching.

dodexahedron avatar Jan 29 '24 22:01 dodexahedron

Some of these are currently already being done by ReSharper in @tig's big PR.

Additional improvement and simplification is likely to still be possible, so I'll leave this issue open for now.

However, just noting a lot of this work is getting done elsewhere right now and will be merged pretty soon.

dodexahedron avatar Feb 11 '24 02:02 dodexahedron

This got largely done across several PRs to the point this issue can be closed now.

Probably a good idea to raise the severity of the related inspections before release time and address any that remain or that have crept in since then, and then raise it clear to error once that's done, to forbid it ever happening again.

dodexahedron avatar May 05 '24 07:05 dodexahedron