KSP-CKAN / CKAN

The Comprehensive Kerbal Archive Network
https://forum.kerbalspaceprogram.com/index.php?/topic/197082-*
Other
1.99k stars 348 forks source link

Avoid index out of range #4176

Closed HebaruSan closed 2 months ago

HebaruSan commented 2 months ago

Problem

Right after #4171, we got a report of an index out of bounds exception in the manage game instances dialog, which was fixed in 2ddf740aca139f017c061ee916958f3cbfc39b72. Today another came up in a different method of the same class.

Cause

Lines like this cause warnings (which we promote to errors) with nullable reference enabled:

https://github.com/KSP-CKAN/CKAN/blob/82e5c3576c3a8abaa42092edeb41399bc03af3b8/GUI/Dialogs/ManageGameInstancesDialog.cs#L352

We need to check whether GameInstancesListView.SelectedItems[0].Tag is actually a string rather than null before we cast it, so I did that:

https://github.com/KSP-CKAN/CKAN/blob/c13930f3cb400bfe22ee7a6c5a84a4e807e91a4a/GUI/Dialogs/ManageGameInstancesDialog.cs#L361-L366

But now we're evaluating GameInstancesListView.SelectedItems[0] before HasSelections runs to confirm that list is non-empty, so when it is empty, it's an array bounds error.

Ideal solution and why we can't do it

C# 11 provides a great way to simplify checking array bounds and eliminate hard coded indexes:

Initially I did a code search to find every instance of a hard coded array index and worked up a set of changes to replace them with list patterns, but I hit a snag when I tried to build it with Mono:

2>CSC : error CS1617: Invalid option '11' for /langversion. Use '/langversion:?' to list supported values.
$ csc /langversion:?
Supported language versions:
default
1
2
3
4
5
6
7.0
7.1
7.2
7.3
8.0
9.0 (default)
latestmajor
preview
latest

So we need a solution that works on C# 9.

Changes

Now every hard coded array index access is replaced by:

If we are ever able to migrate to C# 11, we will have the option of switching to the list patterns that are already there in these comments.