From e5fcc19224d191dcf0e88dd8c785839f6be82ca1 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 14 Sep 2018 11:21:18 -0700 Subject: [PATCH 1/7] Better progress reporting --- .../Impl/Definitions/ILogger.cs | 1 + .../AnalysisProgressReporter.cs | 85 +++++++++++++++++++ .../Impl/Implementation/Server.cs | 7 +- src/LanguageServer/Impl/LanguageServer.cs | 41 ++------- src/LanguageServer/Impl/Resources.Designer.cs | 18 ++++ src/LanguageServer/Impl/Resources.resx | 6 ++ src/LanguageServer/Impl/Services/UIService.cs | 3 +- 7 files changed, 124 insertions(+), 37 deletions(-) create mode 100644 src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs diff --git a/src/LanguageServer/Impl/Definitions/ILogger.cs b/src/LanguageServer/Impl/Definitions/ILogger.cs index 6ef60820f..d9e2f3d8d 100644 --- a/src/LanguageServer/Impl/Definitions/ILogger.cs +++ b/src/LanguageServer/Impl/Definitions/ILogger.cs @@ -19,5 +19,6 @@ namespace Microsoft.Python.LanguageServer { public interface ILogger { void TraceMessage(IFormattable message); + void TraceMessage(string message); } } diff --git a/src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs b/src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs new file mode 100644 index 000000000..a5ff07333 --- /dev/null +++ b/src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs @@ -0,0 +1,85 @@ +// Python Tools for Visual Studio +// Copyright(c) Microsoft Corporation +// All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the License); you may not use +// this file except in compliance with the License. You may obtain a copy of the +// License at http://www.apache.org/licenses/LICENSE-2.0 +// +// THIS CODE IS PROVIDED ON AN *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS +// OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION ANY +// IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR PURPOSE, +// MERCHANTABLITY OR NON-INFRINGEMENT. +// +// See the Apache Version 2.0 License for specific language governing +// permissions and limitations under the License. + +using System; +using System.Collections.Generic; +using Microsoft.PythonTools.Analysis.Infrastructure; + +namespace Microsoft.Python.LanguageServer.Implementation { + sealed class AnalysisProgressReporter : IDisposable { + private readonly DisposableBag _disposables = new DisposableBag(nameof(AnalysisProgressReporter)); + private readonly Dictionary _activeAnalysis = new Dictionary(); + private readonly IProgressService _progressService; + private readonly ILogger _logger; + private readonly Server _server; + private readonly object _lock = new object(); + private IProgress _progress; + + public AnalysisProgressReporter(Server server, IProgressService progressService, ILogger logger) { + _progressService = progressService; + _logger = logger; + + _server = server; + _server.OnAnalysisQueued += OnAnalysisQueued; + _server.OnAnalysisComplete += OnAnalysisComplete; + _disposables + .Add(() => _server.OnAnalysisQueued -= OnAnalysisQueued) + .Add(() => _server.OnAnalysisComplete -= OnAnalysisComplete) + .Add(() => _progress?.Dispose()); + } + + public void Dispose() { + _disposables.TryDispose(); + } + + private void OnAnalysisQueued(object sender, AnalysisQueuedEventArgs e) { + lock (_lock) { + if (_activeAnalysis.ContainsKey(e.uri)) { + _activeAnalysis[e.uri]++; + } else { + _activeAnalysis[e.uri] = 1; + } + UpdateProgressMessage(); + } + } + private void OnAnalysisComplete(object sender, AnalysisCompleteEventArgs e) { + lock (_lock) { + if (_activeAnalysis.TryGetValue(e.uri, out var count)) { + if (count > 1) { + _activeAnalysis[e.uri]--; + } else { + _activeAnalysis.Remove(e.uri); + } + } else { + _logger.TraceMessage($"Analysis completed for {e.uri} that is not in the dictionary."); + } + UpdateProgressMessage(); + } + } + + private void UpdateProgressMessage() { + if(_activeAnalysis.Count > 0) { + _progress = _progress ?? _progressService.BeginProgress(); + _progress.Report(_activeAnalysis.Count == 1 + ? Resources.AnalysisProgress_SingleItemRemaining + : Resources.AnalysisProgress_MultipleItemsRemaining.FormatInvariant(_activeAnalysis.Count)).DoNotWait(); + } else { + _progress?.Dispose(); + _progress = null; + } + } + } +} diff --git a/src/LanguageServer/Impl/Implementation/Server.cs b/src/LanguageServer/Impl/Implementation/Server.cs index 872297840..6977dd978 100644 --- a/src/LanguageServer/Impl/Implementation/Server.cs +++ b/src/LanguageServer/Impl/Implementation/Server.cs @@ -123,11 +123,14 @@ private void Analysis_UnhandledException(object sender, UnhandledExceptionEventA public void Dispose() => _disposableBag.TryDispose(); - public void TraceMessage(IFormattable message) { + #region ILogger + public void TraceMessage(IFormattable message) => TraceMessage(message.ToString()); + public void TraceMessage(string message) { if (_traceLogging) { - LogMessage(MessageType.Log, message.ToString()); + LogMessage(MessageType.Log, message); } } + #endregion #region Client message handling internal InitializeResult GetInitializeResult() => new InitializeResult { diff --git a/src/LanguageServer/Impl/LanguageServer.cs b/src/LanguageServer/Impl/LanguageServer.cs index cd99b1b9c..f3c8dd30c 100644 --- a/src/LanguageServer/Impl/LanguageServer.cs +++ b/src/LanguageServer/Impl/LanguageServer.cs @@ -43,22 +43,23 @@ public sealed partial class LanguageServer: IDisposable { private IServiceContainer _services; private IUIService _ui; private ITelemetryService _telemetry; - private IProgressService _progress; private JsonRpc _rpc; private bool _filesLoaded; private Task _progressReportingTask; private PathsWatcher _pathsWatcher; private IdleTimeTracker _idleTimeTracker; + private AnalysisProgressReporter _analysisProgressReporter; public CancellationToken Start(IServiceContainer services, JsonRpc rpc) { _services = services; _ui = services.GetService(); _telemetry = services.GetService(); - _progress = services.GetService(); - _rpc = rpc; + var progress = services.GetService(); + _analysisProgressReporter = new AnalysisProgressReporter(_server, progress, _server); + _server.OnLogMessage += OnLogMessage; _server.OnShowMessage += OnShowMessage; _server.OnTelemetry += OnTelemetry; @@ -66,8 +67,6 @@ public CancellationToken Start(IServiceContainer services, JsonRpc rpc) { _server.OnApplyWorkspaceEdit += OnApplyWorkspaceEdit; _server.OnRegisterCapability += OnRegisterCapability; _server.OnUnregisterCapability += OnUnregisterCapability; - _server.OnAnalysisQueued += OnAnalysisQueued; - _server.OnAnalysisComplete += OnAnalysisComplete; _disposables .Add(() => _server.OnLogMessage -= OnLogMessage) @@ -77,40 +76,14 @@ public CancellationToken Start(IServiceContainer services, JsonRpc rpc) { .Add(() => _server.OnApplyWorkspaceEdit -= OnApplyWorkspaceEdit) .Add(() => _server.OnRegisterCapability -= OnRegisterCapability) .Add(() => _server.OnUnregisterCapability -= OnUnregisterCapability) - .Add(() => _server.OnAnalysisQueued -= OnAnalysisQueued) - .Add(() => _server.OnAnalysisComplete -= OnAnalysisComplete) - .Add(_prioritizer); + .Add(_prioritizer) + .Add(_analysisProgressReporter) + .Add(() => _pathsWatcher?.Dispose()); return _sessionTokenSource.Token; } - private void OnAnalysisQueued(object sender, AnalysisQueuedEventArgs e) => HandleAnalysisQueueEvent(); - private void OnAnalysisComplete(object sender, AnalysisCompleteEventArgs e) => HandleAnalysisQueueEvent(); - - private void HandleAnalysisQueueEvent() - => _progressReportingTask = _progressReportingTask ?? ProgressWorker(); - - private async Task ProgressWorker() { - await Task.Delay(1000); - - var remaining = _server.EstimateRemainingWork(); - if (remaining > 0) { - using (var p = _progress.BeginProgress()) { - while (remaining > 0) { - var items = remaining > 1 ? "items" : "item"; - // TODO: in localization this needs to be two different messages - // since not all languages allow sentence construction. - await p.Report($"Analyzing workspace, {remaining} {items} remaining..."); - await Task.Delay(100); - remaining = _server.EstimateRemainingWork(); - } - } - } - _progressReportingTask = null; - } - public void Dispose() { - _pathsWatcher?.Dispose(); _disposables.TryDispose(); _server.Dispose(); } diff --git a/src/LanguageServer/Impl/Resources.Designer.cs b/src/LanguageServer/Impl/Resources.Designer.cs index b4253f567..81b84c32d 100644 --- a/src/LanguageServer/Impl/Resources.Designer.cs +++ b/src/LanguageServer/Impl/Resources.Designer.cs @@ -60,6 +60,24 @@ internal Resources() { } } + /// + /// Looks up a localized string similar to Analyzing workspace, {0} items remaining.... + /// + internal static string AnalysisProgress_MultipleItemsRemaining { + get { + return ResourceManager.GetString("AnalysisProgress_MultipleItemsRemaining", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Analyzing workspace, 1 item remaining.... + /// + internal static string AnalysisProgress_SingleItemRemaining { + get { + return ResourceManager.GetString("AnalysisProgress_SingleItemRemaining", resourceCulture); + } + } + /// /// Looks up a localized string similar to Cannot rename. /// diff --git a/src/LanguageServer/Impl/Resources.resx b/src/LanguageServer/Impl/Resources.resx index f5ea08423..c84f73daa 100644 --- a/src/LanguageServer/Impl/Resources.resx +++ b/src/LanguageServer/Impl/Resources.resx @@ -117,6 +117,12 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + Analyzing workspace, {0} items remaining... + + + Analyzing workspace, 1 item remaining... + Cannot rename diff --git a/src/LanguageServer/Impl/Services/UIService.cs b/src/LanguageServer/Impl/Services/UIService.cs index 36c95933c..3c0c7e088 100644 --- a/src/LanguageServer/Impl/Services/UIService.cs +++ b/src/LanguageServer/Impl/Services/UIService.cs @@ -63,7 +63,8 @@ public Task LogMessage(string message, MessageType messageType) { public Task SetStatusBarMessage(string message) => _rpc.NotifyWithParameterObjectAsync("window/setStatusBarMessage", message); - public void TraceMessage(IFormattable message) => LogMessage(message.ToString(), MessageType.Info); + public void TraceMessage(string message) => LogMessage(message.ToString(), MessageType.Info); + public void TraceMessage(IFormattable message) => TraceMessage(message.ToString()); public void SetLogLevel(MessageType logLevel) => _logLevel = logLevel; } From 5156180599ec3ab6d5986355cc337f295a26299d Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 14 Sep 2018 11:56:56 -0700 Subject: [PATCH 2/7] Remove unused field --- src/LanguageServer/Impl/LanguageServer.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/LanguageServer/Impl/LanguageServer.cs b/src/LanguageServer/Impl/LanguageServer.cs index f3c8dd30c..31ff3a407 100644 --- a/src/LanguageServer/Impl/LanguageServer.cs +++ b/src/LanguageServer/Impl/LanguageServer.cs @@ -46,7 +46,6 @@ public sealed partial class LanguageServer: IDisposable { private JsonRpc _rpc; private bool _filesLoaded; - private Task _progressReportingTask; private PathsWatcher _pathsWatcher; private IdleTimeTracker _idleTimeTracker; private AnalysisProgressReporter _analysisProgressReporter; From a2b7b9316baa4e256e8835c90a320be01bc3d1eb Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 14 Sep 2018 12:44:56 -0700 Subject: [PATCH 3/7] PR feedback --- .../Impl/Definitions/ILanguageServerExtensionProvider.cs | 3 +-- src/LanguageServer/Impl/Definitions/ILogger.cs | 1 - src/LanguageServer/Impl/Implementation/Server.cs | 5 ++--- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/LanguageServer/Impl/Definitions/ILanguageServerExtensionProvider.cs b/src/LanguageServer/Impl/Definitions/ILanguageServerExtensionProvider.cs index a42b1095e..c30ba55d5 100644 --- a/src/LanguageServer/Impl/Definitions/ILanguageServerExtensionProvider.cs +++ b/src/LanguageServer/Impl/Definitions/ILanguageServerExtensionProvider.cs @@ -17,9 +17,8 @@ using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; -using Microsoft.Python.LanguageServer.Extensions; -namespace Microsoft.Python.LanguageServer { +namespace Microsoft.Python.LanguageServer.Extensions { /// /// Implemented on a class that can create a language server extension. /// This class must have a default constructor. diff --git a/src/LanguageServer/Impl/Definitions/ILogger.cs b/src/LanguageServer/Impl/Definitions/ILogger.cs index d9e2f3d8d..6ef60820f 100644 --- a/src/LanguageServer/Impl/Definitions/ILogger.cs +++ b/src/LanguageServer/Impl/Definitions/ILogger.cs @@ -19,6 +19,5 @@ namespace Microsoft.Python.LanguageServer { public interface ILogger { void TraceMessage(IFormattable message); - void TraceMessage(string message); } } diff --git a/src/LanguageServer/Impl/Implementation/Server.cs b/src/LanguageServer/Impl/Implementation/Server.cs index 6977dd978..6e6eb7c5b 100644 --- a/src/LanguageServer/Impl/Implementation/Server.cs +++ b/src/LanguageServer/Impl/Implementation/Server.cs @@ -124,10 +124,9 @@ private void Analysis_UnhandledException(object sender, UnhandledExceptionEventA public void Dispose() => _disposableBag.TryDispose(); #region ILogger - public void TraceMessage(IFormattable message) => TraceMessage(message.ToString()); - public void TraceMessage(string message) { + public void TraceMessage(IFormattable message) { if (_traceLogging) { - LogMessage(MessageType.Log, message); + LogMessage(MessageType.Log, message.ToString()); } } #endregion From 35c494edc2db5ff7d45d6ef1d5e8e6c83b5b4bd2 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 14 Sep 2018 13:04:34 -0700 Subject: [PATCH 4/7] Enable tests --- src/Analysis/Engine/Test/AnalysisTest.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Analysis/Engine/Test/AnalysisTest.cs b/src/Analysis/Engine/Test/AnalysisTest.cs index ab711446e..739cb69e2 100644 --- a/src/Analysis/Engine/Test/AnalysisTest.cs +++ b/src/Analysis/Engine/Test/AnalysisTest.cs @@ -3211,7 +3211,6 @@ def f(a): } [TestMethod, Priority(0)] - [Ignore("https://github.com/Microsoft/python-language-server/issues/51")] public async Task ReferencesCrossModule() { var fobText = @" from oar import abc @@ -3238,7 +3237,6 @@ from oar import abc } [TestMethod, Priority(0)] - [Ignore("https://github.com/Microsoft/python-language-server/issues/53")] public async Task SuperclassMemberReferencesCrossModule() { // https://github.com/Microsoft/PTVS/issues/2271 From 09bddb58b790ce7f3be76d9298f5632475f0bb36 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 14 Sep 2018 13:07:20 -0700 Subject: [PATCH 5/7] Enable test --- src/Analysis/Engine/Test/AnalysisTest.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Analysis/Engine/Test/AnalysisTest.cs b/src/Analysis/Engine/Test/AnalysisTest.cs index 739cb69e2..239f1ef5c 100644 --- a/src/Analysis/Engine/Test/AnalysisTest.cs +++ b/src/Analysis/Engine/Test/AnalysisTest.cs @@ -684,7 +684,6 @@ public async Task ImportTrailingComma() { } [TestMethod, Priority(0)] - [Ignore("https://github.com/Microsoft/python-language-server/issues/49")] public async Task ImportStarCorrectRefs() { var text1 = @" from mod2 import * From 5dc3c8061870a475c90a5e2f7adec0d98af878a9 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 14 Sep 2018 14:17:40 -0700 Subject: [PATCH 6/7] Make interface public --- .../Engine/Impl/Values/Definitions/IAnalysisIterableValue.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analysis/Engine/Impl/Values/Definitions/IAnalysisIterableValue.cs b/src/Analysis/Engine/Impl/Values/Definitions/IAnalysisIterableValue.cs index aca8af8fa..4fc2aa859 100644 --- a/src/Analysis/Engine/Impl/Values/Definitions/IAnalysisIterableValue.cs +++ b/src/Analysis/Engine/Impl/Values/Definitions/IAnalysisIterableValue.cs @@ -17,7 +17,7 @@ using System.Collections.Generic; namespace Microsoft.PythonTools.Analysis.Values { - interface IAnalysisIterableValue: IAnalysisValue { + public interface IAnalysisIterableValue: IAnalysisValue { IEnumerable IndexTypes { get; } } } From 7380582e85d03f906cfffe8ce635da5258972906 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 14 Sep 2018 16:21:23 -0700 Subject: [PATCH 7/7] Back to queue tracking --- src/Analysis/Engine/Impl/ProjectEntry.cs | 3 +- src/Analysis/Engine/Impl/PythonAnalyzer.cs | 1 - .../AnalysisProgressReporter.cs | 45 ++++++++++--------- .../Impl/Implementation/Server.Extensions.cs | 3 +- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/Analysis/Engine/Impl/ProjectEntry.cs b/src/Analysis/Engine/Impl/ProjectEntry.cs index c32e75227..babdb397c 100644 --- a/src/Analysis/Engine/Impl/ProjectEntry.cs +++ b/src/Analysis/Engine/Impl/ProjectEntry.cs @@ -159,6 +159,7 @@ internal void SetCompleteAnalysis() { lock (this) { _analysisTcs.TrySetResult(Analysis); } + RaiseNewAnalysis(); } internal void ResetCompleteAnalysis() { @@ -216,7 +217,7 @@ public void Analyze(CancellationToken cancel, bool enqueueOnly) { } } - internal void RaiseNewAnalysis() => NewAnalysis?.Invoke(this, EventArgs.Empty); + private void RaiseNewAnalysis() => NewAnalysis?.Invoke(this, EventArgs.Empty); public int AnalysisVersion { get; private set; } diff --git a/src/Analysis/Engine/Impl/PythonAnalyzer.cs b/src/Analysis/Engine/Impl/PythonAnalyzer.cs index acc1f610b..5b1fe8104 100644 --- a/src/Analysis/Engine/Impl/PythonAnalyzer.cs +++ b/src/Analysis/Engine/Impl/PythonAnalyzer.cs @@ -925,7 +925,6 @@ public void AnalyzeQueuedEntries(CancellationToken cancel) { ddg.Analyze(Queue, cancel, _reportQueueSize, _reportQueueInterval); foreach (ProjectEntry entry in ddg.AnalyzedEntries) { entry.SetCompleteAnalysis(); - entry.RaiseNewAnalysis(); } } diff --git a/src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs b/src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs index a5ff07333..ef41a099f 100644 --- a/src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs +++ b/src/LanguageServer/Impl/Implementation/AnalysisProgressReporter.cs @@ -15,18 +15,18 @@ // permissions and limitations under the License. using System; -using System.Collections.Generic; +using System.Threading.Tasks; using Microsoft.PythonTools.Analysis.Infrastructure; namespace Microsoft.Python.LanguageServer.Implementation { sealed class AnalysisProgressReporter : IDisposable { private readonly DisposableBag _disposables = new DisposableBag(nameof(AnalysisProgressReporter)); - private readonly Dictionary _activeAnalysis = new Dictionary(); private readonly IProgressService _progressService; private readonly ILogger _logger; private readonly Server _server; private readonly object _lock = new object(); private IProgress _progress; + private Task _queueMonitoringTask; public AnalysisProgressReporter(Server server, IProgressService progressService, ILogger logger) { _progressService = progressService; @@ -41,44 +41,45 @@ public AnalysisProgressReporter(Server server, IProgressService progressService, .Add(() => _progress?.Dispose()); } - public void Dispose() { - _disposables.TryDispose(); - } + public void Dispose() => _disposables.TryDispose(); private void OnAnalysisQueued(object sender, AnalysisQueuedEventArgs e) { lock (_lock) { - if (_activeAnalysis.ContainsKey(e.uri)) { - _activeAnalysis[e.uri]++; - } else { - _activeAnalysis[e.uri] = 1; - } UpdateProgressMessage(); + _queueMonitoringTask = _queueMonitoringTask ?? QueueMonitoringTask(); } } private void OnAnalysisComplete(object sender, AnalysisCompleteEventArgs e) { lock (_lock) { - if (_activeAnalysis.TryGetValue(e.uri, out var count)) { - if (count > 1) { - _activeAnalysis[e.uri]--; - } else { - _activeAnalysis.Remove(e.uri); - } - } else { - _logger.TraceMessage($"Analysis completed for {e.uri} that is not in the dictionary."); - } UpdateProgressMessage(); } } private void UpdateProgressMessage() { - if(_activeAnalysis.Count > 0) { + var count = _server.EstimateRemainingWork(); + if (count > 0) { _progress = _progress ?? _progressService.BeginProgress(); - _progress.Report(_activeAnalysis.Count == 1 + _progress.Report(count == 1 ? Resources.AnalysisProgress_SingleItemRemaining - : Resources.AnalysisProgress_MultipleItemsRemaining.FormatInvariant(_activeAnalysis.Count)).DoNotWait(); + : Resources.AnalysisProgress_MultipleItemsRemaining.FormatInvariant(count)).DoNotWait(); } else { + EndProgress(); + } + } + + private async Task QueueMonitoringTask() { + try { + await _server.WaitForCompleteAnalysisAsync(); + } finally { + EndProgress(); + } + } + + private void EndProgress() { + lock (_lock) { _progress?.Dispose(); _progress = null; + _queueMonitoringTask = null; } } } diff --git a/src/LanguageServer/Impl/Implementation/Server.Extensions.cs b/src/LanguageServer/Impl/Implementation/Server.Extensions.cs index e05b10b99..7e2611f3b 100644 --- a/src/LanguageServer/Impl/Implementation/Server.Extensions.cs +++ b/src/LanguageServer/Impl/Implementation/Server.Extensions.cs @@ -77,8 +77,7 @@ private async Task InvokeExtensionsAsync(Func