Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Diagnostics.CSharp;
using Microsoft.CodeAnalysis.Diagnostics.EngineV2;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Options;
using Microsoft.CodeAnalysis.Shared.TestHooks;
Expand Down Expand Up @@ -242,6 +244,43 @@ await service.SynchronizeWithBuildAsync(
// we should reach here without crashing
}

[Fact]
public void TestHostAnalyzerOrdering()
{
var workspace = new AdhocWorkspace(MefV1HostServices.Create(TestExportProvider.ExportProviderWithCSharpAndVisualBasic.AsExportProvider()));

var project = workspace.AddProject(
ProjectInfo.Create(
ProjectId.CreateNewId(),
VersionStamp.Create(),
"Dummy",
"Dummy",
LanguageNames.CSharp));

// create listener/service/analyzer
var listener = new AsynchronousOperationListener();
var service = new MyDiagnosticAnalyzerService(new DiagnosticAnalyzer[] {
new Priority20Analyzer(),
new Priority15Analyzer(),
new Priority10Analyzer(),
new Priority1Analyzer(),
new Priority0Analyzer(),
new CSharpCompilerDiagnosticAnalyzer(),
new Analyzer(),
}, listener, project.Language);

var incrementalAnalyzer = (DiagnosticIncrementalAnalyzer)service.CreateIncrementalAnalyzer(workspace);
var analyzers = incrementalAnalyzer.GetAnalyzersTestOnly(project).ToArray();

Assert.Equal(analyzers[0].GetType(), typeof(CSharpCompilerDiagnosticAnalyzer));
Assert.Equal(analyzers[1].GetType(), typeof(Analyzer));
Assert.Equal(analyzers[2].GetType(), typeof(Priority0Analyzer));
Assert.Equal(analyzers[3].GetType(), typeof(Priority1Analyzer));
Assert.Equal(analyzers[4].GetType(), typeof(Priority10Analyzer));
Assert.Equal(analyzers[5].GetType(), typeof(Priority15Analyzer));
Assert.Equal(analyzers[6].GetType(), typeof(Priority20Analyzer));
}

