A set of F# analyzers which warn against code issues previously encountered by Idura.
You should be able to build the project by simply running dotnet build.
You can test the analyzers by running dotnet test.
The test suite works by running each analyzer on a number of test programs. There are two kinds of tests for the analyzers: positive and negative.
Positive tests check that the analyzer reports issues correctly when presented with a faulty program.
Positive tests run the analyzer on each test program and record the messages generated by the analyzer.
They then check that there is at least one message, and compare the generated messages with a snapshot to check that the analyzer does not change its behavior unintentionally.
If the snapshot does not match, the test suite will fail and generate a __mismatch__ directory containing the new messages so you can compare them with the snapshots while debugging your code.
If you are intentionally changing the behaviour of an analyzer and want to update the snapshots to reflect this, overwrite the old snapshots with the new snapshots generated in the __mismatch__ directory after running the test suite.
Negative tests check that the analyzer does not report issues on correct programs. Negative tests run the analyzer on each test program and check that the analyzer does not produce any messages.
Run dotnet pack --configuration Release to generate a NuGet package (in the bin/Release directory).
You can then use the NuGet package like any other F# analyzer.
The FSharp.Tree.Viewer project contains a small command-line utility which allows you to view the syntax tree and typed declarations as parsed by the F# compiler.
This can be useful to figure out what to look for when traversing the abstract syntax tree to find a specific construct.
From the src/FSharp.Tree.Viewer directory, the tool can be run with e.g. dotnet run -- Program.fs.
Run dotnet run -- Program.fs -- --help to get an overview of the options.
Xunit only supports literals in InlineData.
If you provide a non-literal type as input to an InlineData test, the test will silently be skipped by Xunit.
This can cause surprising behavior when refactoring tests and accidentally changing test input to a non-literal.
This can be somewhat mitigated by annotating the types of all input parameters supplied via InlineData.
This analyzer detects missing type annotations in parameters supplied via InlineData.
| About this analyzer | |
|---|---|
| Code | IDURA-XUNIT-001 |
| Message | Arguments with data supplied via the InlineData annotation must have type annotations |
| Severity | Warning |
| Works in | CLI, Ionide |
XUnit will silently skip any tests tagged with the Fact attribute if they don't have a unit argument. This is an easy way to accidentally disable a test, and it is also easy to forget to add the unit argument.
This analyzer detects missing unit arguments in functions that have the Fact attribute.
| About this analyzer | |
|---|---|
| Code | IDURA-XUNIT-002 |
| Message | Test functions tagged with the XUnit [Fact] attribute must have a unit argument or the test runner will not execute them |
| Severity | Warning |
| Works in | CLI, Ionide |
The random number generator instances in .NET sometimes fail with cryptographic exceptions for no observable reason. We have observed that this can cause the entire generator instance to permanently fail. For this reason, you should never use your own instances of random number generators.
In non-legacy code, the solution is to use the static methods on the RandomNumberGenerator class, which are not bound to a specific instance.
In legacy code where these are not available, you should maintain a pool of random number generator instances which replaces failed generators automatically. At Idura, we have implemented this in our legacy products at Idura by injecting such a pool as a dependency for every module that needs a random number generator.
This analyzer detects use of the constructors and Create methods of the RandomNumberGenerator and RNGCryptoServiceProvider classes.
| About this analyzer | |
|---|---|
| Code | IDURA-CRYPTO-001 |
| Message | Do not use your own instance of RNGCryptoServiceProvider. Depend on a global RNG pool to ensure stability of the generator. |
| Severity | Warning |
| Works in | CLI, Ionide |
| About this analyzer | |
|---|---|
| Code | IDURA-CRYPTO-002 |
| Message | Do not use your own instance of RandomNumberGenerator. Depend on a global RNG pool to ensure stability of the generator. |
| Severity | Warning |
| Works in | CLI, Ionide |
It is dangerous to use the pattern do! ... |> Result.ignore because this may inadvertently end up ignoring an Error if the function being piped accidentally returns a nested Result.
| About this analyzer | |
|---|---|
| Code | IDURA-RESULT-001 |
| Message | The pattern do! ... |
| Severity | Error |
| Works in | CLI, Ionide |
It is dangerous to use the pattern do! ... |> TaskResult.ignore because this may inadvertently end up ignoring an Error if the function being piped accidentally returns a nested TaskResult.
Experience has shown that it is easy to accidentally introduce subtle and high severity bugs with this pattern.
| About this analyzer | |
|---|---|
| Code | IDURA-RESULT-002 |
| Message | The pattern do! ... |
| Severity | Error |
| Works in | CLI, Ionide |
It is dangerous to use the pattern do! ... |> Result.ignore because this may inadvertently end up ignoring an Error if the function being piped accidentally returns a nested Result.
Experience has shown that it is easy to accidentally introduce subtle and high severity bugs with this pattern.
| About this analyzer | |
|---|---|
| Code | IDURA-RESULT-003 |
| Message | The pattern do! ... |
| Severity | Error |
| Works in | CLI, Ionide |
It is dangerous to use the pattern do! ... |> TaskResult.map ignore because this may inadvertently end up ignoring an Error if the function being piped accidentally returns a nested TaskResult.
Experience has shown that it is easy to accidentally introduce subtle and high severity bugs with this pattern.
| About this analyzer | |
|---|---|
| Code | IDURA-RESULT-004 |
| Message | The pattern do! ... |
| Severity | Error |
| Works in | CLI, Ionide |
It is dangerous to use the pattern let! _ = ... because this may inadvertently end up ignoring an Error if the function being piped accidentally returns a nested Result.
Experience has shown that it is easy to accidentally introduce subtle and high severity bugs with this pattern.
| About this analyzer | |
|---|---|
| Code | IDURA-RESULT-005 |
| Message | The pattern let! _ = ... is dangerous because it makes it easy to accidentally ignore errors. |
| Severity | Error |
| Works in | CLI, Ionide |
Wrapping a value in the Result monad twice is often caused by accidentally ignoring a Result.Error.
This can be catastrophic when doing validation of security-related properties.
Intentionally wrapping a value twice is very rare, but can sometimes occur, so this is only a warning.
| About this analyzer | |
|---|---|
| Code | IDURA-RESULT-006 |
| Message | Double-wrapping values in Result is often caused by accidentally ignoring an error. |
| Severity | Warning |
| Works in | CLI, Ionide |
A try/with whose value is a Task, ValueTask or Async guards the construction of the asynchronous computation, not its execution.
Construction almost never throws: the interesting faults are raised while the computation runs, and are delivered when it is awaited, which happens outside the handler.
The handler therefore never runs and the fault escapes unhandled.
Asynchronous sequences are the worst case, because constructing one runs nothing at all.
The fault is thus always raised when the consumer pulls an element.
The fix is to await the computation inside the try, so that the computation expression's own try/with wraps execution (task { try return! ... with ex -> ... }), or to handle faults with a combinator such as TaskResult.catch.
This analyzer detects try/with expressions whose type is Task, ValueTask, Async, IAsyncEnumerable or AsyncSeq, resolving type abbreviations first so that wrappers such as TaskResult are covered too.
To stay off try expressions that are doing their job, it does not report a computation the try merely fetched rather than started, such as a lookup in a cache of work that is already running, nor a body that guards synchronous work as well as the construction unless the handler catches everything.
The latter condition means a genuine bug is missed when the body does synchronous work and the handler names a single exception type.
| About this analyzer | |
|---|---|
| Code | IDURA-ASYNC-001 |
| Message | This 'try' guards the construction of the asynchronous computation, not its execution. A fault raised while the computation runs is delivered when it is awaited so the handler never observes it. Await the computation inside the try or handle faults with a combinator such as TaskResult.catch. |
| Severity | Error |
| Works in | CLI, Ionide |
An asynchronous sequence gets a different message phrased around enumeration rather than awaiting.
A use binding disposes its resource when the enclosing scope is left.
If that scope produces a Task, ValueTask or Async, leaving it means returning the computation, not completing it, so the resource is disposed while the computation is still using it.
For an IAsyncEnumerable or an AsyncSeq, the resource is disposed before any computation happens.
Disposing a CancellationTokenSource in particular does not cancel it.
It releases the timer, so a deadline set with CancelAfter or the constructor overload silently stops working.
The fix is to make the body a computation expression so that the use spans execution rather than construction.
A use (or use!) inside a computation expression goes through the builder's Using member, so its scope already spans the awaits.
This analyzer detects try/finally expressions (which is what a use outside a computation expression is lowered to by the compiler) whose type is Task, ValueTask, Async, IAsyncEnumerable or AsyncSeq.
For a use it reports only when the resource is still referenced by the expression that produces the asynchronous value, so a resource that is fully consumed before that expression is built is not flagged.
A computation the scope merely fetched rather than started is not reported.
Fixing a finding from this rule changes runtime behaviour. Often this will manifest as a timeout that has never fired before beginning to fire, so it is worth baselining existing code and rolling out changes from this analyzer carefully.
| About this analyzer | |
|---|---|
| Code | IDURA-ASYNC-002 |
| Message | This 'use' disposes '' when the function returns the asynchronous computation, not when that computation completes. The computation still holds the resource while it runs, so it observes a disposed 'System.Threading.CancellationTokenSource'. Move the await inside the scope by making the body a computation expression (task { ... return! ... }), so the 'use' spans execution rather than construction. |
| Severity | Error |
| Works in | CLI, Ionide |
The message names the resource, the builder that keeps the type of the flagged expression, and, for a CancellationTokenSource, the released timer.
A non-compiler-generated try/finally, where there is no resource to name, and an asynchronous sequence, where nothing is awaited, get special messages.