Add Roslyn analyzer to detect improper catching of DotvvmInterruptRequestExecutionException#1965
Add Roslyn analyzer to detect improper catching of DotvvmInterruptRequestExecutionException#1965
Conversation
…cutionException Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
src/Analyzers/Analyzers/ApiUsage/DotvvmInterruptExceptionAnalyzer.cs
Outdated
Show resolved
Hide resolved
|
@copilot Please look at my review comments and make the requested changes. |
…es, and nested try blocks Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
| foreach (var catchClause in tryOperation.Catches) | ||
| { | ||
| var caughtType = catchClause.ExceptionType; | ||
| if (caughtType == null) |
There was a problem hiding this comment.
@copilot Does this mean that if there is just catch { ... }, we just ignore it and report that the exception is handled correctly? Can you verify this by adding a test for the catch clause without the specified exception type?
There was a problem hiding this comment.
You're correct - the original code was skipping bare catch { } blocks entirely. I've now fixed this by handling bare catch blocks conservatively: they always trigger the analyzer warning, regardless of whether they contain a throw; statement. This is because we cannot reliably detect rethrows in bare catch blocks through Roslyn's operation tree. Users should use catch (DotvvmInterruptRequestExecutionException) { throw; } instead for proper handling. Added tests for bare catch blocks with and without rethrows - both now correctly trigger warnings. Commit: 94f1b1a
…atic Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
Users wrapping
ReturnFile*orRedirect*calls in try-catch blocks inadvertently swallowDotvvmInterruptRequestExecutionException, breaking request flow control.Changes
ReturnFile*orRedirectTo*methods onIDotvvmRequestContextare called within try-catch blocks that catchDotvvmInterruptRequestExecutionException(or base types likeException) without rethrowingthrow;anywhere in catch block, including inside if statements or other nested operationswhen (ex is not DotvvmInterruptRequestExecutionException))catch { }blocks (without exception type) are always flagged as problematic since we cannot reliably detect rethrows within them. Users should use explicit exception types instead.Example
The analyzer flags these patterns:
And accepts these patterns:
Warning
<issue_title>Roslyn analyzer to detect catching DotvvmInterruptRequestExecutionException</issue_title>
<issue_description>A couple of users were dealing with this unintuitive behavior - when they perform a redirect or returning a file, they wrap it in a try/catch block, and catch all exceptions. However, DotVVM throws
DotvvmInterruptRequestExecutionExceptionwhich needs to be rethrown.We already have Roslyn analyzers in the project. We should detect this pattern when
ReturnFile*orRedirect*is called onIDotvvmRequestContext, and if it is inside try/catch block, ensure there is a special case forDotvvmInterruptRequestExecutionExceptionwhere the exception is rethrown as such:<issue_title>Roslyn analyzer to detect catching DotvvmInterruptRequestExecutionException</issue_title>
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.