private static Document GetDocumentFromIncompleteProject(AdhocWorkspace workspace)
{
var project = workspace.AddProject(
Expand Down Expand Up @@ -291,10 +330,15 @@ private static async Task RunAllAnalysisAsync(IIncrementalAnalyzer analyzer, Doc
private class MyDiagnosticAnalyzerService : DiagnosticAnalyzerService
{
internal MyDiagnosticAnalyzerService(DiagnosticAnalyzer analyzer, IAsynchronousOperationListener listener, string language = LanguageNames.CSharp)
: this(SpecializedCollections.SingletonEnumerable(analyzer), listener, language)
{
}

internal MyDiagnosticAnalyzerService(IEnumerable<DiagnosticAnalyzer> analyzers, IAsynchronousOperationListener listener, string language = LanguageNames.CSharp)
: base(new HostAnalyzerManager(
ImmutableArray.Create<AnalyzerReference>(
new TestAnalyzerReferenceByLanguage(
ImmutableDictionary<string, ImmutableArray<DiagnosticAnalyzer>>.Empty.Add(language, ImmutableArray.Create(analyzer)))),
ImmutableDictionary<string, ImmutableArray<DiagnosticAnalyzer>>.Empty.Add(language, ImmutableArray.CreateRange(analyzers)))),
hostDiagnosticUpdateSource: null),
hostDiagnosticUpdateSource: null,
registrationService: new MockDiagnosticUpdateSourceRegistrationService(),
Expand Down Expand Up @@ -358,5 +402,58 @@ public override Task<ImmutableArray<Diagnostic>> AnalyzeSemanticsAsync(Document
return SpecializedTasks.Default<ImmutableArray<Diagnostic>>();
}
}

private class Priority20Analyzer : PriorityTestDocumentDiagnosticAnalyzer
{
public Priority20Analyzer() : base(priority: 20) { }
}

private class Priority15Analyzer : PriorityTestProjectDiagnosticAnalyzer
{
public Priority15Analyzer() : base(priority: 15) { }
}

private class Priority10Analyzer : PriorityTestDocumentDiagnosticAnalyzer
{
public Priority10Analyzer() : base(priority: 10) { }
}

private class Priority1Analyzer : PriorityTestProjectDiagnosticAnalyzer
{
public Priority1Analyzer() : base(priority: 1) { }
}

private class Priority0Analyzer : PriorityTestDocumentDiagnosticAnalyzer
{
public Priority0Analyzer() : base(priority: -1) { }
}

private class PriorityTestDocumentDiagnosticAnalyzer : DocumentDiagnosticAnalyzer
{
protected PriorityTestDocumentDiagnosticAnalyzer(int priority)
{
Priority = priority;
}

public override int Priority { get; }
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray<DiagnosticDescriptor>.Empty;
public override Task<ImmutableArray<Diagnostic>> AnalyzeSemanticsAsync(Document document, CancellationToken cancellationToken)
=> Task.FromResult(ImmutableArray<Diagnostic>.Empty);
public override Task<ImmutableArray<Diagnostic>> AnalyzeSyntaxAsync(Document document, CancellationToken cancellationToken)
=> Task.FromResult(ImmutableArray<Diagnostic>.Empty);
}

private class PriorityTestProjectDiagnosticAnalyzer : ProjectDiagnosticAnalyzer
{
protected PriorityTestProjectDiagnosticAnalyzer(int priority)
{
Priority = priority;
}

public override int Priority { get; }
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray<DiagnosticDescriptor>.Empty;
public override Task<ImmutableArray<Diagnostic>> AnalyzeProjectAsync(Project project, CancellationToken cancellationToken)
=> Task.FromResult(ImmutableArray<Diagnostic>.Empty);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ namespace Microsoft.CodeAnalysis.Diagnostics
/// </summary>
internal abstract class DocumentDiagnosticAnalyzer : DiagnosticAnalyzer
{
public const int DefaultPriority = 50;

public abstract Task<ImmutableArray<Diagnostic>> AnalyzeSyntaxAsync(Document document, CancellationToken cancellationToken);

public abstract Task<ImmutableArray<Diagnostic>> AnalyzeSemanticsAsync(Document document, CancellationToken cancellationToken);
Expand All @@ -21,5 +23,13 @@ internal abstract class DocumentDiagnosticAnalyzer : DiagnosticAnalyzer
public sealed override void Initialize(AnalysisContext context)
{
}

/// <summary>
/// This lets vsix installed <see cref="DocumentDiagnosticAnalyzer"/> or <see cref="ProjectDiagnosticAnalyzer"/> to
/// specify priority of the analyzer. Regular <see cref="DiagnosticAnalyzer"/> always comes before those 2 different types.
/// Priority is ascending order and this only works on HostDiagnosticAnalyzer meaning Vsix installed analyzers in VS.
/// This is to support partner teams (such as typescript and F#) who want to order their analyzer's execution order.
/// </summary>
public virtual int Priority => DefaultPriority;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ namespace Microsoft.CodeAnalysis.Diagnostics
/// </summary>
internal abstract class ProjectDiagnosticAnalyzer : DiagnosticAnalyzer
{
public const int DefaultPriority = 50;

public abstract Task<ImmutableArray<Diagnostic>> AnalyzeProjectAsync(Project project, CancellationToken cancellationToken);

/// <summary>
Expand All @@ -19,5 +21,13 @@ internal abstract class ProjectDiagnosticAnalyzer : DiagnosticAnalyzer
public sealed override void Initialize(AnalysisContext context)
{
}

/// <summary>
/// This lets vsix installed <see cref="DocumentDiagnosticAnalyzer"/> or <see cref="ProjectDiagnosticAnalyzer"/> to
/// specify priority of the analyzer. Regular <see cref="DiagnosticAnalyzer"/> always comes before those 2 different types.
/// Priority is ascending order and this only works on HostDiagnosticAnalyzer meaning Vsix installed analyzers in VS.
/// This is to support partner teams (such as typescript and F#) who want to order their analyzer's execution order.
/// </summary>
public virtual int Priority => DefaultPriority;
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
Expand Down Expand Up @@ -36,12 +37,6 @@ public IEnumerable<StateSet> GetOrCreateStateSets(string language)
return GetAnalyzerMap(language).GetStateSets();
}

public IEnumerable<DiagnosticAnalyzer> GetAnalyzers(string language)
{
var map = GetAnalyzerMap(language);
return map.GetAnalyzers();
}

public StateSet GetOrCreateStateSet(string language, DiagnosticAnalyzer analyzer)
{
return GetAnalyzerMap(language).GetStateSet(analyzer);
Expand All @@ -64,73 +59,68 @@ private DiagnosticAnalyzerMap CreateLanguageSpecificAnalyzerMap(string language,

private class DiagnosticAnalyzerMap
{
private readonly DiagnosticAnalyzer _compilerAnalyzer;
private const int BuiltInCompilerPriority = -2;
private const int RegularDiagnosticAnalyzerPriority = -1;

private readonly StateSet _compilerStateSet;

private readonly ImmutableArray<StateSet> _orderedSet;
private readonly ImmutableDictionary<DiagnosticAnalyzer, StateSet> _map;

public DiagnosticAnalyzerMap(HostAnalyzerManager analyzerManager, string language, ImmutableDictionary<DiagnosticAnalyzer, StateSet> analyzerMap)
{
// hold directly on to compiler analyzer
_compilerAnalyzer = analyzerManager.GetCompilerDiagnosticAnalyzer(language);
_map = analyzerMap;

var compilerAnalyzer = analyzerManager.GetCompilerDiagnosticAnalyzer(language);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like that we are tracking four different things now for analyzer prioritization.

  • The compiler analyzers
  • Implementations of DocumentDiagnosticAnalyzer
  • Implementations of ProjectDiagnosticAnalyzer
  • Everything else

Each of these four uses a different prioritization mechanism, and none of them leverage the functionality which users come to expect with MEF/VS (OrderAttribute / ExtensionOrderAttribute).

❓ What are our alternatives to unify and simplify this behavior?

Copy link
Contributor Author

@heejaechang heejaechang Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiagnosticAnalyzer doesn't use MEF. and I dont like OrderAttribute which require one to explicitly specify what comes before and after. that is just overkill for what is asked and get pretty complex fast. we used them and probably still use in some cases such as command handler, but we just hated them I believe. since whenever new one is added, one had to change multiple ones to adjust order. it is only suitable for ones that order must be declared quite clearly. this is not the case.

...

we have 4 different groups of analyzers (C#/VB compiler analyzer which is special, regular diagnostic analyzer, Document/ProjectDiagnosticAnalyzer, analyzer that implements IBuiltInAnalyzer). Engine treats them differently. reason is simple. since we want to allow special ability third party analyzer (regular diagnostic analyzer) can't use.

sure, we can have an issue tracking whether we should expose this special ability to third party or not. but that is not something this PR is about.


// in test case, we might not have the compiler analyzer.
if (_compilerAnalyzer == null)
if (compilerAnalyzer != null)
{
_map = analyzerMap;
return;
// hold onto stateSet for compiler analyzer
_compilerStateSet = analyzerMap[compilerAnalyzer];
}

_compilerStateSet = analyzerMap[_compilerAnalyzer];

// hold rest of analyzers
_map = analyzerMap.Remove(_compilerAnalyzer);
// order statesets
// order will be in this order
// BuiltIn Compiler Analyzer (C#/VB) < Regular DiagnosticAnalyzers < Document/ProjectDiagnosticAnalyzers
_orderedSet = _map.Values.OrderBy(PriorityComparison).ToImmutableArray();
}

public IEnumerable<DiagnosticAnalyzer> GetAnalyzers()
public ImmutableArray<StateSet> GetStateSets() => _orderedSet;

public StateSet GetStateSet(DiagnosticAnalyzer analyzer)
{
// always return compiler one first if it exists.
// it might not exist in test environment.
if (_compilerAnalyzer != null)
if (_map.TryGetValue(analyzer, out var stateSet))
{
yield return _compilerAnalyzer;
return stateSet;
}

foreach (var (analyzer, _) in _map)
{
yield return analyzer;
}
return null;
}

public IEnumerable<StateSet> GetStateSets()
private int PriorityComparison(StateSet state1, StateSet state2)
{
// always return compiler one first if it exists.
// it might not exist in test environment.
if (_compilerAnalyzer != null)
{
yield return _compilerStateSet;
}

// TODO: for now, this is static, but in future, we might consider making this a dynamic so that we process cheaper analyzer first.
foreach (var (_, set) in _map)
{
yield return set;
}
return GetPriority(state1) - GetPriority(state2);
}

public StateSet GetStateSet(DiagnosticAnalyzer analyzer)
private int GetPriority(StateSet state)
{
if (_compilerAnalyzer == analyzer)
// compiler gets highest priority
if (state == _compilerStateSet)
{
return _compilerStateSet;
return BuiltInCompilerPriority;
}

if (_map.TryGetValue(analyzer, out var set))
switch (state.Analyzer)
{
return set;
case DocumentDiagnosticAnalyzer analyzer:
return Math.Max(0, analyzer.Priority);
case ProjectDiagnosticAnalyzer analyzer:
return Math.Max(0, analyzer.Priority);
default:
// regular analyzer get next priority after compiler analyzer
return RegularDiagnosticAnalyzerPriority;
}

return null;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,6 @@ public StateManager(HostAnalyzerManager analyzerManager)
/// </summary>
public event EventHandler<ProjectAnalyzerReferenceChangedEventArgs> ProjectAnalyzerReferenceChanged;

/// <summary>
/// Return existing or new <see cref="DiagnosticAnalyzer"/>s for the given <see cref="Project"/>.
/// </summary>
public IEnumerable<DiagnosticAnalyzer> GetOrCreateAnalyzers(Project project)
{
return _hostStates.GetAnalyzers(project.Language).Concat(_projectStates.GetOrCreateAnalyzers(project));
}

/// <summary>
/// Return all <see cref="StateSet"/>.
/// This will never create new <see cref="StateSet"/> but will return ones already created.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,11 @@ internal Action<Exception, DiagnosticAnalyzer, Diagnostic> GetOnAnalyzerExceptio
return Owner.GetOnAnalyzerException(projectId, DiagnosticLogAggregator);
}

internal IEnumerable<DiagnosticAnalyzer> GetAnalyzersTestOnly(Project project)
Copy link
Contributor

@sharwell sharwell Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 It would be good to create an enforced [TestOnly] attribute. Outside the scope of this pull request, but something to think about if this is a common pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. we could create enforcer once we have call graph I believe with attribute.

{
return _stateManager.GetOrCreateStateSets(project).Select(s => s.Analyzer);
}

private static string GetDocumentLogMessage(string title, Document document, DiagnosticAnalyzer analyzer)
{
return $"{title}: ({document.FilePath ?? document.Name}), ({analyzer.ToString()})";
Expand Down
9 changes: 0 additions & 9 deletions src/Features/Core/Portable/Diagnostics/HostAnalyzerManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -221,15 +221,6 @@ public ImmutableDictionary<object, ImmutableArray<DiagnosticAnalyzer>> CreatePro
return CreateDiagnosticAnalyzersPerReferenceMap(CreateProjectAnalyzerReferencesMap(project), project.Language);
}

/// <summary>
/// Create <see cref="DiagnosticAnalyzer"/>s collection for given <paramref name="project"/>
/// </summary>
public ImmutableArray<DiagnosticAnalyzer> CreateDiagnosticAnalyzers(Project project)
{
var analyzersPerReferences = CreateDiagnosticAnalyzersPerReference(project);
return analyzersPerReferences.SelectMany(kv => kv.Value).ToImmutableArray();
}

/// <summary>
/// Check whether given <see cref="DiagnosticData"/> belong to compiler diagnostic analyzer
/// </summary>
Expand Down