faithlife / faithlifeanalyzers Goto Github PK
View Code? Open in Web Editor NEWC# code analyzers
License: MIT License
C# code analyzers
License: MIT License
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.
Prefer ""
over String.Empty
(or string.Empty
).
Array
and List<T>
have Sort
and BinarySearch
methods that take an IComparer<string>
.
@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.
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 objectList<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 constructorIEnumerable<T>
: calls .ToList().AsReadOnly()
and gets a new immutable listFurthermore, 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.)
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.
If a struct
consists entirely of readonly
fields or read-only auto properties, suggest making it a readonly struct
.
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).
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.
Task<T>.GetAwaiter().GetResult()
should be preferred over Task<T>.Result
, and (void) Task.GetAwaiter().GetResult()
over Task.Wait()
.
.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).
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();
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.
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".
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.
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
For methods in which an IDbConnection
is in scope, no methods that create IDbConnection
s should be called.
Performing this analysis efficiently may pose some interesting problems.
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.
In the following code, the standard AsyncWorkItem.Current
fixes should be offered, but they aren't:
if (AsyncWorkItem.Current.Canceled)
{
// ...
}
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:
if
condition, using
declaration, etc.) is split onto multiple lines, include braces.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.
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:
Dto
suffixReadOnlyCollection
/ IReadOnlyCollection
of any of the aboveReadOnlyDictionary
IReadOnlyDictionary
from string to any of the above.These will definitely need to be warnings, given the number of historical violations that we have.
It should be an error to use WorkState.None
(or WorkState.ToDo
) when an IWorkState
is in scope:
IWorkState
method argumentAsyncMethodContext
method argument (use context.WorkState
)CancellationToken
(use WorkState.FromCancellationToken
)IEnumerable<AsyncAction>
method (use AsyncWorkItem.Current
)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:
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>
Similar to CurrentAsyncWorkItemAnalyzer
, use of Libronix.Utility.Threading.AsyncEnumerableUtility.UntilCanceled()
should be disallowed outside of IEnumerable<AsyncAction>
methods.
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:
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?
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.
Catch some of the common patterns described in our internal wiki page and flag them.
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.
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
.
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.
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)
SortedDictionary<>
, SortedList<>
, and SortedSet<>
have constructors that take an IComparer<string>
.
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()
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>
.
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.
With the same rationale as our internal wiki. Possibly related to #36
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).
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.)
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.
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.
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.
When comparing to 0, .Any()
should be preferred.
When comparing to any other value, EnumerableUtility.CountIsExactly
or EnumerableUtility.CountIsAtLeast
should be used.
// unnecessarily verbose
string helloWorld = @"hello world";
string helloWorld = $"hello world";
string helloWorld = $@"hello world";
// preferred
string helloWorld = "hello world";
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.
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:
CultureInfo.InvariantCulture
FormatInfo
is in scope, supply thatToString
calls, if Libronix.Utility
or Faithlife.Utility
is referenced by the current project and an appropriate member of InvariantConvert
is available, switch to it.We never cloned the awaitable duck-typing interface of DispatcherOperation
in our port of it to Mac. Writing code that await
s 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:
BeginInvoke(IfNecessary)
and ignore the result if fire-and-forget is sufficient.AsyncMethodContext
or AsyncAction
s context is available.Invoke
to synchronously execute code on the Dispatcher
's context in a cross-platform compatible way.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);
}
Unused usings should trigger build errors.
I suspect that resolving #36 will also resolve this issue.
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.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.