NightOwl888 commented on issue #1168:
URL: https://github.com/apache/lucenenet/issues/1168#issuecomment-3375600591
I reserved 3 diagnostic ids for this feature:
Rule ID | Category | Severity | Notes
---------------|----------|----------|-----------------------------------------------------------------------------------------------------------------------------------------------------------
LuceneDev1007 | Design | Warning | Generic Dictionary<TKey, TValue>
indexer should not be used to retrieve values because it may throw
KeyNotFoundException (value type value)
LuceneDev1008 | Design | Warning | Generic Dictionary<TKey, TValue>
indexer should not be used to retrieve values because it may throw
KeyNotFoundException (reference type value)
LuceneDev6000 | Usage | Info | IDictionary indexer may be used to
retrieve values, but must be checked for null before using the value
## LuceneDev1007 and LuceneDev1008
The idea here is that we will use 2 different diagnostic ids, one for value
type values and one for reference type values. I don't know if the code fix
will need both of those ids, but it should detect whether the value of the
dictionary is a value type or reference type and act accordingly (suggesting to
add a `null` check to the fix in the latter case).
Do note that there are several ways that this is used and sometimes the
`null` logic is inverted, so the code fix should be set up to make suggestions
based on whether the null check is for `null` or `not null`.
```c#
public override NumericDocValues GetNumeric(FieldInfo field)
{
if (fields.TryGetValue(field.Name, out DocValuesProducer
producer) && producer != null)
{
return producer.GetNumeric(field);
}
return null;
}
```
```c#
internal void AddBinaryField(FieldInfo fieldInfo, int docID,
BytesRef value)
{
BinaryDocValuesWriter binaryWriter;
if (!writers.TryGetValue(fieldInfo.Name, out DocValuesWriter
writer) || writer is null)
{
binaryWriter = new BinaryDocValuesWriter(fieldInfo,
bytesUsed);
writers[fieldInfo.Name] = binaryWriter;
}
else if (writer is BinaryDocValuesWriter temp)
{
binaryWriter = temp;
}
else
{
throw new ArgumentException($"Incompatible DocValues type:
field \"{fieldInfo.Name}\" changed from {GetTypeDesc(writer)} to binary");
}
binaryWriter.AddValue(docID, value);
}
```
Going forward, the code fix should suggest `is null` (instead of `== null`)
and `is not null` (instead of `!= null`) in these cases.
Also note that in some cases, we are simply returning the value. Java would
not have a `null` check in this case, but we should still suggest a ternary
operator to safely get the value without a `KeyNotFoundException`.
```c#
public SomeClass GetValue()
{
return dict.TryGetValue(key, out T value) ? value : null;
}
```
> NOTE: I strongly suggest you review several examples of how these look in
Java Lucene and how we ported them, and consider that the `.get(key)` method is
normally blindly converted to the indexer `this[key]` in .NET before we
discover that they throw `KeyNotFoundException`. The best way to do this review
is to search for `TryGetValue(` in the Lucene.NET project, and then find the
corrsponding code in [Lucene
4.8.1](https://github.com/apache/lucene/tree/releases/lucene-solr/4.8.1).
Also note that the code fix for `LuceneDev1007` will be much simpler because
no `null` check is needed.
> If it is more sensible to use a new diagnostic id for the return value
case, let me know so I can reserve another one.
### Types to Check
Keep in mind, there are many different implementations of `IDictionary<TKey,
TValue>`, so we should not only be checking for the interface itself, but also
any class that implements it (either directly or through inheritance). But the
scope should be limited to `IDictionary<TKey, TValue>` and
`IReadOnlyDictionary<TKey, TValue>` for these issues because these are the only
ones that throw `KeyNotFoundException`.
## LuceneDev6000
As I mentioned, the `IDictionary.this[key]` indexer property does not throw
a `KeyNotFoundException`, so we can generally use it the same way it is used in
Java. However, this diagnostic is for reviewing these calls and will serve as
an advanced search feature to find them.
```c#
var value = dict[key];
if (value is not null)
{
// use value
}
```
This one doesn't need a code fix, it is just an analyzer we will turn to to
review these, since "Find and Replace" doesn't help us here.
### Types to Check
There are several different implementations that implement the non-generic
`IDictionary` interface. So, the analyzer should be set up for all
implementations as well as the interface itself.
Note that all BCL generic dictionaries support this interface, as well, and
that the behavior of the `this[key]` property changes when casting
`Dictionary<TKey, TValue>` to `IDictionary`.
--------------
Finally, let us know if you run into any roadblocks and we will do our best
to help. And thanks so much for offering to help us!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
