How to check if IDataReader is closed by using the .net compiler API

Question

I am trying to write a code analyzer that will check if there are any IDataReaders that are not closed.

I have gone through this question but it does not explain how it can be done, I have also tried to read through the documentation in the github link The English language used here is too complicated and I did not understand how I will be able to find all instances of type IDataReader and verify that the method close() is being called on it before any variable of the said type goes out of scope.

I have tried creating a project of type Analyzer with code fix in visual studio, I tried to register the operation context in the Initialize method of my class (Which is extended from the type DiagnosticAnalyzer as follows:

 [DiagnosticAnalyzer(LanguageNames.CSharp)]
public class DataReaderAnalyzerAnalyzer : DiagnosticAnalyzer
{
    public const string DiagnosticId = "DataReaderAnalyzer";

    private static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.AnalyzerTitle), Resources.ResourceManager, typeof(Resources));
    private static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.AnalyzerMessageFormat), Resources.ResourceManager, typeof(Resources));
    private static readonly LocalizableString Description = new LocalizableResourceString(nameof(Resources.AnalyzerDescription), Resources.ResourceManager, typeof(Resources));
    private const string Category = "DBConnectionCheck";

    private static DiagnosticDescriptor Rule = new DiagnosticDescriptor(DiagnosticId, Title, MessageFormat, Category, DiagnosticSeverity.Error, isEnabledByDefault: true, description: Description);

    public override ImmutableArray SupportedDiagnostics { get { return ImmutableArray.Create(Rule); } }

    public override void Initialize(AnalysisContext context)
    {

        context.RegisterOperationAction((operationContext) => 
        {
            ((Microsoft.CodeAnalysis.CSharp.Syntax.AssignmentExpressionSyntax)((Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionStatementSyntax)operationContext.Operation.Syntax).Expression).Left
        }
           , OperationKind.ExpressionStatement);
    }
}

I want to find all the references of the occurrence of the variable that holds the type IDataReader, make sure that the close method is being called in this variable before it is lost out of scope.

A sample of my code that I would like to analyze is as follows.

 class Program
{
    static void Main(string[] args)
    {
        IDataReader reader = null;
        try
        {

            Database db = DatabaseFactory.CreateDatabase("ApplicationConnection");



            reader = GetDataReader(db);
            while (reader.Read())
            {
                   //Do somethig with the data here
            }
            reader.Close();
        }
        catch (Exception)
        {

            throw;
        }
        finally
        {
            if (reader != null && !reader.IsClosed)
            {
                reader.Close();
            }
        }
    }
public static IDataReader GetDataReader(Database db)
    {
        DbCommand dbcmd = db.GetSqlStringCommand("some select statement to get data from oracle data base");
        var reader = db.ExecuteReader(dbcmd);
        return reader;
    }
}

Marc_Gravell_23354 · Answer

Ultimately, the code shown isn't great, and IMO it would be the wrong solution to write an analyzer to enforce it.

There is a very simple way of doing this style of operation, and it mostly involves forgetting about Close, and using the fact that it is IDisposable - which is the intended API for this kind of scenario. Then it becomes much, much simpler - so much simpler that a: you don't need a special analyzer for it, and b: existing analyzers that work against IDisposable probably do the job for you.

using var reader = GetDataReader(db);
while (reader.Read())
{
    //Do somethig with the data here
}

with no try/catch/finally etc; the compiler will add everything you need for this to do the right thing, simply via the using. Note that with older compilers, this needs to be:

using (var reader = GetDataReader(db))
{
    while (reader.Read())
    {
        //Do somethig with the data here
    }
}

As a side note: I would strongly suggest not fighting the ADO.NET API - it isn't a useful way of spending your time; tools like Dapper do most common things for you, so you don't need to write this code - and it knows all the corner cases to avoid.

A typical Dapper usage might be:

string region = ...
var users = connection.Query(
    "some * from Users where Region = @region",
    new { region } // parameters
).AsList();

with the library dealing with all the ADO.NET details internally.

user_7484766 · Answer

The below code is a non battle hardened approach you can follow.

analysisContext.RegisterCompilationStartAction(compilationContext =>
{
    var variables = new HashSet();
    var tree = compilationContext.Compilation.SyntaxTrees.First();

    //iterate over all childnodes starting from root
    foreach (var node in tree.GetRoot().ChildNodes())
    {
        var flat = Flatten(node).ToList();
        //find all variable declarations
        var varDecls = flat.OfType();
        foreach (var decl in varDecls)
        {
            if (!(decl.Type is IdentifierNameSyntax id)) continue;
            if (!id.Identifier.Text.Equals("IDataReader")) continue;
            //if you are declaring an IDataReader, go store the var name in set
            foreach (var reader in decl.Variables)
            {
                variables.Add(reader.Identifier.Text);
            }
        }
        //find all method calls i.e. reader.Read() etc
        var invokes = flat.OfType();
        foreach (var invoke in invokes)
        {
            var memberAccess = invoke.Expression as MemberAccessExpressionSyntax;
            var ident = memberAccess.Expression as IdentifierNameSyntax;
            if(!variables.Contains(ident.Identifier.Text)) continue;
            var name = memberAccess.Name as IdentifierNameSyntax;
            //if we find any Close() method on reader, remove from var set
            if (name.Identifier.Text.Equals("Close"))
            {
                variables.Remove(ident.Identifier.Text);
            }
        }
    }
    // if we have any variables left in set it means Close() was never called
    if (variables.Count != 0)
    {
        //this is where you can report
        //var diagnostic = Diagnostic.Create(Rule, location, value);

        //context.ReportDiagnostic(diagnostic);
    }
 
});

public static IEnumerable Flatten(SyntaxNode node)
{
    yield return node;
    var childNodes = node.ChildNodes();
    foreach (var child in childNodes)
        foreach (var descendant in Flatten(child))
            yield return descendant;
}

How to check if IDataReader is closed by using the .net compiler API

2 Answers