Suspicious precondition checks - SergeyTeplyakov/ErrorProne.NET GitHub Wiki
Let's start from the demo first and than I will dive into more details about what "suspicous" preconditions are and why they're actually so suspicious:
C# is a high level language that could perform various complex transformation during code generation. In most cases such transformation are very reasonable and invisible to the user. But in some case high level syntax constructs could affect how program behave in terms, for instance, exception handling. Two good examples are related to argument validation for async methods and iterator blocks.
Consider following example:
public IEnumerable<string> ReadFile(string fileName)
{
if (fileName == null) throw new ArgumentNullException(nameof(fileName));
using (var textReader = File.OpenText(fileName))
{
string line;
while ((line = textReader.ReadLine()) != null)
{
yield return line;
}
}
}
Maybe this code is looking suspicious from the beginning, but this not the poing here. The main question for this code right now is related to exception propagation and about the time when this exception would be observed by the client of this code. So the question is when this code will blow up?
// Here?
var content = ReadFile(null);
// Or here?
var projection = content.Select(s => s.Length);
// Or maybe here?
var result = projection.Count();
Because iterator blocks are implemented as a state machine under the hood, the body of the method ReadFile
will not be executed until the very first call to MoveNext
method of the iterator. It means, that ArgumentNullException
will occur only on the third line in the previous example. (And of course you can imagine more complex scenarios when iterator object could be passed around though different layers of abstraction that could cause to really weird errors in seemingly unrelated parts of the codebase)
Such behaviour could be a big suprise for many people and it contradicts from a common understand that method precondition checks should happen immediately during a method invocation (that's why if you'll switch to Contract.Requires
instead of if-throw
the behavior would be "correct" immediately: contract violation would be "synchronous" and will happen during the call to ReadFile
method).
The solution of this problem is fairly simple: we need to split the method in two, leave argument validation in one method and move the rest of the method into another one.
As you may suspect following transformation was made by ErrorProne.NET:
public IEnumerable<string> ReadFile(string fileName)
{
if (fileName == null) throw new ArgumentNullException(nameof(fileName));
return DoReadFile(fileName);
}
private IEnumerable<string> DoReadFile(string fileName)
{
using (var textReader = File.OpenText(fileName))
{
string line;
while ((line = textReader.ReadLine()) != null)
{
yield return line;
}
}
}
Asynchronous methods (methods marked with async
keyword) shares some implementation aspects with iterator blocks. Both of them are implemented using similar state machines to emulate continuation passing style behavior. Similar implementation details lead to similar conserns in terms of exception handling.
Every method has formal or informal contract: method requires something from it's clients (i.e. method preconditions) and if those requirements are met, it will guarantee some results if possible (i.e. method postconditions). In terms of exception handling, preconditions are usually modeled via exceptions derived from System.ArgumentExcpetion
, and failed postcondition could lead to exception of any other type.
Task-based async pattern introduces additional way of handling exceptions: some exceptions could happen synchronously from client point of view and some exceptions could happen inside the deep guts of the method and would be notified to the client via failed task. But when TAP method is implemented using C# constructs like async/await
this difference could easily be lost.
Consider following async method:
public async Task<int> GetLengthAsync(string s)
{
if (s == null) throw new ArgumentNullException(nameof(s));
await Task.Delay(42);
return s.Length;
}
Of course this method is very artificial, but you may ask yourself, can you think about if-throw
statement as of precondition check, and when it would be observed by the client?
// When excpetion will happen?
// During GetLengthAsync method call?
Task<int> task = GetLengthAsync(null);
// Or during observing task's state?
int lenth = task.GetAwaiter().GetResult();
As you may guess already, "precondition" check in async method is not actually a precondition any more, because it won't happen "synchronously" during method invocation, but it will fail the resulting task instead! Solution to this problem is similar to previous one: you may use Contract.Requires
and leave the problem to ccrewrite to handle, or you may use ErrorProne.NET and extract the method to split precondition check from the rest of the async method:
public Task<int> GetLengthAsync(string s)
{
if (s == null) throw new ArgumentNullException(nameof(s));
return DoGetLengthAsync(s);
}
private async Task<int> DoGetLengthAsync(string s)
{
await Task.Delay(42);
return s.Length;
}