C# CA2000 Dispose Object Before Losing Scope

Question

This is my code, it gives me CA2000 on "new DataTable()..." and "new DataColumn()..."s

usersDS.Tables.Add(new DataTable()
{
    TableName = "Users",
    Columns = { new DataColumn() { ColumnName = "Handle", DataType = typeof(string) }, new DataColumn() { ColumnName = "Nickname" ,DataType = typeof(string) } }
});

Is it possible to fix without declaring variables?

In this case, the code looks fine to me as it is, and I would simply suppress the warning. Neither a `DataTable` nor a `DataColumn` holds any unmanaged resources. See also http://stackoverflow.com/questions/913228/should-i-dispose-dataset-and-datatable — , Nov 22 '14 at 12:38
Related: http://stackoverflow.com/questions/6684206/how-to-fix-a-ca2000-idisposable-c-sharp-compiler-warning-when-using-a-global-ca and http://stackoverflow.com/questions/4264986/to-dispose-or-not-to-dispose-ca2000 — Soner Gönül, Nov 22 '14 at 12:39
@JasonEvans Its base class does. `DataTable` derives from `MarshalByValueComponent`, and that implements `IDisposable`. Same for `DataColumn`. — , Nov 22 '14 at 12:39

score 7 · Accepted Answer · edited May 23 '17 at 11:51

7

This is nearly a duplicate of How to fix a CA2000 IDisposable C# compiler warning, when using a global cache. Maybe it should be considered a duplicate of that one. I'm not sure.

Code Analysis is legitimately complaining that it is theoretically possible for the method to complete without the IDisposable object being disposed and without it being safely stored somewhere else. The latter can happen if an exception occurs during the initialization of the DataTable object or adding the DataTable object to the usersDS.Table object (whatever that is).

If you can guarantee that no exception will be thrown here, then IMHO it is perfectly fine to suppress the CA warning. In that case, you know more than CA can, and you're promising you know what you're doing.

If you can't make the guarantee, then no…it's not possible to fix the warning without introducing local variables so that you're able to dispose the object in the event of an exception. For example:

DataTable dataTable = null;
DataColumn dataColumn1 = null, dataColumn2 = null;

try
{
    dataColumn1 = new DataColumn() { ColumnName = "Handle", DataType = typeof(string) };
    dataColumn2 = new DataColumn() { ColumnName = "Nickname", DataType = typeof(string) };
    dataTable = new DataTable()
    {
        TableName = "Users",
        Columns = { dataColumn1, dataColumn2 }
    };
    usersDS.Tables.Add(dataTable);
}
catch
{
    if (dataTable != null)
    {
        dataTable.Dispose();
    }
    if (dataColumn1 != null)
    {
        dataColumn1.Dispose();
    }
    if (dataColumn2 != null)
    {
        dataColumn2.Dispose();
    }
    throw;
}

edited May 23 '17 at 11:51

Community

1
1

answered Nov 22 '14 at 20:36

Peter Duniho

68,759
7
102
136

"Code Analysis is legitimately complaining that it is theoretically possible for the method to complete without the IDisposable object being disposed" -- It's true that it's possible, but it's not a legitimate complaint for any class for which `Dispose()` has no meaningful effect. As pointed out in the comments, even though `DataTable` and `DataColumn` implement `IDisposable`, that doesn't really do anything useful, and those classes *should* have been designed so that they do not implement `IDisposable`. We cannot fix that. We can avoid pointless calls to their `Dispose()` methods though. – Nov 22 '14 at 20:45
2

The fact that `DataTable` and `DataColumn` have no meaningful action in `IDisposable` is an _implementation detail_. I.e. it's not part of the contract for `DataTable` or `DataColumn`, and could in theory change in the future. It is my preference to code to the public contract, not the private implementation and IMHO it is better to just go ahead and dispose `IDisposable` objects regardless of what you know about their private implementation details. YMMV. – Peter Duniho Nov 22 '14 at 20:50
That makes sense in general, but in this case, not so much. Not only that, your answer is founded on the assumption that it's okay not to dispose of the data tables and columns once they're added to the data set, presumably since disposing of the data set will also dispose the contained tables and columns, but a data set does not do that, and its documentation does not claim to do that. According to your own logic, your own suggested approach is not right. – Nov 22 '14 at 20:57
Really? You want to make a huge argument about this? Even though there's not even a "data set" in the example here? And even though the `System.Data.DataSet` type is authored as part of the same library containing `DataTable` and `DataColumn`? I.e. if changes are made to `DataTable` and/or `DataColumn` it's trivial for the author to also update the code _in the same library_ to match, something that is not true for user code. Really? Oh well...like I said: your mileage may vary. You're welcome to your opinion. – Peter Duniho Nov 22 '14 at 21:06
I'm assuming that `usersDS` is named `usersDS` because it is the "users" `DataSet`. Sorry if that assumption turns out incorrect. BTW, you may want to vote on some of the answers on the (again) [linked question](http://stackoverflow.com/questions/913228/should-i-dispose-dataset-and-datatable): the top-voted answers don't agree with you, but a few of the other answers do. – Nov 22 '14 at 21:09
It may be a fine assumption. But as far as I'm concerned, it wouldn't matter, due to the very close relationship `DataSet` has with `DataTable` and `DataColumn`. Implementation choices that are fine in `DataSet` aren't necessarily fine in user code. – Peter Duniho Nov 22 '14 at 21:16
And I looked at the other answers for the question you linked to, and yes...there are a couple that agree with my position. Including one answerer that has seen in real-world code the harm of not disposing: when the `DataTable` object is owned by a component container, it's not removed from the container unless you dispose it, leading to failure of it to be GC'ed. – Peter Duniho Nov 22 '14 at 21:21

C# CA2000 Dispose Object Before Losing Scope

1 Answers1