faithlife / faithlifeanalyzers Goto Github PK

When sorting strings (i.e. not comparing them for equality), ignoring case (e.g. by using *IgnoreCase from StringComparison or StringComparer) will order case differences arbitrarily. You probably want case differences to be ordered consistently and, if sorting strings to show to the user, culturally-sensitively.

Task<T>.GetAwaiter().GetResult() should be preferred over Task<T>.Result, and (void) Task.GetAwaiter().GetResult() over Task.Wait().

Similar to CurrentAsyncWorkItemAnalyzer, use of Libronix.Utility.Threading.AsyncEnumerableUtility.UntilCanceled() should be disallowed outside of IEnumerable<AsyncAction> methods.

@bryanrsmith recently introduced me to commitlint and semantic-release, which both greatly simplify the process of issuing releases and release notes.

By contrast, the release policy in this repo is to manually update Version History both when making changes and when issuing a new release. (Aside: I'm not even entirely certain how new releases happen. I suspect that it has something to do with manually changing a version number somewhere, but I'm not confident about this.)

I'd like to adopt something similar for FaithlifeAnalyzers (and possibly other Faithlife C# projects), but there don't seem to be mature alternatives to commitlint and semantic-release in the .net ecosystem.

I did, however, find this article, which describes using semantic-release directly from cake.

Are there any strong objections to trying this out? (particularly from active contributors: @bgrainger, @ejball, @ddunkin, @StephenCleary)

In addition to configuring semantic-release, I'd also want to reconfigure the build to run commitlint against all PR commit messages, since semantic-release isn't reliable without parsable commit messages.

This last bit is perhaps the largest potential downside: in the node ecosystem, there are tools like husky that make it easy to install precommit hooks that run commitlint at commit time—without this, some developers might miss the need to follow the commit message guidelines until the PR build output informs them of violations—resulting in the need for a possibly painful rebase.

If a type is used with JSON-serialization methods (for example JsonUtility.ToJson), raise a warning if the type does not have a Dto suffix.

For types that do have a Dto suffix, verify that all properties are auto-implemented and are of "approved" types:

• string
• number (int, long, float, double, whatever…)
• bool
• other types that have a Dto suffix
• ReadOnlyCollection / IReadOnlyCollection of any of the above
• ReadOnlyDictionary IReadOnlyDictionary from string to any of the above.

These will definitely need to be warnings, given the number of historical violations that we have.

Historically, we've enforced a nitpicky guideline that when .Count and .Length are available, they should be preferred over .Any() or .Count().

