Output syntax errors from the parser (microsoft#584)

* Fix microsoft#470

* Output syntax errors

* Properly clear

* - Fix async issue with analysis completion
- Clean up diagnostics service interface
- Use real DS in tests

* Add publishing test
Add NSubstitute
Move all services to the same namespace.

* Unused var

* Test forced publish on close

* Fix typo

* Update test framework

* Remove incorrect reference

* Move interface to the main class part

Author: Mikhail Arkhipov

src/Analysis/Ast/Impl/Analyzer/Definitions/IExpressionEvaluator.cs

@@ -14,8 +14,10 @@
 // permissions and limitations under the License.
 
 using System;
+using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Python.Analysis.Diagnostics;
 using Microsoft.Python.Analysis.Types;
 using Microsoft.Python.Analysis.Values;
 using Microsoft.Python.Core;
@@ -59,12 +61,12 @@ public interface IExpressionEvaluator {
 
         IMember LookupNameInScopes(string name, out IScope scope);
 
-        IPythonType GetTypeFromPepHint(Node node);
         IPythonType GetTypeFromString(string typeString);
 
         PythonAst Ast { get; }
         IPythonModule Module { get; }
         IPythonInterpreter Interpreter { get; }
         IServiceContainer Services { get; }
+        IEnumerable<DiagnosticsEntry> Diagnostics { get; }
     }
 }

src/Analysis/Ast/Impl/Analyzer/Evaluation/ExpressionEval.Hints.cs

@@ -25,7 +25,7 @@ namespace Microsoft.Python.Analysis.Analyzer.Evaluation {
     /// Helper class that provides methods for looking up variables
     /// and types in a chain of scopes during analysis.
     /// </summary>
-    internal sealed partial class ExpressionEval : IExpressionEvaluator {
+    internal sealed partial class ExpressionEval {
         public IPythonType GetTypeFromPepHint(Node node) {
             var location = GetLoc(node);
             var content = (Module as IDocument)?.Content;

src/Analysis/Ast/Impl/Analyzer/Evaluation/ExpressionEval.cs

@@ -35,7 +35,8 @@ namespace Microsoft.Python.Analysis.Analyzer.Evaluation {
     /// </summary>
     internal sealed partial class ExpressionEval : IExpressionEvaluator {
         private readonly Stack<Scope> _openScopes = new Stack<Scope>();
-        private readonly IDiagnosticsService _diagnostics;
+        private readonly List<DiagnosticsEntry> _diagnostics = new List<DiagnosticsEntry>();
+        private readonly object _lock = new object();
 
         public ExpressionEval(IServiceContainer services, IPythonModule module, PythonAst ast) {
             Services = services ?? throw new ArgumentNullException(nameof(services));
@@ -47,7 +48,6 @@ public ExpressionEval(IServiceContainer services, IPythonModule module, PythonAs
             DefaultLookupOptions = LookupOptions.Normal;
 
             //Log = services.GetService<ILogger>();
-            _diagnostics = services.GetService<IDiagnosticsService>();
         }
 
         public LookupOptions DefaultLookupOptions { get; set; }
@@ -69,6 +69,7 @@ public ExpressionEval(IServiceContainer services, IPythonModule module, PythonAs
         IScope IExpressionEvaluator.CurrentScope => CurrentScope;
         IGlobalScope IExpressionEvaluator.GlobalScope => GlobalScope;
         public LocationInfo GetLocation(Node node) => node?.GetLocation(Module, Ast) ?? LocationInfo.Empty;
+        public IEnumerable<DiagnosticsEntry> Diagnostics => _diagnostics;
 
         public Task<IMember> GetValueFromExpressionAsync(Expression expr, CancellationToken cancellationToken = default)
             => GetValueFromExpressionAsync(expr, DefaultLookupOptions, cancellationToken);
@@ -228,11 +229,11 @@ private async Task<IMember> GetValueFromConditionalAsync(ConditionalExpression e
             return trueValue ?? falseValue;
         }
 
-        private void AddDiagnostics(Uri documentUri, IEnumerable<DiagnosticsEntry> entries) {
+        private void ReportDiagnostics(Uri documentUri, IEnumerable<DiagnosticsEntry> entries) {
             // Do not add if module is library, etc. Only handle user code.
             if (Module.ModuleType == ModuleType.User) {
-                foreach (var e in entries) {
-                    _diagnostics?.Add(documentUri, e);
+                lock (_lock) {
+                    _diagnostics.AddRange(entries);
                 }
             }
         }

src/Analysis/Ast/Impl/Analyzer/PythonAnalyzer.cs

@@ -22,7 +22,7 @@
 using Microsoft.Python.Core;
 using Microsoft.Python.Core.Diagnostics;
 using Microsoft.Python.Core.Logging;
-using Microsoft.Python.Core.Shell;
+using Microsoft.Python.Core.Services;
 
 namespace Microsoft.Python.Analysis.Analyzer {
     public sealed class PythonAnalyzer : IPythonAnalyzer, IDisposable {
@@ -55,7 +55,6 @@ public PythonAnalyzer(IServiceManager services, string root) {
         public async Task AnalyzeDocumentAsync(IDocument document, CancellationToken cancellationToken) {
             var node = new DependencyChainNode(document);
             using (var cts = CancellationTokenSource.CreateLinkedTokenSource(_globalCts.Token, cancellationToken)) {
-                node.Analyzable.NotifyAnalysisPending();
                 try {
                     var analysis = await AnalyzeAsync(node, cts.Token);
                     node.Analyzable.NotifyAnalysisComplete(analysis);
@@ -78,17 +77,21 @@ public async Task AnalyzeDocumentDependencyChainAsync(IDocument document, Cancel
             using (var cts = CancellationTokenSource.CreateLinkedTokenSource(_globalCts.Token, cancellationToken)) {
                 var dependencyRoot = await _dependencyResolver.GetDependencyChainAsync(document, cts.Token);
                 // Notify each dependency that the analysis is now pending
-                NotifyAnalysisPending(dependencyRoot);
+                NotifyAnalysisPending(document, dependencyRoot);
 
                 cts.Token.ThrowIfCancellationRequested();
                 await AnalyzeChainAsync(dependencyRoot, cts.Token);
             }
         }
 
-        private void NotifyAnalysisPending(IDependencyChainNode node) {
-            node.Analyzable.NotifyAnalysisPending();
+        private void NotifyAnalysisPending(IDocument document, IDependencyChainNode node) {
+            // Notify each dependency that the analysis is now pending except the source
+            // since if document has changed, it already incremented its expected analysis.
+            if (node.Analyzable != document) {
+                node.Analyzable.NotifyAnalysisPending();
+            }
             foreach (var c in node.Children) {
-                NotifyAnalysisPending(c);
+                NotifyAnalysisPending(document, c);
             }
         }
 

src/Analysis/Ast/Impl/Analyzer/PythonInterpreter.cs

@@ -23,7 +23,7 @@
 using Microsoft.Python.Analysis.Specializations.Typing;
 using Microsoft.Python.Analysis.Types;
 using Microsoft.Python.Core;
-using Microsoft.Python.Core.Shell;
+using Microsoft.Python.Core.Services;
 using Microsoft.Python.Parsing;
 
 namespace Microsoft.Python.Analysis.Analyzer {

src/Analysis/Ast/Impl/Diagnostics/IDiagnosticsService.cs

@@ -18,8 +18,25 @@
 
 namespace Microsoft.Python.Analysis.Diagnostics {
     public interface IDiagnosticsService {
-        IReadOnlyList<DiagnosticsEntry> Diagnostics { get; }
-        void Add(Uri documentUri, DiagnosticsEntry entry);
+        /// <summary>
+        /// Current complete diagnostics.
+        /// </summary>
+        IReadOnlyDictionary<Uri, IReadOnlyList<DiagnosticsEntry>> Diagnostics { get; }
+
+        /// <summary>
+        /// Replaces diagnostics for the document by the new set.
+        /// </summary>
+        void Replace(Uri documentUri, IEnumerable<DiagnosticsEntry> entries);
+
+        /// <summary>
+        /// Removes document from the diagnostics report. Typically when document closes.
+        /// </summary>
+        void Remove(Uri documentUri);
+
+        /// <summary>
+        /// Defines delay in milliseconds from the idle time start and
+        /// the diagnostic publishing to the client.
+        /// </summary>
         int PublishingDelay { get; set; }
     }
 }

src/Analysis/Ast/Impl/Extensions/DiagnosticsServiceExtensions.cs

@@ -56,6 +56,7 @@ protected enum State {
         private readonly DocumentBuffer _buffer = new DocumentBuffer();
         private readonly CancellationTokenSource _allProcessingCts = new CancellationTokenSource();
         private IReadOnlyList<DiagnosticsEntry> _parseErrors = Array.Empty<DiagnosticsEntry>();
+        private readonly IDiagnosticsService _diagnosticsService;
 
         private string _documentation; // Must be null initially.
         private TaskCompletionSource<IDocumentAnalysis> _analysisTcs;
@@ -72,14 +73,15 @@ protected enum State {
         protected State ContentState { get; set; } = State.None;
 
         protected PythonModule(string name, ModuleType moduleType, IServiceContainer services) {
-            Check.ArgumentNotNull(nameof(name), name);
-            Name = name;
-            Services = services;
+            Name = name ?? throw new ArgumentNullException(nameof(name));
+            Services = services ?? throw new ArgumentNullException(nameof(services));
             ModuleType = moduleType;
 
             Log = services?.GetService<ILogger>();
             Interpreter = services?.GetService<IPythonInterpreter>();
             Analysis = new EmptyAnalysis(services, this);
+
+            _diagnosticsService = services.GetService<IDiagnosticsService>();
         }
 
         protected PythonModule(string moduleName, string filePath, ModuleType moduleType, IPythonModule stub, IServiceContainer services) :
@@ -218,13 +220,13 @@ protected virtual string LoadContent() {
         private void InitializeContent(string content) {
             lock (AnalysisLock) {
                 LoadContent(content);
+
                 var startParse = ContentState < State.Parsing && _parsingTask == null;
                 var startAnalysis = startParse | (ContentState < State.Analyzing && _analysisTcs?.Task == null);
 
                 if (startAnalysis) {
-                    _analysisTcs = new TaskCompletionSource<IDocumentAnalysis>();
+                    ExpectNewAnalysis();
                 }
-
                 if (startParse) {
                     Parse();
                 }
@@ -250,6 +252,7 @@ private void LoadContent(string content) {
         public void Dispose() => Dispose(true);
 
         protected virtual void Dispose(bool disposing) {
+            _diagnosticsService?.Remove(Uri);
             _allProcessingCts.Cancel();
             _allProcessingCts.Dispose();
         }
@@ -288,7 +291,7 @@ public async Task<PythonAst> GetAstAsync(CancellationToken cancellationToken = d
             Task t = null;
             while (true) {
                 lock (AnalysisLock) {
-                    if(t == _parsingTask) {
+                    if (t == _parsingTask) {
                         break;
                     }
                     cancellationToken.ThrowIfCancellationRequested();
@@ -314,10 +317,8 @@ public async Task<PythonAst> GetAstAsync(CancellationToken cancellationToken = d
 
         public void Update(IEnumerable<DocumentChange> changes) {
             lock (AnalysisLock) {
-                ExpectedAnalysisVersion++;
-
+                ExpectNewAnalysis();
                 _linkedAnalysisCts?.Cancel();
-                _analysisTcs = new TaskCompletionSource<IDocumentAnalysis>();
 
                 _parseCts?.Cancel();
                 _parseCts = new CancellationTokenSource();
@@ -352,18 +353,22 @@ private void Parse() {
         }
 
         private void Parse(CancellationToken cancellationToken) {
-            var sink = new CollectingErrorSink();
+            CollectingErrorSink sink = null;
             int version;
             Parser parser;
 
             //Log?.Log(TraceEventType.Verbose, $"Parse begins: {Name}");
 
             lock (AnalysisLock) {
                 version = _buffer.Version;
-                parser = Parser.CreateParser(new StringReader(_buffer.Text), Interpreter.LanguageVersion, new ParserOptions {
-                    StubFile = FilePath != null && Path.GetExtension(FilePath).Equals(".pyi", FileSystem.StringComparison),
-                    ErrorSink = sink
-                });
+                var options = new ParserOptions {
+                    StubFile = FilePath != null && Path.GetExtension(FilePath).Equals(".pyi", FileSystem.StringComparison)
+                };
+                if (ModuleType == ModuleType.User) {
+                    sink = new CollectingErrorSink();
+                    options.ErrorSink = sink;
+                }
+                parser = Parser.CreateParser(new StringReader(_buffer.Text), Interpreter.LanguageVersion, options);
             }
 
             var ast = parser.ParseFile();
@@ -376,7 +381,13 @@ private void Parse(CancellationToken cancellationToken) {
                     throw new OperationCanceledException();
                 }
                 _ast = ast;
-                _parseErrors = sink.Diagnostics;
+                _parseErrors = sink?.Diagnostics ?? Array.Empty<DiagnosticsEntry>();
+
+                // Do not report issues with libraries or stubs
+                if (sink != null) {
+                    _diagnosticsService?.Replace(Uri, _parseErrors);
+                }
+
                 _parsingTask = null;
                 ContentState = State.Parsed;
             }
@@ -428,11 +439,10 @@ public override void Add(string message, SourceSpan span, int errorCode, Severit
         public void NotifyAnalysisPending() {
             lock (AnalysisLock) {
                 // The notification comes from the analyzer when it needs to invalidate
-                // current analysis since one of the dependencies changed. Upon text
-                // buffer change the version may be incremented twice - once in Update()
-                // and then here. This is normal.
-                ExpectedAnalysisVersion++;
-                _analysisTcs = _analysisTcs ?? new TaskCompletionSource<IDocumentAnalysis>();
+                // current analysis since one of the dependencies changed. If text
+                // buffer changed then the notification won't come since the analyzer
+                // filters out original initiator of the analysis.
+                ExpectNewAnalysis();
                 //Log?.Log(TraceEventType.Verbose, $"Analysis pending: {Name}");
             }
         }
@@ -448,10 +458,11 @@ public virtual bool NotifyAnalysisComplete(IDocumentAnalysis analysis) {
                     // to perform additional actions on the completed analysis such
                     // as declare additional variables, etc.
                     OnAnalysisComplete();
+                    ContentState = State.Analyzed;
 
-                    _analysisTcs.TrySetResult(analysis);
+                    var tcs = _analysisTcs;
                     _analysisTcs = null;
-                    ContentState = State.Analyzed;
+                    tcs.TrySetResult(analysis);
 
                     NewAnalysis?.Invoke(this, EventArgs.Empty);
                     return true;
@@ -477,6 +488,11 @@ public Task<IDocumentAnalysis> GetAnalysisAsync(CancellationToken cancellationTo
         }
         #endregion
 
+        private void ExpectNewAnalysis() {
+            ExpectedAnalysisVersion++;
+            _analysisTcs = _analysisTcs ?? new TaskCompletionSource<IDocumentAnalysis>();
+        }
+
         private string TryGetDocFromModuleInitFile() {
             if (string.IsNullOrEmpty(FilePath) || !FileSystem.FileExists(FilePath)) {
                 return string.Empty;