dotnet / roslyn-analyzers Goto Github PK

Consider (from DiagnosticAnalyzer.cs):

public const string RuleCategory = "Tutorial";

RuleCategory is only used within MetaCompilationAnalyzer, and as such it should be private. This goes for any member of a type. Also, in this particular case you'll need to change the name to match the coding conventions for private const/static members. So you'll end up with this:

private const string s_ruleCategory = "Tutorial";

Principle: Types and members should only be as accessible as they need to be.

Add a one paragraph description of the MetaCompilation analyzer to README.md

Don't force (IfStatementSyntax), allow as IfStatementSyntax

Rewrite the messages so that there are no ...s and they are consistent and allow people to not use the code fixes. Make the squiggled thing the subject.

• "identifying" -> "distinguishing", id message

Review uses of SyntaxFactory, and when applicable, replace them with SyntaxGenerator uses instead

id: "Fix me" (or something like that)
defaultSeverity: default(DiagnosticSeverity)
isEnabledByDefault: default(bool)

Move the code fix from SyntaxNodeAnalyzer into the blank template, with an explanatory comment

Have most code fixes come with a comment
Sync with template
Comments above rather than below

• Register comment, "IfStatementSyntax" -> "IfStatement"
• "intergers" -> "integers"
• red squiggle is quoted

This includes moving projects under the following paths in the dotnet/roslyn repo:

• src\Samples\CSharp\Analyzers
• src\Samples\VisualBasic\Analyzers

Sync up all the code fixes written by different people so they follow the same format. Use SyntaxGenerator instead of SyntaxFactory, separate into two methods the creation of the new node and the insertion of that node, and move the creation of the node to the CodeFixNodeCreator class

• inserting context.Register needs AnalyzeIfStatement comment inserted
• inserting AnalyzeIfStatement needs AnalyzeIfStatement comment inserted
• incorrect parameter needs AnalyzeIfStatement comment inserted

Look at flattening code in IfStatementAnalysis methods

Allow the user to take advantage of messageFormat being a format string if they want to

This includes the following:

• Porting BuildAndTest.proj from dotnet/roslyn
• Creating the appropriate cibuild.cmd/cibuild.sh scripts
• Setting up Jenkins to work with this repo
• Displaying Jenkins status badges on the home page

ReportDiagnostic(context, IncorrectAccessorReturnRule, returnDeclaration.GetLocation());

vs.

ReportDiagnostic(context, IncorrectAccessorReturnRule, returnDeclaration.GetLocation(), IncorrectAccessorReturnRule.MessageFormat);

includes in the user end product

"IfSpacing" -> "IfSpacing001"

In particular, note how to get started with the project (downloading NuGet packages, building, etc.).

This probably involves creating a separate project that just runs packaging for everything after Analyzers.sln is built, or maybe giving each sub-project (Microsoft.CodeAnalysis.Analyzers, AnalyzerPowerPack, etc.) it's own packaging project. We should consider using NuProj as well.

The trivia checks should be as follows:

