sergeyteplyakov / errorprone.net Goto Github PK

Technically it would be good to proof non-nullability of the result, but even warning on return null for TAP would be good.

Potentially the following code could be unsafe:

using System;
public class C {
    public void M() {
        int n = 42;
        Foo(n);
        Action a = () => {Console.WriteLine(n); n++;};
        a();
    }
    
    public void Foo(in int x) {}
}

If the Foo method is slow and a delegate is called from a different thread, the argument observed in Foo may be different at different points in time.

Even though it was always possible to do the same by passing an argument by ref.

Automate the build using appveyor.
Automate publishing new prerelease version when the build is successful in master.

Warn if the method is not presented or could not be found, or return value is not string etc.

For instance, in new ArgumentException("argName") to switch to new ArgumentException(nameof(arg)).

Explanation, msdn

If the method that calls the GetCallingAssembly method is expanded inline by the just-in-time (JIT) compiler, or if its caller is expanded inline, the assembly that is returned by GetCallingAssembly may differ unexpectedly. For example, consider the following methods and assemblies:

• Method M1 in assembly A1 calls GetCallingAssembly.
• Method M2 in assembly A2 calls M1.
• Method M3 in assembly A3 calls M2.

When M1 is not inlined, GetCallingAssembly returns A2. When M1 is inlined, GetCallingAssembly returns A3. Similarly, when M2 is not inlined, GetCallingAssembly returns A2. When M2 is inlined, GetCallingAssembly returns A3.
This effect also occurs when M1 executes as a tail call from M2, or when M2 executes as a tail call from M3. You can prevent the JIT compiler from inlining the method that calls GetCallingAssembly, by applying the MethodImplAttribute attribute with the MethodImplOptions.NoInlining flag, but there is no similar mechanism for preventing tail calls.

Actually there's undocumented trick in the BCL to prevent the behavior described above. Hovewer, I'd prefer to have a warning for code like this.

As far as I can remember the same error is possible when calling the MethodBase.GetCurrentMethod(), calling the StackTrace() constructor with skipFrames arg set, calling the GetExecutingAssembly() and so on.

As a sidenote: the project should be definitely added into awesome-analysers list.

Calling string.Clone() is never ever useful. Nevertheless DevDiv has 19 references to it :)

Consider the following case:

public void Run(Action<int> a) {
  a();
}

try
{
  Run(async n => Parse(n));
}
catch(Exception) {}

This method will crash the app. This issue is very hard to spot.

The compiler knows about the nullable types are immutable and causes no hidden copies for them.

Real-world typo:

var newGuid = new Guid(); // Guid.NewGuid(); assumed

Add an attribute NoHeapAllocation and AllowHeapAllocation.

User can put an attribute on assembly, class or member level. If the attribute is present than the member should follow some heap allocation rule. If the rule is saying that allocations are prohibited, analyzer will warn an any heap allocations.

See controlflow stuff as an example here: https://github.com/controlflow/resharper-heapview

This is the case:

