Rule S2221: "Exception" should not be caught when not required by called methods #458
Conversation
| "id": "S2221", | ||
| "message": "Catch a list of specific exception subtype or use exception filters instead.", | ||
| "location": { | ||
| "uri": "Ember-MM\Ember.Plugins\PluginSectionHandler.cs", |
There was a problem hiding this comment.
One of the tests raises on this :)
try
{
(code)
}
catch
{
//TODO: Hmmm?
}
| @@ -2233,7 +2233,7 @@ | |||
| <value>misra,cert</value> | |||
| </data> | |||
| <data name="S2201_Title" xml:space="preserve"> | |||
| <value>Return values from functions without side effects should not be ignored</value> | |||
| <value>Return values should not be ignored when function calls don't have any side effects</value> | |||
There was a problem hiding this comment.
Should not have been updated by your commit.
There was a problem hiding this comment.
Hmm, not sure where it came from.
| { | ||
| var caughtTypeSyntax = catchDeclaration?.Type; | ||
| return caughtTypeSyntax == null || | ||
| semanticModel.GetTypeInfo(caughtTypeSyntax).Type.Is(KnownType.System_Exception); |
There was a problem hiding this comment.
Not sure whether this could happen or not but you might want to check for null after the GetTypeInfo() call.
There was a problem hiding this comment.
This won't happen, as it returns a struct. I've added more test cases.
There was a problem hiding this comment.
Ok thanks for the clarification.
| if (!c.IsTest() && | ||
| IsSystemException(catchClause.Declaration, c.SemanticModel) && | ||
| catchClause?.Filter?.FilterExpression == null && | ||
| !IsThrowTheLastStatementInTheBlock(catchClause?.Block)) |
There was a problem hiding this comment.
Not sure this is the best heuristic. I am in favor of early exit so I would most probably not have the throw as the last statement.
There was a problem hiding this comment.
Not ideal, but this is what fxcop does :)
Very likely because before exception filters it was a common thing to catch exception, do something if it is of particular type, otherwise rethrow.
There was a problem hiding this comment.
Ok so let's keep it like this.
| { | ||
| var catchClause = (CatchClauseSyntax)c.Node; | ||
|
|
||
| if (!c.IsTest() && |
There was a problem hiding this comment.
Are we supposed to ignore test classes?
There was a problem hiding this comment.
The rule is already quite noisy, so I think it is reasonable. Should I flag it in rspec?
There was a problem hiding this comment.
I don't think that "the rule is quite noisy" is the right explanation, also I tend to think that it is as bad to do it in tests as in real production code. Most of the time if you have a unit test catching exception you will not have the right behavior (of course you could test that some method throws nothing at all but I am pretty sure 90% of the cases could use a specific exception type and the rest mark it as FP). So I would rather update the code than the RSPEC.
@valhristov WDYT?
There was a problem hiding this comment.
I'll appreciate a verbal explanation because I am not sure what are you talking about :)
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
May be you should not use DescendantNodes() because it is recursive. I think, you only need the last statement in the block to be throw, hence you could probably use ChildNodes... In other words this should not be acceptable:
catch
{
if (something)
{
throw;
}
}
but this should be:
catch
{
if (something) { ... }
throw;
}
There was a problem hiding this comment.
That is exactly what I thought, but fxcop disagrees - first example is considered fine.
There was a problem hiding this comment.
There are cases when the exception is logged (via different mechanisms, so we cannot always tell) and some cases with "return" - but I'm not sure if the verbosity can be reduced. This rule is probably only useful in some contexts, and is not in sonar way.
|
@michalb-sonar You need to rebase on master to fix the QA |
michalb-sonar
changed the title
Fix #433