if (ifKeyword.HasTrailingTrivia) 
{
    if (ifKeyword.TrailingTrivia.Count == 1)
    {
        var trailingTrivia = ifKeyword.TrailingTrivia.First();

add .Count ==1 and change .Last() to .First()

Move the Initialize step later in the tutorial

Create .targets files to be used by all projects in the repo.

Need to setup dotnetbot for CLA review

By and large we should copy the set of labels used by dotnet/roslyn:

• 0 - Backlog (#CCCCCC)
• 1 - Planning (#CCCCCC)
• 2 - Ready (#CCCCCC)
• 3 - Working (#CCCCCC)
• 4 - In Review (#CCCCCC)
• Area-Infrastructure (#5319e7)
• Area-MetaCompilation (#5319e7)
• Blocked (#6400)
• Bug (#fc2929)
• Discussion (#fbca04)
• Enhancement (#fc2929)
• Feature Request (#fc2929)
• Localization (#fc2929)
• Need More Info (#fbca04)
• Question (#fc2929)
• Resolution-Answered (#CCCCCC)
• Resolution-By Design (#CCCCCC)
• Resolution-Duplicate (#CCCCCC)
• Resolution-External (#CCCCCC)
• Resolution-Fixed (#CCCCCC)
• Resolution-Not Applicable (#CCCCCC)
• Resolution-Not Reproducible (#CCCCCC)
• Resolution-Won't Fix (#CCCCCC)
• Test (#fc2929)
• Up for Grabs (#90EE90)
• Verification Not Required (#6400)
• Verified (#6400)
• cla-already-signed (#009800)
• cla-not-required (#90EE90)
• cla-required (#e11d21)
• cla-signed (#207de5)
• Update existing issues with tags

This includes moving the following projects in the dotnet/roslyn repo:

• BasicCodeAnalysisDiagnosticAnalyzers
• CodeAnalysisDiagnosticAnalyzers
• CodeAnalysisDiagnosticAnalyzersTest
• CodeAnalysisDiagnosticsSetup
• CSharpCodeAnalysisDiagnosticAnalyzers

I see a fair number of places where ToString() is being called to get a string representation of an object which is then compared to some known/expected value.

The contract of the object.ToString method is very loose: it must simply return some string representation of the object. That string representation may not be what you want or expect, and in many cases it isn't documented at all. For example, calling ToString() on a SyntaxNode may return a string that preserves the original formatting in the document, or it may not.

For that reason you should generally only use ToString for debugging or logging purposes, and all other uses should be replaced with something else.

This involves moving the projects under src\Samples\CSharp\AsyncPackage in the dotnet/roslyn repo.

Make sure the links in the ReadMe are ok to use

• use ``` to insert code snippets
• links should be new lines, text should be links
• link to how to recreate the syntax tree image
• "underline" -> "squiggle"
• "produce live warnings" -> "produce live diagnostics"
• when explaining what the tutorial creates, it should allow for no trivia to be present
• analyzer information section is wordy
• link to Alex Turner's blog
• verify accuracy of current links
• don't discourage use of code fixes
• open a new file for debugging should tell them to open a console app
• walk through the steps of making an analyzer, per Kasey's suggestion
• crop image

Move to consuming the 1.0.0-rc3 Roslyn binaries. This implies devs moving to a new build of VS.

This includes moving the following projects in the dotnet/roslyn repo:

• AnalyzerPowerPack.Common
• AnalyzerPowerPack.CSharp
• AnalyzerPowerPack.Setup
• AnalyzerPowerPack.Test
• AnalyzerPowerPack.VisualBasic

I see a number of diagnostics where the title and message are the same.

Diagnostic titles should be very short and general, as they describe the diagnostic in general. Messages, on the other hand, can and should be customized to include information specific to a particular instance of a diagnostic.

For example, if your diagnostic's title is

Diagnostic ID has not been declared

then the accompanying message should be

Diagnotic ID '{0}' has not been declared

where {0} is a placeholder for the actual ID. Later, when calling Diagnostic.Create you can pass in the actual ID and the placeholder will be filled in.

Enable full strong-name and Authenticode signing of binaries.

For example (from DiagnosticAnalyzer.cs):

internal List<object> CheckInitialize(CompilationAnalysisContext context)

This method always returns the same three things: an IMethodSymbol representing the Register method, a List<ISymbol> representing the arguments, and (sometimes) an InvocationExpressionSyntax for the method call. By putting them into a List<object> to return them, you're losing that type information. There are at least two problems with this:

1. You aren't documenting what CheckInitialize really returns; instead, the caller needs to somehow know that the first item in the list is an IMethodSymbol, the second is a List<object>, etc.
2. If you accidentally change the set of items returned, or return them in the wrong order, the compiler can't help you check that the caller is still using them in the correct way. For example, if the IMethodSymbol accidentally became the second item in the list, you wouldn't know until you run the code that something was broken.

The fix here is to create a helper type to hold these three pieces of information. E.g.,

class InitializeInfo
{
    public IMethodSymbol RegisterMethod { get; }
    public List<ISymbol> RegisterArgs { get; }
    public InvocationExpressionSyntax InvocationExpression { get; }

    public InitializeInfo(IMethodSymbol registerMethod, List<ISymbol> registerArgs, InvocationExpressionSyntax InvocationExpression = null;
    {
        RegisterMethod = registerMethod;
        RegisterArgs = registerArgs;
        InvocationExpression = invocationExpression;
    }
}

Principle: you should avoid using object whenever possible. In practice, you always have more information about type of an object, and by being as specific as possible the compiler can help catch errors.

Check the count of TrailingTrivia in the final user product, and change .Last() to .First()

MSTest has issues with running unit tests involving Roslyn. Also, dotnet/roslyn uses xUnit instead of MSTest and it would be good to be consistent.

I've noticed a lot of methods in DiagnosticAnalyzer.cs that take a CompilationAnalysisContext as as argument, but then make no use of it (or pass it along to some other method that makes no use of it). These should be removed.

See, for example, a declaration of an empty protected instance constructor in the class Microsoft.CodeAnalysis.Location:

    public abstract class Location
    {
        protected Location()
        {
        }

        // ...

This declaration is redundant. If you remove it from source, the compiler generates exactly the same by default. So, it does not affect a shape of public API in any way.

But when I tried to remove it, the build failed with an error:

error RS0017: Symbol 'Microsoft.CodeAnalysis.Location.Location()' is part of the declared API, but is either not public or could not be found

EXPECTED: This error does not occur.

Force the user to use either Warning or Error

For example (from DiagnosticAnalyzer.cs), don't do this:

if (registerInfo == null)
{
    return;
}
var registerSymbol = (IMethodSymbol)registerInfo[0];

Instead, do this:

if (registerInfo == null)
{
    return;
}

var registerSymbol = (IMethodSymbol)registerInfo[0];

Principle: A block contains a set of related code. It should be visually distinct from the code that come after it, which is not related (at least, not in the same way).

• change "must" to "should" everywhere
• change "single whitespace" to "single space" everywhere

Create a separate diagnostic for if the id: is set to a string literal.