Report unresolved imports #593

MikhailArkhipov · 2019-02-10T04:48:01Z

Closes #583
Closes #512

Adds reporting of unresolved imports
Incorporates suggestions from Diagnostic list flickers annoyingly when opening a document #512
Restores message severity mapping
Addresses comments missed in Language server using new analysis #546 review.
Tests

I guess I will need to re-fork my master since all commits from earlier work still drag behind.

Fix post-mortem earlier PR comment

jakebailey · 2019-02-11T20:22:11Z

I don't know the exact line in the PR to comment on but for as imports, but I think that the original name is the more useful one to be reporting. For example, matplotlib/matplotlib.pyplot is the thing that's missing here, but the message reports plt:

jakebailey · 2019-02-11T20:22:43Z

For example, in the old LS:

MikhailArkhipov · 2019-02-12T01:19:28Z

Yep, changing

- Add tests

AlexanderSher · 2019-02-12T14:26:14Z

src/Analysis/Ast/Test/ImportTests.cs

+            analysis.Diagnostics.Should().HaveCount(1);
+            var d = analysis.Diagnostics.First();
+            d.ErrorCode.Should().Be(ErrorCodes.UnresolvedImport);
+            d.SourceSpan.Should().Be(1, 8, 1, 19);


Can we use Range instead of SourceSpan?

Range is not in analysis, it is in LSP. Can theoretically move it down.

I thought Diagnostics is part of LSP as well. We also have IndexSpan.

Diagnostics yes. DiagnosticsEntry, no. Switching away from SourceSpan would require changing tens of tests. Analysis is using LocationInfo which produces SourceSpan.

AlexanderSher · 2019-02-12T14:31:43Z

src/Analysis/Ast/Impl/Modules/PythonModule.cs

@@ -460,6 +460,11 @@ public override void Add(string message, SourceSpan span, int errorCode, Severit
                    OnAnalysisComplete();
                    ContentState = State.Analyzed;

+                    // Do not report issues with libraries or stubs


What if user has opened library file explicitly?

Also see: #584 (comment)

We don’t report issues in libraries, this is not something actionable to the user. Something like resharper does not produce issues in disassembled code...

src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs

src/Analysis/Ast/Impl/Analyzer/Handlers/ImportHandler.cs

src/Analysis/Ast/Impl/Analyzer/Handlers/FromImportHandler.cs

…-server

AlexanderSher · 2019-02-13T17:18:58Z

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

+
+        public DiagnosticsSeverityMap(string[] errors, string[] warnings, string[] information, string[] disabled) {
+            _map.Clear();
+            // disabled > error > warning > information


Is it compatible with VSC?

Yes. Same code as before

AlexanderSher · 2019-02-13T17:28:38Z

src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs

+            set {
+                lock (_lock) {
+                    _severityMap = value;
+                    PublishDiagnostics();


One more place under lock.

As a matter of fact, I made PublishDiagnostics inside locks intentionally. The problem here if publishing goes off the lock, then we can go out of sync between _changed and the state of the collection. For example,

public void Remove(Uri documentUri) { lock (_lock) { // Before removing the document, make sure we clear its diagnostics. _diagnostics[documentUri] = new List<DiagnosticsEntry>(); PublishDiagnostics(); _diagnostics.Remove(documentUri); } }

PublishDiagnostics must be called between _diagnostics[documentUri] = new List<DiagnosticsEntry>() which makes collection of the error empty and VSC clears the task list and _diagnostics.Remove(documentUri) which removes the document. Calling PublishDiagnostics after removal will lead to stale errors in the error list.

In the code above if we do _diagnostics[documentUri] = new List<DiagnosticsEntry>() and persist to array, then call PublishDiagnostics off the lock, and then do _diagnostics.Remove(documentUri); under lock again, the document can technically get reopened while we are not under lock and we will remove document incorrectly.

It's probably small price to pay since we don't wait and basically just post the diagnostics under the lock.

AlexanderSher · 2019-02-13T17:28:55Z

src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs

-                    };
-                    _clientApp.NotifyWithParameterObjectAsync("textDocument/publishDiagnostics", parameters).DoNotWait();
-                }
+                diagnostics = Diagnostics.ToArray();


We aren't calling _diagnostics.Clear(); anywhere. Is that correct?

ClearAllDiagnostics, on Dispose. Otherwise it stays.

* Part 7 * Buildable * PR feedback * Merge conflict * Fix microsoft#446 * Fix microsoft#446 * Part 8 * Part 9 * Buildable * Part 10 * Part 11 * Part 12 * Buildable * Part 14 * First passing test * Simplify configuration * Style * Fix test and move code to folders * Builtins import * Fix microsoft#470 * Fluents * Add search path * Import analysis, part I * Simplify builtins handling * Remove IMember * Handle import specific * More tests * Add typeshed * Renames * Make sure lazy modules are loaded * Renames * Move/rename * Rework importing * Derivation rework * Part 2 * Part 3 * Buildable * Module members * Async walk * Imports test pass * Remove lazy types * Fix from import * Stubs * Double overloads * Fix datetime test * Couple more tests + fluents * Few more tests * Additionl test + union type * Built-in scrape tests * Full stdlib scrape test * Complete async AST walker * Conditional defines test + variable loc cleanup * More stub tests Fix stub loading for packages (port from DDG) Split walker into multiple files * Add some (broken mostly) tests from DDG * Move document tests * Function arg eval, part I * Instance/factory * Builds * Test fixes * Fix static and instance call eval * More tests * More ported tests * Specialize builtin functions * Make walkers common and handle nested functions * Moar tests * Parser fixes + more tests * Handle negative numbers * Fix null ref * Basic list support * Few more list tests * Basic iterators * Support __iter__ * Iterators * Fix couple of tests * Add decorator test * Generics, part I * Generics, part 2 * Generics, part 3 * Basic TypeVar test * Typings, part 4 * Fix test * Generics, part 6 * Generics, part 7 * More tests (failing) * Forward ref fixes * Reorg * Improve symbol resolution + test fixes * Test fixes * Dictionary, part I * Part 11 * Fix test * Tests * Tests * More dict work * List ctor * Skip some tests for now * Fix iterators * Tuple slicing * Polish type comparo in return types * Add Mapping and tests * Add Iterable * Fix typo * Add Iterator[T] + test * Simplify typing types * Class reduction * Fix tests * Test fix * Handle 'with' statement * Handle try-except * Class method inheritance + NewType * Container types * Containers test * Tests * Handle generic type alias * Named tuple * Global/non-local * Handle tuples in for Handle custom iterators * Basic generator * Any/AnyStr * Test fixes * Type/Optional/etc handling * Proper doc population * Tests + range * Argument match * Basic argset and diagnostics * Argset tests * Exclude WIP * Exclude WIP * Arg eval * Arg match, part 2 * Tests and generic arg comparisons * Function eval with arguments * Baselines * Fix test * Undo AST formatting change and update baseline * LS cleanup 1 * Fix list ctor argument unpacking * Cleanup 2 * Builds * Partial completions * Partial * Partial * Simple test * Tests * Basic startup * Clean up a bit * Remove debug code * Port formatter tests * Fix tokenizer crash * Async fixes * Hover * Basic hover * Adjust expression options * Basic signature help * Fix class/instance * Update test * Fix builtin creation exception * Fix tests * Actually provide declared module * Completion test (partial) * Undo * Fix null await Fix override completions + test * Exports filtering Prevent augmenting imported types * Filter variables & exports * Ported tests * Test fixes * More ported tests * Fix exception completions * Import completions * Scope completions * With completions * Test fixes * WIP * Test fix * Better arg match * Temp disable WIP * First cut * Fix type leak * WIP * Remove ConfigureAwait and handle canceled and failed in the analysis notifications * WIP * Generic class base * Generic forward reference resolution * Suppress completion in strings + test * Prevent recursion on generic resolution Better match arguments * Handle call expression in generics * Relax condition as it happens in tensorflow * Fix typeshed version search Make writing cached modules async Fix module doc fetching * Hover tests * Fix prom import hover * Hover tests * Synchronize test cache writing * First cut * Test * Fixes * Add tests for os.path Null ref fix * Fix cache check * Improve resolution of builtins and typing in stubs * Merge tests * Add ntst for requests * Handle typeshed better * Fix custom stub handling * Better sync * Move files * Fix parameter locations * Hover improvement * PEP hints * One more test for PEP hints * Better handle hover over import as * Text based generic constraints * Handle with better with generic stubs * Undo debug * Handle non-binary open() Temporary fix 'with' handler since we haven't specialized IO/TextIO/BinaryIO yet. * Output syntax errors * Properly clear * - Fix async issue with analysis completion - Clean up diagnostics service interface - Use real DS in tests * Use proper scope when analyzing module * Severity mapping and reporting * Add publishing test Add NSubstitute Move all services to the same namespace. * Unused var * Test forced publish on close * Fix typo * Update test framework * Import location * Remove incorrect reference * Diagnostic severity mapping test Fix post-mortem earlier PR comment * Minor fixes * Move interface to the main class part * Flicker reduction * - Correct reported unresolved import name - Add tests * PR feedback

MikhailArkhipov added 30 commits December 4, 2018 15:46

Part 7

98067f1

Buildable

c12df18

PR feedback

500e969

Merge master

dd2f114

Merge conflict

b829969

Fix #446

780757a

Fix #446

7bf1f38

Merge branch 'analysis2' into analysis5

710fc18

Part 8

6fb8ef6

Part 9

32616d5

Buildable

aca9aea

Part 10

60bc841

Part 11

f3ac892

Part 12

6c62582

Buildable

4de6d73

Part 14

9f21d05

First passing test

542a469

Simplify configuration

7a53a17

Style

8badc5e

Fix test and move code to folders

e6d1e69

Builtins import

f714cda

Fix #470

9ce186a

Fluents

bba86a6

Add search path

e107d9c

Import analysis, part I

5815db9

Simplify builtins handling

cccc6d8

Remove IMember

281c293

Handle import specific

f42a5ce

More tests

97092b5

Add typeshed

f3df3fc

Mikhail Arkhipov added 10 commits February 7, 2019 14:47

Merge branch 'master' into imprep

627eddf

Severity mapping and reporting

d1f9b73

Remove incorrect reference

77aa79b

Merge branch 'master' into imprep

0b7f8b5

Diagnostic severity mapping test

63660d8

Fix post-mortem earlier PR comment

Minor fixes

9c0b724

Move interface to the main class part

2efd0d8

Merge master

8daa58c

Flicker reduction

67790ce

Merge master

d245be8

MikhailArkhipov requested a review from AlexanderSher February 11, 2019 22:18

- Correct reported unresolved import name

606d8b2

- Add tests

AlexanderSher reviewed Feb 12, 2019

View reviewed changes

src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs Outdated Show resolved Hide resolved

AlexanderSher reviewed Feb 12, 2019

View reviewed changes

src/Analysis/Ast/Impl/Analyzer/Handlers/ImportHandler.cs Outdated Show resolved Hide resolved

AlexanderSher reviewed Feb 12, 2019

View reviewed changes

src/Analysis/Ast/Impl/Analyzer/Handlers/FromImportHandler.cs Show resolved Hide resolved

Mikhail Arkhipov added 2 commits February 12, 2019 21:27

Merge branch 'master' of https://github.com/Microsoft/python-language…

f6b054a

…-server

PR feedback

c7822d0

AlexanderSher reviewed Feb 13, 2019

View reviewed changes

MikhailArkhipov mentioned this pull request Feb 13, 2019

Resolve issue when specific types shared data after creation off generic template #600

Merged

AlexanderSher approved these changes Feb 13, 2019

View reviewed changes

MikhailArkhipov merged commit 037d248 into microsoft:master Feb 13, 2019

MikhailArkhipov mentioned this pull request Mar 5, 2019

LS: Output lint-level diagnostics #581

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report unresolved imports #593

Report unresolved imports #593

MikhailArkhipov commented Feb 10, 2019 •

edited

Loading

jakebailey commented Feb 11, 2019

jakebailey commented Feb 11, 2019

MikhailArkhipov commented Feb 12, 2019

AlexanderSher Feb 12, 2019

MikhailArkhipov Feb 12, 2019

AlexanderSher Feb 13, 2019

MikhailArkhipov Feb 13, 2019

AlexanderSher Feb 12, 2019

jakebailey Feb 12, 2019

MikhailArkhipov Feb 12, 2019

AlexanderSher Feb 13, 2019

MikhailArkhipov Feb 13, 2019

AlexanderSher Feb 13, 2019

MikhailArkhipov Feb 13, 2019 •

edited

Loading

AlexanderSher Feb 13, 2019

MikhailArkhipov Feb 13, 2019

Report unresolved imports #593

Report unresolved imports #593

Conversation

MikhailArkhipov commented Feb 10, 2019 • edited Loading

jakebailey commented Feb 11, 2019

jakebailey commented Feb 11, 2019

MikhailArkhipov commented Feb 12, 2019

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 Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikhailArkhipov commented Feb 10, 2019 •

edited

Loading

MikhailArkhipov Feb 13, 2019 •

edited

Loading