apache / apache/lucenenet

Dev Analyzer: Catch helper (extension method) rules

Open
#1,419 0 comments 0 reactions 0 assignees View on GitHub
is:task
Dominant language
C#
Stars
2.4k
Forks
658
Avg merge
3d 5h
Merged PRs (30d)
9

Description

### Is there an existing issue for this?

- [x] I have searched the existing issues

### Task description

The extension methods that were added in #476 (#446) help put some guardrails on which exception types are thrown and caught by wrapping .NET exceptions in static factories and providing helpers for catch blocks to ensure that the business logic matches what Java does. While the exception wrappers for throwing direct users to use the `Create()` methods, the catch blocks require more rigorous inspection to ensure they are wired up correctly.

## Catch Block Rule 1 (Error)

When catch is using an extension method from `Lucene.ExceptionExtentions` (`Lucene.Net` assembly) or `Lucene.Net.ExceptionExtensions` (`Lucene.Net.TestFramework` assembly), it **must** also catch a `System.Exception` type, never anything more specific.

#### Pass

```c#
}
catch (Exception e) when (e.IsIOException())
{
throw RuntimeException.Create(e);
}
```

#### Fail

```c#
}
catch (IOException e) when (e.IsIOException())
{
throw RuntimeException.Create(e);
}
```
The `IsIOException()` extension method includes exceptions that do not subclass `System.IO.IOException`, so this is too strict of a filter to allow all exception types to be caught here than should be. What's more is that this is easy to miss when manually reviewing.

Failure to configure this correctly should result in a build error, not a warning.

Ideally, there would be an associated code fix to correct the issue simply by using `System.Exception` as the type that is caught.

This rule should apply to all tests, as well.

## Catch Block Rule 2 (Warning)

With the exception of the `Lucene.Net.Support` namespace, we should have a rule that reminds users to use a `when` clause with a method from `Lucene.ExceptionExtentions` (`Lucene.Net` assembly) or `Lucene.Net.ExceptionExtensions` (`Lucene.Net.TestFramework` assembly). This rule should apply to all tests, as well (although we could consider excluding tests or test classes that are marked with `[LuceneNetSpecific]` attribute depending on how much noise there is in that category).

### Pass

```c#
}
catch (Exception e) when (e.IsIOException())
{
throw RuntimeException.Create(e);
}
```

### Fail

```c#
}
catch (IOException e)
{
throw RuntimeException.Create(e);
}
```

This rule will need to be suppressed or ignored sometimes, but having a reminder to review and apply the fix would be good. Note that the review that was done in #476 explicitly decided to leave some of these without a `when` clause (particularly when .NET throws a type that could be conflated in our exception handlers), so, we should probably suppress most or all of the issues that exist currently. But this rule will come in handy when porting new code from Lucene.

The fix list should provide options for all of the public extension methods in `Lucene.ExceptionExtentions` (`Lucene.Net` assembly).

> Note that there are public methods for `PrintStackTrace()` and `ToTypeMessageString()` in `Lucene.ExceptionExtentions` (`Lucene.Net` assembly) that should ideally be moved elsewhere (such as `Lucene.Net.Util.ExceptionExtensions`) that are not part of this system. Failing that, they should be ignored from the code fix as options.

The extension methods in `Lucene.Net.ExceptionExtensions` (`Lucene.Net.TestFramework` assembly) are overrides for tests when catching more specific exception types is required. These share the exact same signature of those extension methods in `Lucene.ExceptionExtentions` (`Lucene.Net` assembly). I am just pointing this out in case the types matter for the analyzer or fix. The syntax for reaching either is exactly the same. The compiler ensures the "closest" match wins.

Contributor guide

Open the contributing guide

Research direction

Start by locating the Dev Analyzer entry point and reviewing the extension methods in Lucene.ExceptionExtentions and Lucene.Net.ExceptionExtensions. Define the two catch-block diagnostics, their severity and namespace exceptions, then add code-fix options for the applicable public methods while excluding PrintStackTrace() and ToTypeMessageString().

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.