Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not effective - CA2200: Rethrow to preserve stack details #6284

Closed
LiangZugeng opened this issue Nov 23, 2022 · 4 comments · Fixed by #6304
Closed

Not effective - CA2200: Rethrow to preserve stack details #6284

LiangZugeng opened this issue Nov 23, 2022 · 4 comments · Fixed by #6304
Labels
Bug The product is not behaving according to its current intended design

Comments

@LiangZugeng
Copy link

LiangZugeng commented Nov 23, 2022

Analyzer

Diagnostic ID: CA2200: Rethrow to preserve stack details

Version: [SDK 7.0.100]

Describe the bug

dotnet build or the build action in Visual Studio for Mac didn't show the warning of CA2200.

Steps To Reproduce

  1. With .NET 7 SDK installed, create a new console app using dotnet new console -o TestConsoleApp
  2. Replace all content with the following code in Program.cs.
class TestsRethrow
{
    static void Main()
    {
        TestsRethrow testRethrow = new TestsRethrow();
        testRethrow.CatchException();
    }

    void CatchException()
    {
        try
        {
            CatchAndRethrowExplicitly();
        }
        catch (ArithmeticException e)
        {
            Console.WriteLine("Explicitly specified:{0}{1}",
               Environment.NewLine, e.StackTrace);
        }

        try
        {
            CatchAndRethrowImplicitly();
        }
        catch (ArithmeticException e)
        {
            Console.WriteLine("{0}Implicitly specified:{0}{1}",
               Environment.NewLine, e.StackTrace);
        }
    }

    void CatchAndRethrowExplicitly()
    {
        try
        {
            ThrowException();
        }
        catch (ArithmeticException e)
        {
            // Violates the rule.
            throw e;
        }
    }

    void CatchAndRethrowImplicitly()
    {
        try
        {
            ThrowException();
        }
        catch (ArithmeticException)
        {
            // Satisfies the rule.
            throw;
        }
    }

    void ThrowException()
    {
        throw new ArithmeticException("illegal expression");
    }
}

Content of the .csproj file

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

Expected behavior

Warning should be reported that CA2200 was found.

Actual behavior

No warnings reported.

MSBuild version 17.4.0+18d5aef85 for .NET
  Determining projects to restore...
  Restored /Users/james/Projects/Temp/TestConsoleApp/TestConsoleApp.csproj (in 39 ms).
  TestConsoleApp -> /Users/james/Projects/Temp/TestConsoleApp/bin/Debug/net7.0/TestConsoleApp.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.64

Additional context

.NET SDK: 7.0.100
OS: MacOS Ventura

@mavasani
Copy link
Contributor

mavasani commented Dec 7, 2022

Interesting, we do have an existing test for this same scenario:

[Fact]
public async Task CA2200_DiagnosticForThrowCaughtExceptionAsync()
{
await VerifyCS.VerifyAnalyzerAsync(@"
using System;
class Program
{
void CatchAndRethrowExplicitly()
{
try
{
ThrowException();
}
catch (ArithmeticException e)
{
[|throw e;|]
}
}
void ThrowException()
{
throw new ArithmeticException();
}
}
");

But the analyzer is not firing on the same code snippet. I am wondering if this is due to a change in Operation tree shape that the analyzer expects. I'll investigate.

@mavasani
Copy link
Contributor

mavasani commented Dec 8, 2022

The issue seems to be that starting C# 8, the operation tree shape for throw ex actually has an IConversionOperation wrapping the thrown exception operation if the type of ex is not System.Exception. This breaks few analyzers that do not handle conversions as operands of throw operation - I'll send a PR with the fix.

mavasani added a commit to mavasani/roslyn-analyzers that referenced this issue Dec 8, 2022
@Youssef1313 Youssef1313 added Bug The product is not behaving according to its current intended design Area-Microsoft.CodeQuality.Analyzers labels Dec 8, 2022
@LiangZugeng
Copy link
Author

Will this fix be included in the next .NET 7 SDK release?

@mavasani
Copy link
Contributor

mavasani commented Dec 8, 2022

No, this would go into .NET 8, especially given this is about false negatives and not false positives. You should be able to get the analyzer NuGet package with this fix from the dotnet public package feed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug The product is not behaving according to its current intended design
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants