[WIP] Move unresolved imports to the language server + related bug fixes #4367

AlexanderSher · 2018-06-13T14:53:19Z

Move unresolved imports to the analysis server
Fix LastReportedDiagnostics isn't cleared after document is closed and reopened #4349 for VS Code: LastReportedDiagnostics isn't cleared after document is closed and reopened bug
Fix OpenFile._pendingChanges isn't cleared after document is closed #4351 for VS Code: OpenFile._pendingChanges isn't cleared after document is closed
Fix Inconsistent behavior when showing errors in project items #4363: Inconsistent behavior when showing errors in project items
Fixed memory leaks in ErrorTaskProvider

Fixes for #4349 and #4351 for VS require better synchronization between BufferParser.AddBuffer and BufferParser.RemoveBuffer that has to be converted into async to send and await for textDocument/didOpen and textDocument/didClose.

- Fix microsoft#4349 for VS Code: LastReportedDiagnostics isn't cleared after document is closed and reopened bug - Fix microsoft#4351 for VS Code: OpenFile._pendingChanges isn't cleared after document is closed - Fix microsoft#4363: Inconsistent behavior when showing errors in project items - Fixed memory leaks in ErrorTaskProvider

AlexanderSher · 2018-06-13T14:55:15Z

Python/Product/Analysis/LanguageServer/Server.Completion.cs

@@ -27,8 +27,6 @@ public sealed partial class Server {
            await IfTestWaitForAnalysisCompleteAsync();

            var uri = @params.textDocument.uri;
-            // Make sure document is enqueued for processing
-            var openFile = _openFiles.GetDocument(uri);


This doesn't ensure anything, so removed

AlexanderSher · 2018-06-13T14:56:45Z

Python/Product/Analysis/LanguageServer/OpenFiles.cs

@@ -49,7 +49,7 @@ sealed class OpenFile {

        public void DidChangeTextDocument(DidChangeTextDocumentParams @params, Action<IDocument> enqueueAction) {
            var changes = @params.contentChanges;
-            if (changes == null) {
+            if (changes == null || changes.Length == 0) {


VSC can send textDocument/didChange with no changes.

For instance, it sometimes happens when file is saved. Version isn't updated as well, so we don't have to do anything here.

zooba · 2018-06-13T23:07:06Z

Python/Product/Analysis/Analyzer/ImportStatementWalker.cs

+
+        private static bool IsImportError(Expression expr) {
+            if (expr is NameExpression name) {
+                return name.Name == "Exception" || name.Name == "BaseException" || name.Name == "ImportError";


Python 3.6 added ModuleNotFoundError, which we should probably also have here

zooba · 2018-06-13T23:07:27Z

Python/Product/Analysis/LanguageServer/OpenFiles.cs

@@ -49,7 +49,7 @@ sealed class OpenFile {

        public void DidChangeTextDocument(DidChangeTextDocumentParams @params, Action<IDocument> enqueueAction) {
            var changes = @params.contentChanges;
-            if (changes == null) {
+            if (changes == null || changes.Length == 0) {


zooba · 2018-06-13T23:09:22Z

Python/Product/Analysis/LanguageServer/Server.cs

-            if (_clientCaps?.python?.analysisUpdates ?? false) {
-                pyItem.OnNewAnalysis += ProjectEntry_OnNewAnalysis;
-            }
+            pyItem.OnNewAnalysis += (o, e) => OnPythonEntryNewAnalysis(pyItem);


Why change the event handler signature? We don't need the lambda and closure here, and for large workspaces it's a cost we may notice.

Since OnNewAnalysis is a public API, I can't change its signature and provide correct type in eventArgs. Type check inside old handler was misleading - there can be no other type, so this code is cleaner and easier to understand (no need to look for all existing implementations of IPythonProjectTree). Closure contains two reference type fields, so I can't imagine how we may notice it considering hundreds of kilobytes of garbage strings in G2 per every new tree parse.

zooba · 2018-06-13T23:20:31Z

Python/Product/Analysis/LanguageServer/Server.cs

+            _clientCaps = new Capabilities(@params.capabilities);
+            _settings.SetCompletionTimeout(_clientCaps.Python.CompletionsTimeout);
+            _traceLogging = _clientCaps.Python.TraceLogging;
+            _analyzer.EnableDiagnostics = _clientCaps.Python.LiveLinting;


LiveLinting is currently false by default, since we have other messages behind that flag that we do not want to release. If unresolved imports are going there too, we might need an alternative filter somewhere in the analyzer to allow "good" diagnostics.

The LiveLinting flag is meant to enable experimental stuff, so maybe internally we can change it to EnableExperimentalDiagnostics and filter those somehow?

Yeah, unresolved imports should be default on (as warning). We can expose controls for severity like VS does so user can suppress warnings if they chose to.

Another alternative is create new structure with flags for assorted diagnostic controls and I can expose it in the VSC settings. We can then keep expanding it as needed

_analyzer.EnableDiagnostics doesn't affect unresolved imports, and yes, they are reported as warnings.
I like the idea of having granulated diagnostics controls on the client side, that we can switch from false to true as needed.

MikhailArkhipov · 2018-06-15T05:32:21Z

LGTM apart from the options setting

zooba · 2018-06-15T14:39:30Z

Yep, but let’s make it “from true to false” on the client side. The language server should be able to introduce new warnings without a client update. But I’d still like to have a “give me experimental warnings” option that doesn’t refer to each warning code individually.

- Address CR comments - Enable default values population during deserialization

- Remove some dead code - Merge branch 'master' of https://github.com/microsoft/PTVS # Conflicts: # Python/Product/Analysis/LanguageServer/Server.cs # Python/Product/VSCode/AnalysisVsc/LanguageServer.cs

AlexanderSher · 2018-06-22T16:38:21Z

Matching PR in vscode: microsoft/vscode-python#2049

zooba · 2018-06-22T22:52:51Z

Python/Product/Analysis/Analysis.csproj

@@ -44,6 +44,10 @@
    <PlatformTarget>AnyCPU</PlatformTarget>
  </PropertyGroup>
  <ItemGroup>
+    <Reference Include="Newtonsoft.Json">


I want to be very certain that we need dependencies for this assembly before we add them. Right now this assembly has no external dependencies, so let's be 110% sure we need this and it can't be done in some other way (even if it's a little less convenient for us).

zooba

Making the LSP structures in Analysis.dll a direct representation of the JSON packets is deliberate, and I'm not sure what benefit we get from making them different and adding an external dependency to hide the differences when we could just keep them the same.

zooba · 2018-06-22T22:53:53Z

Python/Product/Analysis/LanguageServer/Structures.cs

@@ -320,23 +324,33 @@ public struct ExecuteCommandCapabilities { public bool dynamicRegistration; }
    }

    [Serializable]
-    public class TextDocumentClientCapabilities {
+    public struct TextDocumentClientCapabilities {


I believe this will prevent us from changing the contents of this struct/class in the future. I'd rather it stay as a class (and probably should be sealed).

(Similar for the other types in this file)

I agree that we need to declare clear rules on how types for serialization are declared, but I'm not sure how classes would help with versioning. To be detached from protocol changes, we need to remove all references to protocol data types from public part of API and/or inject a group of abstractions.

The benefits of structs are that they are not nullable, which saves us from endless null checks, enforces default values and makes null a real expected value instead of a sign that we haven't serialized something.

The benefit of classes is that they are safely mutable, but I'm against mutable protocol data types. Another benefit is that class loses its default constructor when non-default is presented, which will immediately give us deseriazliation error if default value isn't properly defined.

@MikhailArkhipov , what is your opinion?

I'm thinking more about assemblies that reference us. We can add members to a class without breaking those dependencies, but we can't do that with a value type because it will change the size of the type.

zooba · 2018-06-22T22:54:12Z

Python/Product/Analysis/LanguageServer/Structures.cs

        [Serializable]
        public struct SynchronizationCapabilities {
+            [DefaultValue(false)]


This is implied - no need to specify it.

zooba · 2018-06-22T22:56:52Z

Python/Product/Analysis/LanguageServer/Structures.cs

        /// <summary>
        /// Client expects analysis progress updates, including notifications
        /// when analysis is complete for a particular document version.
        /// </summary>
-        public bool? analysisUpdates;
+        public readonly bool AnalysisUpdates;


Why change these from how the JSON is represented?

Because inside C# code we don't care about JSON (that's the whole point of these types) and everywhere else public fields and properties are UpperCamelCase (which is also default for C#). Names in constructor match names in JSON. Type isn't serialized in .net, only deserialized.

As the author of these types, I say the whole point is to represent the JSON messages as closely as possible :) (See my comment below for my actual argument about not changing this)

zooba · 2018-06-22T22:57:22Z

Python/Product/Analysis/LanguageServer/Structures.cs

-        public object experimental;
-        public PythonClientCapabilities python;
+        public readonly WorkspaceClientCapabilities Workspace;
+        public readonly TextDocumentClientCapabilities TextDocument;


These no longer serialize correctly.

AFAIK, we don't serialize them - only deserialize.

Well, now we can't serialize them :) Tests would be the main place I'd expect we'd want to serialize them, but even so, consistency with the rest of this file (where we do serialize values) seems like a good enough reason to keep them consistent.

We can set up parallel (and private) data structures for internal use if we don't want to directly work with the JSON-like types, but I don't have any issue with using them directly.

zooba · 2018-06-26T19:18:32Z

Python/Product/Analysis/Analyzer/ImportStatementWalker.cs

@@ -0,0 +1,100 @@
+using System;


Needs a copyright header (did you add this or just merge it in from someone else's PR? regardless, copy/paste a header please)

# Conflicts: # Python/Product/PythonTools/PythonTools/Intellisense/ProjectAnalyzer.cs

AlexanderSher added 2 commits June 13, 2018 07:33

Fix merge

208f999

AlexanderSher requested review from zooba and MikhailArkhipov and removed request for zooba June 13, 2018 14:53

AlexanderSher commented Jun 13, 2018

View reviewed changes

zooba reviewed Jun 13, 2018

View reviewed changes

MikhailArkhipov mentioned this pull request Jun 14, 2018

Warn against unused imports microsoft/vscode-python#43

Closed

AlexanderSher added 2 commits June 22, 2018 08:07

- Add diagnostics settings

9562cb9

- Address CR comments - Enable default values population during deserialization

- Fix assembly loading for .net framework

dd72264

- Remove some dead code - Merge branch 'master' of https://github.com/microsoft/PTVS # Conflicts: # Python/Product/Analysis/LanguageServer/Server.cs # Python/Product/VSCode/AnalysisVsc/LanguageServer.cs

zooba reviewed Jun 22, 2018

View reviewed changes

Revert changes in LSP.

df241fe

zooba approved these changes Jun 26, 2018

View reviewed changes

Merge branch 'master' of https://github.com/microsoft/PTVS

185c70b

# Conflicts: # Python/Product/PythonTools/PythonTools/Intellisense/ProjectAnalyzer.cs

AlexanderSher merged commit 1dc7e02 into microsoft:master Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Move unresolved imports to the language server + related bug fixes #4367

[WIP] Move unresolved imports to the language server + related bug fixes #4367

AlexanderSher commented Jun 13, 2018

AlexanderSher Jun 13, 2018

AlexanderSher Jun 13, 2018

zooba Jun 13, 2018

AlexanderSher Jun 15, 2018

zooba Jun 13, 2018

AlexanderSher Jun 15, 2018

zooba Jun 13, 2018

zooba Jun 13, 2018

AlexanderSher Jun 15, 2018

zooba Jun 13, 2018

MikhailArkhipov Jun 14, 2018

MikhailArkhipov Jun 14, 2018

AlexanderSher Jun 15, 2018

MikhailArkhipov commented Jun 15, 2018

zooba commented Jun 15, 2018 via email

AlexanderSher commented Jun 22, 2018

zooba Jun 22, 2018

zooba left a comment •

edited

Loading

zooba Jun 22, 2018

zooba Jun 22, 2018

AlexanderSher Jun 25, 2018

zooba Jun 25, 2018

zooba Jun 22, 2018

zooba Jun 22, 2018

AlexanderSher Jun 25, 2018 •

edited

Loading

zooba Jun 25, 2018

zooba Jun 22, 2018

AlexanderSher Jun 25, 2018

zooba Jun 25, 2018

zooba Jun 26, 2018

[WIP] Move unresolved imports to the language server + related bug fixes #4367

[WIP] Move unresolved imports to the language server + related bug fixes #4367

Conversation

AlexanderSher commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikhailArkhipov commented Jun 15, 2018

zooba commented Jun 15, 2018 via email

AlexanderSher commented Jun 22, 2018

Choose a reason for hiding this comment

zooba left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderSher Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zooba left a comment •

edited

Loading

AlexanderSher Jun 25, 2018 •

edited

Loading