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

Detect invalid access to IsFailure on ServiceResult<T> objects. #43

Open
randymarsh77 opened this issue Jan 23, 2019 · 1 comment
Open
Labels
new analyzer Suggestion for a new analyzer to add

Comments

@randymarsh77
Copy link
Member

Calling .Value on a ServiceResult<T> will call Verify(), which will throw if the result is a failure.

However, this semantic might not be obvious, so a developer might think this is perfectly fine error checking code:

var result = await ServiceResultReturningTask();
var someImportantValue = result.Value?.Property;
if (result.IsFailure || someImportantValue == null)
  ... error case

Instead, this code would throw.

It seems like accessing to the .Value property before accessing the .IsFailure is always a bug, and also detectable.

A common case that is not a bug would be using the Verify() semantic of .Value to throw on failure. In this case, explicitly accessing .IsFailure would not be needed.

@randymarsh77 randymarsh77 added the new analyzer Suggestion for a new analyzer to add label Jan 23, 2019
@randymarsh77
Copy link
Member Author

randymarsh77 commented Jan 23, 2019

Suggested code fixes:

var result = await ServiceResultReturningTask();
if (result.IsFailure || result.Value?.Property == null)
  ... error case
var result = await ServiceResultReturningTask();
var someImportantValue = result.IsFailure ? null : result.Value?.Property;
if (someImportantValue == null)
  ... error case

someImportantValue.useThisObject();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new analyzer Suggestion for a new analyzer to add
Development

No branches or pull requests

1 participant