[DebuggerDisplay("{Foo,nq}")
class FooBar {
  private string Foo() {return string.Empty;}
}

In this case, the warning should be different and it should be covered by the memory analyzer.

Especially for properties to itself!

Consider following code:

private static async Task<string> ReadFileContentAsync(Path spec)
        {
            using (var reader = File.OpenText(spec.AbsolutePath))
            {
                return await reader.ReadToEndAsync();
            }
        }

In this case, removing await and async will change the behavior of the code!

So the rule should respect this.

In many cases I need init-once readonlyness, when instance can be instantiated using object-initialization syntax with inability to change properties after construction.

It could sounds very simple and unlikely to happen, but I just got the situation when I've assigned property to itself!

And as far as I know, there was even a bug in Roslyn for self-assignment for arguments.

Code is in attachment. I can't find any hidden copies in IL.

HiddenCopy.zip

The idea is to add design time analyzer that will duplicate C# warning for unused fields in design time.

Roslyn team members mentioned that design time implementation is too time consuming and this could be correct, but anyway I would like to give it a try.

Current impementation of the CS0169 is following:

• Warn on private fields if they're not used in the class (considering all partial classes)
• Warn on protected fields if class is not public or when class is internal but no InternalVisibleToAttribute defiend for the assembly

This is controversial check and maybe some other tool should check this.

But I found very useful for all boolean arguments have names on the invocation side.

Here is an example, when the analyzer still could show the warning:

struct Immutable {
  private int _x;
  public Immutable(int x) => _x = x;
}

But in this case, the field should be made readonly as well.

Tasks in many cases are pure. I.e. if the await expression returns something this should be a warning.

Some examples could be found here: https://github.com/ljw1004/blog/blob/master/Analyzers/MissingAwaitAnalyzer/MissingAwaitAnalyzer/readme.md

Consider following case:

public async Task<object> Foo() {
  return await Task.FromResult("foo");
}

Removing async and await like this:

public Task<object> Foo() {
  return Task.FromResult("foo");
}

Will lead to an invalid code!

Example:

public async Task<T> Get<T>()
{
    return await InnerGet<T>();
}

I think it should be okay to copy small (64-bits) struct, shoudn't it?

This method lead to additional allocation without no clear benefits.

Catch errors for potentially wrong equals/gethashcode.

First: mismatch between gethashcode and equals. Second, if something looks like a part of the state, just warn if that part is not used in the equality. Fixer can fix this stuff (maybe).

With a fixer to use a bit-wise comparison.

Enumerators for BCL collections (and any struct-based iterators)
SpinLock

Analyze the BCL to find other instances.

• Warn on async void
• Warn on Task.Result/Task.Wait prefer Task.GetAwaiter().GetResult because later will unwrap exception. Question: how to catch correct cases?
• Warn on incorrect preconditions in async methods

Do to suggest: pass by in if the argument is reassigned. argument is used in an anonymous method.

Formatting issue with the fixer that adds 'in'. Need to carry trivia. Done
Do not suggest using in for task-based methods, not only for async methods. (it could be task-like thing, not only task!) Done.

Warn if 'in'-modifier is used for Task-based method. Done
Analyze indexers for 'use 'in' modifier. Done

Remove primitives from 'non-readonly struct 'int32' used as in-parameter.

Do not convert argument if it is used in out context.

Do not warn about 'non readonly struct is used as in' if the method overrides. Done

If the struct is stored in readonly field, result of the invocation should be on the left hand side! Otherwise it will not have any effects.

Void return method should be an exception, because it is hard to check purity of the method.

Warn if the result is not observed.

Otherwise the tool will notify that catch block swallows an exception even that it doesn't.

Sample: https://twitter.com/ViIvanov/status/753602621651509248

Analyzer methods with Pure attribute: warn if method changes the state.

One caveat here: technically speaking, pure method can change state if this change is invicible to the client. This is similar to physical and logical const-ness in C++. Logically, method could be const, even when it is changing the state.

THe same is true for pure methods (at least based on current semantic of the PureAttribute that is described in the official documentation). I.e. method that implements cache-aside pattern could be considered as a pure method.

Suggest to convert it to pass by 'in'.

new Excpetion without throw (DONE)

if (arg == null) {
  new ArgumentException("arg");
}

Exception propagation is invalid (DONE)

try {
}
catch(Exception ex) {
  throw ex; // should be throw;
}

Empty if (DONE by compiler)

if (foo); {
  Console.WriteLine("foo is true");
}

Self equals (DONE by compiler)

if (a == a) {
}
// The same for equals, object.Equals

Self assignment (DONE by compiler)

var x = 42;
x = x;

Check return value for pure methods (DONE)

Enumerable.Range(1, 10);

This check can rely on simple knowledge what methods are pure (string, enumerable, delegate, immutable collections, domino collections).

ToString is silly

Emit a warning for calls ToString on collections. Fix could introduce string.Join(", ", collection).

Equality on collections is silly

Emit a warning for calling Equals on collections... Is it possible?

Malformed string format (DONE)

This one should be easy. Fix could be to switch to interpolated string!
Add custom attribute (use ducktyping and R# attributes) to warn on those method invocations as well!!

Missing case in enum switch (DONE)

NoAllocation attribute

This could be very fun rule to implement!

Idea: http://errorprone.info/bugpattern/NoAllocation

LINQ rules

Use Count() instead of Count? Count() != 0 instead of Any()?

Non-pure method on readonly mutable struct

Problem: how to check that the method is not-pure? Void-returned?

No Exception swallowing (DONE)

This could be an attribute on the assembly and if its defined, than my exception rule will work and break the build!

The Roslyn analyzer is primarily uses during development, so the default package reference should be privateAssets="all", e.g.

    <PackageReference Include="ErrorProne.NET.Structs" Version="0.1.0" PrivateAssets="all" />

This will remove the package dependency from any generated nuget package.