We could write an analyzer that enforces this, but the more that I think about it, the more it seems like this guidance is primarily aimed at avoiding multiple enumerations of IEnumerable. (see #32)

Perhaps we should instead have good analyzers for #32 and instead have a guideline that we should always use the most appropriate of

• .Any()
• .Count()
• .CountIsExactly()
• .CountIsAtLeast()

We never cloned the awaitable duck-typing interface of DispatcherOperation in our port of it to Mac. Writing code that awaits a DispatcherOperation will compile, and then crash at runtime when executed on Mac.

Furthermore, a common mechanism of obtaining a DispatcherOperation is from a call to DispatcherUtility.BeginInvokeIfNecessary, which will return null if the invoke is not necessary. This makes would-be usage somewhat a pit of failure.

Instead, either:

• Use BeginInvoke(IfNecessary) and ignore the result if fire-and-forget is sufficient.
• Use an explicit thread-switching mechanism if an AsyncMethodContext or AsyncActions context is available.
• Use Invoke to synchronously execute code on the Dispatcher's context in a cross-platform compatible way.

When used on strings, Enumerable.Min and Enumerable.Max use the current culture for ordering. Unfortunately, there are no overloads that take an IComparer<string>.

StyleCop does not appear to support our preferred rules, and a quick search did not turn up any analyzers that do.

Our main rule is

Omit optional braces around single line blocks.

However, it has some exceptions and nuances:

• The emphasis is on single line, rather than single statement. If a statement is split onto multiple lines, it should be surrounded by braces.
• If the control flow mechanism (if condition, using declaration, etc.) is split onto multiple lines, include braces.
• If the statement is part of an if-else chain in which any of the blocks require braces, use braces on all of them. (this part is covered by SA1520)

The little-used AsyncWorkItem.Start method has two overloads:

static AsyncWorkItem Start(AsyncWorkGroup group, Action<object> action);
static AsyncWorkItem Start(AsyncWorkGroup group, Action<object> action, object state, AsyncWorkOptions options);

Within the body of action it is permissible to use AsyncWorkItem.Current. This is very difficult to check for in general.

We could probably allow (and enforce) the following pattern, though:

AsyncWorkItem.Start(group, obj => Method(AsyncWorkItem.Current, obj), (data), AsyncWorkOptions.None);

void Method(IWorkState state, object obj)
{ /* ... */ }

That is, AsyncWorkItem.Current is permitted within the body of a lambda expression passed to AsyncWorkItem.Start. This "names" the work state and permits it to be used in the called method.

No code fix provider is required to produce this; we just need to stop flagging it as an illegal usage of AsyncWorkItem.Current.

Flag async void as an error. We have other patterns for fire-and-forget work (and don't need its primary use case, attaching async work as an event handler).

If the current project has a reference to Libronix.Utility or Faithlife.Utility, there should be a code fix that recommends switching to StartsWithOrdinal (or whichever method is appropriate). The code fix should add a using declaration if necessary.

I could easily be persuaded to go the other way on this one, but the point is that we should have exactly one way of doing it.

See Faithlife/FaithlifeUtility#5.

ToReadOnlyCollection has somewhat unpredictable behaviour, depending on the nature of the IEnumerable<T> it's called on:

• ReadOnlyCollection<T>: returns same object
• List<T>: returns an .AsReadOnly() wrapper around it; changes the caller makes to the list will be reflected in this wrapper, which is usually undesirable behaviour when the return value of this call is stored in a field by a constructor
• IEnumerable<T>: calls .ToList().AsReadOnly() and gets a new immutable list

Furthermore, this method returns ReadOnlyCollection<T>, which we would like to phase out.

Most usages of this method should be replaced with AsReadOnlyList(); some should change to .ToList().AsReadOnly().

N.B. enforcing this rule would be a huge breaking change. An intermediate step might be to enforce that .ToReadOnlyCollection() may not be used to initialize fields or properties in constructors and that .ToList().AsReadOnly() must be used instead. (This addresses the buggiest and least frequent usage.)

The expression !a.Equals(b) is converted to !a == b, which is invalid C#. The suggested replacement should be a != b.

Test case to repro:

		[Test]
		public void DiagnoseAndFixNegatedStringEquals()
		{
			const string program = @"using System;
namespace ConsoleApplication1
{
	class Program
	{
		static void Main(string[] args)
		{
			GC.KeepAlive(!""a"".Equals(""b""));
		}
	}
}
";
			var expected = new DiagnosticResult
			{
				Id = OverloadWithStringComparisonAnalyzer.AvoidStringEqualsDiagnosticId,
				Message = "Use operator== or a non-ordinal StringComparison.",
				Severity = DiagnosticSeverity.Warning,
				Locations = new[] { new DiagnosticResultLocation("Test0.cs", 8, 22) },
			};

			VerifyCSharpDiagnostic(program, expected);

			const string fix1 = @"using System;
namespace ConsoleApplication1
{
	class Program
	{
		static void Main(string[] args)
		{
			GC.KeepAlive(""a"" != ""b"");
		}
	}
}
";
			VerifyCSharpFix(program, fix1, 0);

			const string fix2 = @"using System;
namespace ConsoleApplication1
{
	class Program
	{
		static void Main(string[] args)
		{
			GC.KeepAlive(!""a"".Equals(""b"", StringComparison.OrdinalIgnoreCase));
		}
	}
}
";
			VerifyCSharpFix(program, fix2, 1);
		}

A pattern that I sometimes see in C# is:

var foo = default(string);
if (condition1)
  foo = bar;
else if (condition2)
  foo = baz;
else
  foo = foobar;

In cases like this, the initial assignment of foo is unnecessary and arguably harmful.

If a variable is left unassigned, C# will analyze possible code paths and issue errors for any instances in which the variable is accessed before being definitely assigned.

Assigning an initial value circumvents this check and obscures developer intent. (Aside: I suspect that this practice is inspired by languages such as C++, in which a declaration without an assignment results in undefined behavior)

The proposed analyzer would flag the above snippet as an error. Similarly, any case in which a variable has two assignments without any possible accesses between them should be flagged as an error. (I'm uncertain of the practicality of searching for the latter case—this one may need to be punted)

Another common pattern is to have an initial assignment followed immediately by a check that sets the variable to a new value:

var foo = default(string);
if (condition)
  foo = overriddenValue;

These would be better written as

var foo = condition ? overriddenValue : default(string);

or

string foo;
if (condition)
  foo = overriddenValue;
else
  foo = default(string);

Although this pattern is sufficiently common that it may be more appropriate to warn instead of error for these.

Code fix nuances

For the first pattern, the code fix provider should offer to remove the initial assignment; this will need to be able to transform a var into an explicitly declared type.

There may be some edge cases related to ref and out parameters, especially when considering the pairs of assignments without intermediate access and early return statements.

Some initial assignments may have side effects (e.g. an assignment to the result of a method call). Code fix providers should not remove the method call when removing the additional assignment. Similarly, when assignments may have side effects, code fix providers should take care not to change the order in which statements are executed.

It would be better to provide no fix than to provide a fix that might change the behavior of the method.

Default parameters work by using OptionalAttribute and DefaultParameterValueAttribute, and the defaults are specified at compile-time based on the exact type of the object on which the method is called. That is, if you have default parameters specified both on an interface IWidget and a concrete class Widget, the defaults that you get are dependent on whether your variable is declared as IWidget or as Widget.

public interface IWidget
{
  void DoSomething(int parameter = 0);
}

public sealed class Widget : IWidget
{
  public void DoSomething(int parameter = 1)
  {
    Console.WriteLine(parameter);
  }
}

…

IWidget a = new Widget();
Widget b = new Widget();

a.DoSomething(); // prints 0
b.DoSomething(); // prints 1

Similarly, if IWidget has a method with default parameters specified and Widget does not declare those default parameters, then you cannot use default parameters if you have a variable of type Widget.

public interface IWidget
{
  void DoSomething(int parameter = 0);
}

public sealed class Widget : IWidget
{
  public void DoSomething(int parameter)
  {
    Console.WriteLine(parameter);
  }
}

…

IWidget a = new Widget();
Widget b = new Widget();

a.DoSomething(); // prints 0
b.DoSomething(); // compiler error

These rules are exactly the same for virtual methods of classes.

Because of the potential for confusion, this leads me to the conclusion that default parameters should not be used for virtual members. (including interfaces)

Array and List<T> have Sort and BinarySearch methods that take an IComparer<string>.

In the following code, the standard AsyncWorkItem.Current fixes should be offered, but they aren't:

if (AsyncWorkItem.Current.Canceled)
{
    // ...
}

Prefer "" over String.Empty (or string.Empty).

For the incorrect code str.IndexOf("x") where "x" is a string constant consisting of one character, suggest the fix str.IndexOf('x'), which performs an ordinal search.

I'm told that ReSharper does find this one (see #36), although it is unaware of WhereNotNull.

Caveat: we may not wish to flag instances in which WhereNotNull is called with a Nullable<T> type parameter, since this also changes the return type of the expression.

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/Configuration.md#sharing-configuration-among-solutions describes how to package a stylecop.ruleset file in order to share it among multiple solutions. This issue is to add StyleCop.Analyzers 1.0.2 (most recent at the time this issue was created) as a nuget dependency to Faithlife.Analyzers and bundle in a Faithlife stylecop.ruleset with our styling guidelines.

Concerns:

• Does it make sense to bundle in style issues with Faithlife.Analyzers?
• Do all teams working in C# at Faithlife agree on one set of style rules?

Here's a stylecop.ruleset file already in use at Faithlife that can serve as a discussion starter (currently used by multiple teams):

<?xml version="1.0" encoding="utf-8"?>
<RuleSet Name="Faithlife Custom Rules" Description="Based on the Microsoft Minimum Recommended Rules. These rules focus on the most critical problems in your code, including potential security holes, application crashes, and other important logic and design errors. It is recommended to include this rule set in any custom rule set you create for your projects." ToolsVersion="14.0">
  <Localization ResourceAssembly="Microsoft.VisualStudio.CodeAnalysis.RuleSets.Strings.dll" ResourceBaseName="Microsoft.VisualStudio.CodeAnalysis.RuleSets.Strings.Localized">
    <Name Resource="MinimumRecommendedRules_Name" />
    <Description Resource="MinimumRecommendedRules_Description" />
  </Localization>
  <Rules AnalyzerId="Microsoft.Analyzers.ManagedCodeAnalysis" RuleNamespace="Microsoft.Rules.Managed">
    <Rule Id="CA1001" Action="Warning" />
    <Rule Id="CA1003" Action="None" />
    <Rule Id="CA1009" Action="None" />
    <Rule Id="CA1012" Action="Warning" />
    <Rule Id="CA1013" Action="Warning" />
    <Rule Id="CA1032" Action="None" />
    <Rule Id="CA1033" Action="Warning" />
    <Rule Id="CA1038" Action="Warning" />
    <Rule Id="CA1041" Action="Warning" />
    <Rule Id="CA1043" Action="Warning" />
    <Rule Id="CA1044" Action="None" />
    <Rule Id="CA1046" Action="Warning" />
    <Rule Id="CA1047" Action="Warning" />
    <Rule Id="CA1048" Action="Warning" />
    <Rule Id="CA1049" Action="Warning" />
    <Rule Id="CA1050" Action="Warning" />
    <Rule Id="CA1051" Action="Warning" />
    <Rule Id="CA1052" Action="Warning" />
    <Rule Id="CA1053" Action="Warning" />
    <Rule Id="CA1058" Action="Warning" />
    <Rule Id="CA1060" Action="Warning" />
    <Rule Id="CA1061" Action="Warning" />
    <Rule Id="CA1063" Action="Warning" />
    <Rule Id="CA1064" Action="Warning" />
    <Rule Id="CA1065" Action="Warning" />
    <Rule Id="CA1301" Action="Warning" />
    <Rule Id="CA1307" Action="None" />
    <Rule Id="CA1309" Action="None" />
    <Rule Id="CA1400" Action="Warning" />
    <Rule Id="CA1401" Action="Warning" />
    <Rule Id="CA1403" Action="Warning" />
    <Rule Id="CA1404" Action="Warning" />
    <Rule Id="CA1410" Action="Warning" />
    <Rule Id="CA1415" Action="Warning" />
    <Rule Id="CA1500" Action="Warning" />
    <Rule Id="CA1502" Action="None" />
    <Rule Id="CA1504" Action="Warning" />
    <Rule Id="CA1505" Action="None" />
    <Rule Id="CA1708" Action="Warning" />
    <Rule Id="CA1711" Action="None" />
    <Rule Id="CA1712" Action="Warning" />
    <Rule Id="CA1713" Action="Warning" />
    <Rule Id="CA1714" Action="None" />
    <Rule Id="CA1715" Action="Warning" />
    <Rule Id="CA1717" Action="None" />
    <Rule Id="CA1719" Action="Warning" />
    <Rule Id="CA1721" Action="None" />
    <Rule Id="CA1722" Action="Warning" />
    <Rule Id="CA1724" Action="None" />
    <Rule Id="CA1725" Action="Warning" />
    <Rule Id="CA1800" Action="None" />
    <Rule Id="CA1801" Action="Warning" />
    <Rule Id="CA1802" Action="Warning" />
    <Rule Id="CA1804" Action="Warning" />
    <Rule Id="CA1806" Action="None" />
    <Rule Id="CA1809" Action="None" />
    <Rule Id="CA1811" Action="None" />
    <Rule Id="CA1813" Action="None" />
    <Rule Id="CA1821" Action="Warning" />
    <Rule Id="CA1822" Action="None" />
    <Rule Id="CA1823" Action="Warning" />
    <Rule Id="CA1900" Action="Warning" />
    <Rule Id="CA1901" Action="Warning" />
    <Rule Id="CA2000" Action="None" />
    <Rule Id="CA2002" Action="Warning" />
    <Rule Id="CA2101" Action="Warning" />
    <Rule Id="CA2104" Action="None" />
    <Rule Id="CA2108" Action="Warning" />
    <Rule Id="CA2111" Action="Warning" />
    <Rule Id="CA2112" Action="Warning" />
    <Rule Id="CA2114" Action="None" />
    <Rule Id="CA2116" Action="Warning" />
    <Rule Id="CA2117" Action="Warning" />
    <Rule Id="CA2121" Action="Warning" />
    <Rule Id="CA2122" Action="Warning" />
    <Rule Id="CA2123" Action="Warning" />
    <Rule Id="CA2124" Action="Warning" />
    <Rule Id="CA2126" Action="Warning" />
    <Rule Id="CA2131" Action="Warning" />
    <Rule Id="CA2132" Action="Warning" />
    <Rule Id="CA2133" Action="Warning" />
    <Rule Id="CA2134" Action="Warning" />
    <Rule Id="CA2137" Action="Warning" />
    <Rule Id="CA2138" Action="Warning" />
    <Rule Id="CA2140" Action="Warning" />
    <Rule Id="CA2141" Action="Warning" />
    <Rule Id="CA2146" Action="Warning" />
    <Rule Id="CA2147" Action="Warning" />
    <Rule Id="CA2149" Action="Warning" />
    <Rule Id="CA2200" Action="Warning" />
    <Rule Id="CA2201" Action="Info" />
    <Rule Id="CA2207" Action="Warning" />
    <Rule Id="CA2208" Action="Warning" />
    <Rule Id="CA2211" Action="Warning" />
    <Rule Id="CA2212" Action="Warning" />
    <Rule Id="CA2213" Action="Warning" />
    <Rule Id="CA2214" Action="Warning" />
    <Rule Id="CA2215" Action="Warning" />
    <Rule Id="CA2216" Action="Warning" />
    <Rule Id="CA2217" Action="Warning" />
    <Rule Id="CA2218" Action="Warning" />
    <Rule Id="CA2219" Action="Warning" />
    <Rule Id="CA2220" Action="Warning" />
    <Rule Id="CA2222" Action="Warning" />
    <Rule Id="CA2224" Action="Warning" />
    <Rule Id="CA2226" Action="Warning" />
    <Rule Id="CA2229" Action="Warning" />
    <Rule Id="CA2230" Action="Warning" />
    <Rule Id="CA2231" Action="Warning" />
    <Rule Id="CA2232" Action="Warning" />
    <Rule Id="CA2235" Action="Warning" />
    <Rule Id="CA2236" Action="Warning" />
    <Rule Id="CA2238" Action="Warning" />
    <Rule Id="CA2240" Action="Warning" />
    <Rule Id="CA2241" Action="Warning" />
    <Rule Id="CA2242" Action="Warning" />
  </Rules>
  <Rules AnalyzerId="StyleCop.Analyzers" RuleNamespace="StyleCop.Analyzers">
    <Rule Id="SA1003" Action="None" />
    <Rule Id="SA1008" Action="None" />
    <Rule Id="SA1009" Action="None" />
    <Rule Id="SA1027" Action="None" />
    <Rule Id="SA1101" Action="None" />
    <Rule Id="SA1111" Action="None" />
    <Rule Id="SA1116" Action="None" />
    <Rule Id="SA1117" Action="None" />
    <Rule Id="SA1118" Action="None" />
    <Rule Id="SA1122" Action="None" />
    <Rule Id="SA1124" Action="Info" />
    <Rule Id="SA1133" Action="None" />
    <Rule Id="SA1134" Action="None" />
    <Rule Id="SA1200" Action="None" />
    <Rule Id="SA1201" Action="None" />
    <Rule Id="SA1202" Action="Info" />
    <Rule Id="SA1204" Action="None" />
    <Rule Id="SA1303" Action="None" />
    <Rule Id="SA1308" Action="None" />
    <Rule Id="SA1310" Action="None" />
    <Rule Id="SA1311" Action="None" />
    <Rule Id="SA1402" Action="None" />
    <Rule Id="SA1407" Action="None" />
    <Rule Id="SA1503" Action="None" />
    <Rule Id="SA1516" Action="None" />
    <Rule Id="SA1604" Action="None" />
    <Rule Id="SA1611" Action="None" />
    <Rule Id="SA1614" Action="None" />
    <Rule Id="SA1615" Action="None" />
    <Rule Id="SA1618" Action="None" />
    <Rule Id="SA1623" Action="None" />
    <Rule Id="SA1633" Action="None" />
    <Rule Id="SA1642" Action="None" />
    <Rule Id="SA1643" Action="None" />
    <Rule Id="SA1652" Action="None" />
    <Rule Id="SX1101" Action="Warning" />
  </Rules>
</RuleSet>

SortedDictionary<>, SortedList<>, and SortedSet<> have constructors that take an IComparer<string>.

.OrderBy(x => x.Prop) where Prop is a string will use a culture-sensitive comparison. One should use .OrderBy(x => x.Prop, StringComparer.Ordinal) instead (or explicitly specify a culture-sensitive comparer if that is desired).

It should be an error to use WorkState.None (or WorkState.ToDo) when an IWorkState is in scope:

• IWorkState method argument
• AsyncMethodContext method argument (use context.WorkState)
• CancellationToken (use WorkState.FromCancellationToken)
• IEnumerable<AsyncAction> method (use AsyncWorkItem.Current)

JavaScript template literals use ${variable} inside the string to interpolate a variable. C# interpolated strings use $"...{variable}...".

Because of the similarity, programmers who use both languages may mistakenly write $"...${variable}...", which "works", but adds an extra dollar sign into the content of the string. Because putting a literal $ immediately before an interpolated variable should be extremely uncommon (if you were actually formatting currency you should probably be using a currency-specific symbol, not a literal $), all occurrences of ${ in an interpolated string should be flagged as an error.

For methods in which an IDbConnection is in scope, no methods that create IDbConnections should be called.

Performing this analysis efficiently may pose some interesting problems.

These methods are thoroughly unnecessary in modern code, but new usages continue to be written.

We should write code fix providers for the most common usage patterns.

Calling (Dispatcher.)VerifyAccess() (typically through NotifyPropertyChangedDispatcherBase) from a non dispatcher-affined thread (typically, any threadpool thread) causes an application crash. Yet, this is a very common developer mistake that results in wasted time at the very least.

Detecting the dispatcher context of calls to VerifyAccess seems possible, though computationally intensive. Doing a fixed N-level deep search on code in the scope of a switch to the threadpool seems like it might be less computationally expensive, and still provide value.

However, perhaps the biggest challenge would be to resolve variant runtime behavior in order to properly detect invalid code. e.g.

context.Switch(await context.ToThreadPool());
if (x) context.Switch(await context.ToSyncContext());
if (y) VerifyAccess();

With the same rationale as our internal wiki. Possibly related to #36

FL0006 reports an error when OrderBy is used on a string, but not when it is used on a tuple that includes a string, even though the string will be sorted with the current culture.

A fixer would be challenging, but it still might be worth reporting as an issue.

There are likely to be existing analyzers or products that perform checks we would like to enforce at Faithlife. For example, I believe ReSharper gives a warning or suggestion about #35 already, and there may be an existing analyzer on NuGet that checks for a number of common Task-related pitfalls.

Should we:

• recommend that existing analyzer packages be installed, and customized for only the rules we want
• bundle an existing analyzer (as a dependency of Faithlife.Analyzers somehow?) and delegate certain rules to it
• copy/paste the code into Faithlife.Analyzers (license permitting)
• some combination of all of the above, depending on the situation?

For example, it seems pointless to try to duplicate StyleCop; our approach should be to provide the right ruleset then get projects to enable it (by including it as another analyzer).

But for one-off rules, maybe we don't want to go down a path of enabling dozens of different analyzers for just a couple of checks provided by each one. What's the right approach?

The patterns

foo != null ? foo.Bar.OtherStuff : null

and

foo.HasValue ? foo.Value.Bar.OtherStuff : null // foo is of type Nullable<T>

Should be simplified to

foo?.Bar.OtherStuff

Unused usings should trigger build errors.

I suspect that resolving #36 will also resolve this issue.

We should ensure that all calls to new ArgumentException and new ArgumentNullException use one of the overloads that accept a parameter name.

Additionally, we should verify that this value is a compile-time-resolvable constant that matches one of the argument names.

Code fix providers: maybe one for each of the arguments that supplies nameof(argumentName) in the appropriate position? Other than that, I don't know of automatic fixes that could be provided.

We might also provide a code fix provider that finds constant strings and turns them into nameof operator usages, although this would probably be need to be flagged at a level less than a warning, unless we want to introduce a lot of noise to older projects.

We could presumably use Jekyll to generate the table of contents to avoid title duplication. Perhaps we could generate "see also" link titles as well.

A couple of possible variations:

When an IDbTransaction is in scope, find all methods that could be called with the IDbTransaction (but aren't) and flag them as errors. Create a code fix provider that calls the appropriate overload.

When an IDbTransaction is in scope, find all methods that accept an IDbConnection, and flag them as errors if the IDbTransaction is not passed to them. Create one code fix provider that attempts to call an appropriate overload, and another code fix provider that adds a parameter to the target method if there isn't an appropriate overload.

Split from #15:

An intermediate step might be to enforce that .ToReadOnlyCollection() may not be used to initialize fields or properties in constructors and that .ToList().AsReadOnly() must be used instead. (This addresses the buggiest and least frequent usage.)

Perhaps there should be multiple fixers, as many as one for each of StringComparer.Ordinal, StringComparer.InvariantCulture, and/or StringComparer.CurrentCulture.

Probably not worth trying to come up with a fixer for orderby usage, since partially changing LINQ syntax format to LINQ method format (the only way to specify the comparer) can't be trivial.

When comparing to 0, .Any() should be preferred.

When comparing to any other value, EnumerableUtility.CountIsExactly or EnumerableUtility.CountIsAtLeast should be used.

If a struct consists entirely of readonly fields or read-only auto properties, suggest making it a readonly struct.

When creating an ArgumentException with a message parameter (and all calls should use that parameter, right?), the phrase "must not" should be preferred over "cannot" and "can not".

Presumably, this could be found by searching for the Regex @"\bcan ?not\b" (culture invariant, case insensitive) in the text of ArgumentExceptions and offering a code fix provider that changes the affected text to "must not".

We have written code of the form object.ReferenceEquals(x, null) to check if an object is null and deliberately bypass an operator== implementation. (This can be important to avoid infinite recursion in custom Equals implementations, assuming operator== simply delegates to Equals.)

This should be simplified to x is null.

The inverse check !object.ReferenceEquals(x, null) can be simplified to !(x is null) (straightforward) or x is object (less obvious, but easier to search-and-replace with a better replacement possibly coming in C# 8.0).

yield return AsyncActions.WaitForTask does not automatically propagate exceptions from the waited-for Task. Although in some cases, the Task is saved in a local variable and its .Result property is accessed (which will throw any saved exceptions), for consistency it seems better to never allow yield return AsyncActions.WaitForTask but instead save the action in a local variable, yield return that, then call .Verify or .Result on the action.

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.

Catch some of the common patterns described in our internal wiki page and flag them.

When a method is called that has an overload accepting an IFormatInfo, flag it as an error if this overload is not used.

Code fixes:

• Supply CultureInfo.InvariantCulture
• If a FormatInfo is in scope, supply that
• For ToString calls, if Libronix.Utility or Faithlife.Utility is referenced by the current project and an appropriate member of InvariantConvert is available, switch to it.