dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.07k stars 4.04k forks source link

Running analysis on CSharpScript compilation has unexpected warnings regarding unused fields #37681

Open watfordgnf opened 5 years ago

watfordgnf commented 5 years ago

Background We have an old application which used CodeDom to execute user scripts. We're migrating that application to Roslyn. It would be nice to include many of our code review rules as Roslyn Analyzers, however, it looks like semantic analysis on scripts isn't quite what we'd expect.

Version Used: Microsoft.CodeAnalysis.CSharp.Scripting 3.2.0 Microsoft.CodeAnalysis.FxCopAnalyzers 2.9.4

Steps to Reproduce:

  1. Create a CSharpScript from the following:
    double x = 23.0;
    var y = new[]{ 1, 2, 3 };
    var b = Sin(x);
    Write(""stdout"", x);
    WriteLine(""stdout"", b);
    Write(""stdout"", y);
  2. Get the compilation from the script and run FxCopAnalyzers on it.
  3. Review the diagnostics.

This gist has the full code used.

Expected Behavior: y and b are not listed as unused fields. They are used in the script.

Actual Behavior: Any variable declared var Variables only used by the global type are always unused: Variables passed to methods that are void-returning or whose results are ignored are marked as unused. The same code in a non-script does not have the same warnings.

(2,5): warning CA1823: Unused field 'y'.
(3,5): warning CA1823: Unused field 'b'.

Other behavior seen when I include StyleCop.Analyzers (this makes sense):

(1,8): warning SA1400: Element 'x' should declare an access modifier
(2,5): warning SA1400: Element 'y' should declare an access modifier
(3,5): warning SA1400: Element 'b' should declare an access modifier
watfordgnf commented 5 years ago

Red herring: var. Expanding the script to be:

double x = 23.0;
var y = new[]{ 1, 2, 3 };
var b = Sin(x);
var c = Cos(y[0]);
var d = b + c;
double e = c;
Write("stdout", x);
WriteLine("stdout", b);
Write("stdout", y);
Write("stdout", d);
Write("stdout", e);

d and e are reported as unused. Making a private copy of AvoidUnusedPrivateFieldsAnalyzer I was able to see that the following never fires for any operation using members of the global type passed to the script:

                    compilationContext.RegisterOperationAction(
                        (operationContext) =>
                        {
                            IFieldSymbol field = ((IFieldReferenceOperation)operationContext.Operation).Field;
                            if (field.DeclaredAccessibility == Accessibility.Private)
                            {
                                referencedPrivateFields.TryAdd(field, default);
                                maybeUnreferencedPrivateFields.TryRemove(field, out _);
                            }
                        },
                        OperationKind.FieldReference);

https://github.com/dotnet/roslyn-analyzers/blob/c9d6579f31fd4ad45f3d23187189657343b395a4/src/Microsoft.CodeQuality.Analyzers/Core/Maintainability/AvoidUnusedPrivateFields.cs#L103-L113

So I presume this means that analysis is "stopped" by the global type? I'll try and do some more digging into why the global type stops this (or what 'stop' even means in this context).

watfordgnf commented 5 years ago

e is unused in the following:

double e = 1.0;
Write("stdout", Sin(e));

But if the globals type returns a value, that does not appear to cause any problems:

double dbl = 0.0;
string str = String.Empty;
int integer = 0;

int a = Test(dbl); // int Test(double _) => 0;
object b = Test((object)dbl); // object Test(object o) => o;
string c = Test(str); // T Test<T>(T t) => t;
Test(integer); // void Test(int a) { }

In this case only integer is listed as unused. But if we change that script slightly:

double dbl = 0.0;
string str = String.Empty;
int integer = 0;

Test(dbl); // int Test(double _) => 0;
Test((object)dbl); // object Test(object o) => o;
Test(str); // T Test<T>(T t) => t;
Test(integer); // void Test(int a) { }

...then in this case dbl, str, and integer are all "unused". Even adding a discard pattern does not allow them to be marked as used:

_ = Test(str); // `str` is still unused
watfordgnf commented 5 years ago

Another red herring: the globals type. Given:

double dbl = 0.0;
string str = String.Empty;
int integer = 0;

Sin(dbl);
Abs(integer);
String.Compare(str, String.Empty);

All three are again listed as unused.

The method can even be local to the script:

double x = 0.0;
double Testing(double _) => 0.0;
void Testing2(double x) => Testing(x);
Testing(x);
Testing2(x);
// (1,8): warning CA1823: Unused field 'x'
mavasani commented 5 years ago

Tagging @tmat @AlekseyTs @333fred

I don't believe this has scenario been tested before and would potentially need some initial investigation + design/feature work.

watfordgnf commented 5 years ago

I tried investigating this, but I'm not savvy enough with the internals of Roslyn to know where to begin.

I attempted disabling the rules with a ruleset applied to the compilation, but that didn't work either.

tmat commented 5 years ago

As @mavasani says we don't currently support analyzers running on scripts.

watfordgnf commented 5 years ago

Where would one start to add support to these situations? That's what I had the most difficulty tracking down.