Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add overload to Diagnostic Analyzer template to support extra MetadataReferences #2369

Open
conniey opened this issue Apr 29, 2015 · 8 comments
Labels
Area-Analyzers help wanted The issue is "up for grabs" - add a comment if you are interested in working on it Test Test failures in roslyn-CI
Milestone

Comments

@conniey
Copy link
Member

conniey commented Apr 29, 2015

My team has been writing some Roslyn diagnostic analyzers and all the new developers on our team have encountered this situation, so I thought I'd bring up this suggestion.

The Roslyn analyzers template auto-generates a test project along with verifiers. By default, they only compile the test source code with 4 assemblies (ie. mscorlib, System.Core, etc).
We found ourselves in a situation where any symbols returned from the SyntaxTree is null and we were not sure why because the logic in the analyser looked good. We realised the null was because it wasn’t able to compile the test source code that we were using due to references from other assemblies like ComposablePartsCatalog in System.Composition.ComponentModel.dll.

We had to add an overload to DiagnosticVerifier.Helper.cs to accept more metadata references.

I think any developer who wants to use a class other than those 4 will find themselves confused and spend time debugging just to come to the same conclusion as we did.

This is the code we added to the existing template code in DiagnosticVerifier.Helper.cs.

        private static Diagnostic[] GetSortedDiagnostics(string[] sources, string language, params MetadataReference[] additionalReferences)
        { }
...
        /// <summary>
        /// Create a project using the inputted strings as sources.
        /// </summary>
        /// <param name="sources">Classes in the form of strings</param>
        /// <param name="language">The language the source code is in</param>
        /// <returns>A Project created out of the Documents created from the source strings</returns>
        private static Project CreateProject(string[] sources, string language = LanguageNames.CSharp, params MetadataReference[] additionalReferences)
        {
            string fileNamePrefix = DefaultFilePathPrefix;
            string fileExt = language == LanguageNames.CSharp ? CSharpDefaultFileExt : VisualBasicDefaultExt;

            var projectId = ProjectId.CreateNewId(debugName: TestProjectName);

            var solution = new AdhocWorkspace()
                .CurrentSolution
                .AddProject(projectId, TestProjectName, TestProjectName, language)
                .AddMetadataReference(projectId, CorlibReference)
                .AddMetadataReference(projectId, SystemCoreReference)
                .AddMetadataReference(projectId, CSharpSymbolsReference)
                .AddMetadataReference(projectId, CodeAnalysisReference)
                .AddMetadataReferences(projectId, additionalReferences);

            int count = 0;
            foreach (var source in sources)
            {
                var newFileName = fileNamePrefix + count + "." + fileExt;
                var documentId = DocumentId.CreateNewId(projectId, debugName: newFileName);
                solution = solution.AddDocument(documentId, newFileName, SourceText.From(source));
                count++;
            }
            return solution.GetProject(projectId);
        }
@bkoelman
Copy link
Contributor

bkoelman commented May 2, 2015

Yes, I have run into this too. And patched the template code likewise. Besides this, additional metadata references should be supported in the CodeFixVerifier.

@bkoelman
Copy link
Contributor

bkoelman commented May 2, 2015

Something else that would be nice: being able to specify a custom file name for running the analyzer and fix provider. I'm working on an analyzer that skips generated code files like Form1.Designer.cs. Such files are generated by VS without [GeneratedCode] attributes.

@sharwell
Copy link
Member

sharwell commented May 3, 2015

@bkoelman In the StyleCopAnalyzers project, we don't look for GeneratedCodeAttribute at all; it's just not worth the performance hit. We suppress analysis only for known designer files, generated backing files created by the XAML compiler, and files containing <auto-generated in a comment at the top of the file. See DotNetAnalyzers/StyleCopAnalyzers#670 for more information.

@bkoelman
Copy link
Contributor

bkoelman commented May 5, 2015

@sharwell Good to know about performance, will keep that in mind. Interesting to scan comments at top-of-file also. That pull request does not contain unit tests though. Wouldn't you need my request for that?

@sharwell
Copy link
Member

sharwell commented May 5, 2015

@bkoelman I don't think I understood your question. Can you rephrase it?

@bkoelman
Copy link
Contributor

bkoelman commented May 5, 2015

@sharwell Correct me if I've missed something, but the StyleCop source seems to contain checks based on comment and filename (HasAutoGeneratedComment() and IsGeneratedFileName() in https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/d10db47a8777dc882ef196d78692a1ece67bbb1d/StyleCop.Analyzers/StyleCop.Analyzers/GeneratedCodeAnalysisExtensions.cs.

Furthermore, I see a unittest that verifies comments (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/aa3d45dd97e72e3d15dcd4ac62cf16145cc31e60/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1600UnitTests.cs), but not one that verifies behavior based on filename.

Just like I did, you would need to replace/rewrite parts of the generated template code for VerifyCSharpDiagnostic() that analyzers come with by default:

namespace TestHelper
{
    public abstract partial class DiagnosticVerifier
    {
        private static Document[] GetDocuments(string[] sources, string language)
        {
            // ...

            for (int i = 0; i < sources.Length; i++)
            {
                string fileName = language == LanguageNames.CSharp ? "Test" + i + ".cs" : "Test" + i + ".vb";
            }

So my question is: how would you write that missing unit test? To me it seems you are missing the same feature (filename support) that I am missing currently in the generated template code.

@srivatsn srivatsn added the Test Test failures in roslyn-CI label May 15, 2015
@srivatsn srivatsn added this to the Unknown milestone May 15, 2015
@srivatsn srivatsn added the help wanted The issue is "up for grabs" - add a comment if you are interested in working on it label May 15, 2015
@Evangelink
Copy link
Member

I am wondering whether this issue is still relevant with the new test framework?

@Youssef1313
Copy link
Member

I am wondering whether this issue is still relevant with the new test framework?

@sharwell Is this still relevant? If it's, I think it will belong to roslyn-sdk repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers help wanted The issue is "up for grabs" - add a comment if you are interested in working on it Test Test failures in roslyn-CI
Projects
None yet
Development

No branches or pull requests

7 participants