From 3193a934db5a167a7221b857c0a4bbf4ecb5c09b Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Tue, 8 Oct 2019 16:19:55 -0700 Subject: [PATCH 01/43] first draft of add import on python. currently it does only basic behavior and works only for module known to path resolver and loaded modules (variable module) no test yet. require many refactorings --- .../Modules/Definitions/IModuleManagement.cs | 4 + .../Resolution/MainModuleResolution.cs | 12 + .../Resolution/ModuleResolutionBase.cs | 5 - .../PathResolverSnapshot.cs | 12 +- src/Core/Impl/Extensions/ArrayExtensions.cs | 17 + src/Core/Impl/Text/Position.cs | 15 +- .../Impl/Implementation/Server.Editor.cs | 20 +- .../Impl/Implementation/Server.cs | 11 +- src/LanguageServer/Impl/LanguageServer.cs | 10 +- src/LanguageServer/Impl/Protocol/Classes.cs | 21 ++ .../Impl/Sources/CodeActionSource.cs | 332 ++++++++++++++++++ .../Impl/Ast/SourceLocationExtensions.cs | 16 + 12 files changed, 456 insertions(+), 19 deletions(-) create mode 100644 src/LanguageServer/Impl/Sources/CodeActionSource.cs diff --git a/src/Analysis/Ast/Impl/Modules/Definitions/IModuleManagement.cs b/src/Analysis/Ast/Impl/Modules/Definitions/IModuleManagement.cs index a74e221c3..91a034f98 100644 --- a/src/Analysis/Ast/Impl/Modules/Definitions/IModuleManagement.cs +++ b/src/Analysis/Ast/Impl/Modules/Definitions/IModuleManagement.cs @@ -14,6 +14,8 @@ // permissions and limitations under the License. using System; +using System.Collections.Generic; +using System.Threading; using Microsoft.Python.Analysis.Caching; using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Types; @@ -86,5 +88,7 @@ public interface IModuleManagement : IModuleResolution { ImmutableArray LibraryPaths { get; } bool SetUserConfiguredPaths(ImmutableArray paths); + + IEnumerable GetImportedModules(CancellationToken cancellationToken); } } diff --git a/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs b/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs index 26c0d3f83..055c5ad04 100644 --- a/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs +++ b/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs @@ -55,6 +55,18 @@ public MainModuleResolution(string root, IServiceContainer services, ImmutableAr public IBuiltinsPythonModule BuiltinsModule { get; private set; } + public IEnumerable GetImportedModules(CancellationToken cancellationToken) { + foreach (var moduleRef in Modules.Values) { + if (moduleRef.Value != null) { + yield return moduleRef.Value; + } + } + + foreach (var module in _specialized.Values) { + yield return module; + } + } + protected override IPythonModule CreateModule(string name) { var moduleImport = CurrentPathResolver.GetModuleImportFromModuleName(name); if (moduleImport == null) { diff --git a/src/Analysis/Ast/Impl/Modules/Resolution/ModuleResolutionBase.cs b/src/Analysis/Ast/Impl/Modules/Resolution/ModuleResolutionBase.cs index c38a1eeeb..3a0c0b8dd 100644 --- a/src/Analysis/Ast/Impl/Modules/Resolution/ModuleResolutionBase.cs +++ b/src/Analysis/Ast/Impl/Modules/Resolution/ModuleResolutionBase.cs @@ -74,11 +74,6 @@ public IPythonModule GetOrLoadModule(string name) { return module; } - module = Interpreter.ModuleResolution.GetSpecializedModule(name); - if (module != null) { - return module; - } - // Now try regular case. if (Modules.TryGetValue(name, out var moduleRef)) { return moduleRef.GetOrCreate(name, this); diff --git a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs index 68aa0f8c7..2d17c2ff9 100644 --- a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs +++ b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs @@ -82,6 +82,14 @@ private PathResolverSnapshot(PythonLanguageVersion pythonLanguageVersion, string } public ImmutableArray GetAllImportableModuleNames(bool includeImplicitPackages = true) { + return GetAllImportableModuleFullName(n => !string.IsNullOrEmpty(n.FullModuleName), includeImplicitPackages); + } + + public ImmutableArray GetAllImportableModuleByName(string name, bool includeImplicitPackages = true) { + return GetAllImportableModuleFullName(n => string.Equals(n.Name, name), includeImplicitPackages); + } + + private ImmutableArray GetAllImportableModuleFullName(Func predicate, bool includeImplicitPackages = true) { var roots = _roots.Prepend(_nonRooted); var items = new Queue(roots); var names = ImmutableArray.Empty; @@ -89,7 +97,7 @@ public ImmutableArray GetAllImportableModuleNames(bool includeImplicitPa while (items.Count > 0) { var item = items.Dequeue(); if (item != null) { - if (!string.IsNullOrEmpty(item.FullModuleName) && (item.IsModule || includeImplicitPackages)) { + if (predicate(item) && (item.IsModule || includeImplicitPackages)) { names = names.Add(item.FullModuleName); } @@ -101,7 +109,7 @@ public ImmutableArray GetAllImportableModuleNames(bool includeImplicitPa return names.AddRange( _builtins.Children - .Where(b => !string.IsNullOrEmpty(b.FullModuleName)) + .Where(b => predicate(b)) .Select(b => b.FullModuleName) ); } diff --git a/src/Core/Impl/Extensions/ArrayExtensions.cs b/src/Core/Impl/Extensions/ArrayExtensions.cs index 20c6478b2..4bc152b93 100644 --- a/src/Core/Impl/Extensions/ArrayExtensions.cs +++ b/src/Core/Impl/Extensions/ArrayExtensions.cs @@ -14,6 +14,7 @@ // permissions and limitations under the License. using System; +using System.Collections.Generic; namespace Microsoft.Python.Core { public static class ArrayExtensions { @@ -36,5 +37,21 @@ public static int IndexOf(this T[] array, TValue value, Func AddIfNotNull(this List list, T item) where T : class { + if (item == null) { + return list; + } + + list.Add(item); + return list; + } + public static List AddIfNotNull(this List list, params T[] items) where T : class { + foreach (var item in items) { + list.AddIfNotNull(item); + } + + return list; + } } } diff --git a/src/Core/Impl/Text/Position.cs b/src/Core/Impl/Text/Position.cs index d7ad18649..08b38569d 100644 --- a/src/Core/Impl/Text/Position.cs +++ b/src/Core/Impl/Text/Position.cs @@ -18,7 +18,7 @@ namespace Microsoft.Python.Core.Text { [Serializable] - public struct Position { + public struct Position : IEquatable { /// /// Line position in a document (zero-based). /// @@ -39,7 +39,20 @@ public struct Position { public static bool operator >(Position p1, Position p2) => p1.line > p2.line || p1.line == p2.line && p1.character > p2.character; public static bool operator <(Position p1, Position p2) => p1.line < p2.line || p1.line == p2.line && p1.character < p2.character; + public static bool operator ==(Position p1, Position p2) => p1.Equals(p2); + public static bool operator !=(Position p1, Position p2) => !p1.Equals(p2); + public bool Equals(Position other) => line == other.line && character == other.character; + + public override bool Equals(object obj) { + if (obj is Position other) { + return Equals(other); + } + + return false; + } + + public override int GetHashCode() => 0; public override string ToString() => $"({line}, {character})"; } } diff --git a/src/LanguageServer/Impl/Implementation/Server.Editor.cs b/src/LanguageServer/Impl/Implementation/Server.Editor.cs index 0898c023c..b566015e6 100644 --- a/src/LanguageServer/Impl/Implementation/Server.Editor.cs +++ b/src/LanguageServer/Impl/Implementation/Server.Editor.cs @@ -98,7 +98,7 @@ public async Task GotoDeclaration(TextDocumentPositionParams @params, var analysis = await Document.GetAnalysisAsync(uri, Services, CompletionAnalysisTimeout, cancellationToken); var reference = new DeclarationSource(Services).FindDefinition(analysis, @params.position, out _); - return reference != null ? new Location { uri = reference.uri, range = reference.range} : null; + return reference != null ? new Location { uri = reference.uri, range = reference.range } : null; } public Task FindReferences(ReferencesParams @params, CancellationToken cancellationToken) { @@ -112,5 +112,23 @@ public Task Rename(RenameParams @params, CancellationToken cancel _log?.Log(TraceEventType.Verbose, $"Rename in {uri} at {@params.position}"); return new RenameSource(Services).RenameAsync(uri, @params.position, @params.newName, cancellationToken); } + + public async Task CodeAction(CodeActionParams @params, CancellationToken cancellationToken) { + var uri = @params.textDocument.uri; + _log?.Log(TraceEventType.Verbose, $"Code Action in {uri} at {@params.range}"); + + if (@params.context.diagnostics?.Length == 0) { + return Array.Empty(); + } + + // * NOTE * CodeActionParams.CodeActionContext._version is always null. PublishDiagnostic doesn't send version + // so not sure where that version is supposed to come from. and not sure whether what version it is + // supposed to be. text version? analysis version? + // we might want to check version at some point and return no code action if we are called with + // staled diagnostics + var analysis = await Document.GetAnalysisAsync(uri, Services, CompletionAnalysisTimeout, cancellationToken); + var codeActions = await new CodeActionSource(Services).GetCodeActionsAsync(analysis, @params.range, @params.context.diagnostics, cancellationToken); + return codeActions ?? Array.Empty(); + } } } diff --git a/src/LanguageServer/Impl/Implementation/Server.cs b/src/LanguageServer/Impl/Implementation/Server.cs index 4a553a2f8..3a2139983 100644 --- a/src/LanguageServer/Impl/Implementation/Server.cs +++ b/src/LanguageServer/Impl/Implementation/Server.cs @@ -98,6 +98,7 @@ public Server(IServiceManager services) { firstTriggerCharacter = "\n", moreTriggerCharacter = new[] { ";", ":" } }, + codeActionProvider = true, } }; @@ -191,11 +192,11 @@ public void DidChangeConfiguration(DidChangeConfigurationParams @params, Cancell _disposableBag.ThrowIfDisposed(); switch (@params.settings) { case ServerSettings settings: { - Settings = settings; - _symbolHierarchyMaxSymbols = Settings.analysis.symbolsHierarchyMaxSymbols; - _completionSource.Options = Settings.completion; - break; - } + Settings = settings; + _symbolHierarchyMaxSymbols = Settings.analysis.symbolsHierarchyMaxSymbols; + _completionSource.Options = Settings.completion; + break; + } default: _log?.Log(TraceEventType.Error, "change configuration notification sent unsupported settings"); break; diff --git a/src/LanguageServer/Impl/LanguageServer.cs b/src/LanguageServer/Impl/LanguageServer.cs index 64bb7f293..da05462b9 100644 --- a/src/LanguageServer/Impl/LanguageServer.cs +++ b/src/LanguageServer/Impl/LanguageServer.cs @@ -215,11 +215,11 @@ public async Task DocumentSymbol(JToken token, CancellationTok } } - //[JsonRpcMethod("textDocument/codeAction")] - //public async Task CodeAction(JToken token, CancellationToken cancellationToken) { - // await _prioritizer.DefaultPriorityAsync(cancellationToken); - // return await _server.CodeAction(ToObject(token), cancellationToken); - //} + [JsonRpcMethod("textDocument/codeAction")] + public async Task CodeAction(JToken token, CancellationToken cancellationToken) { + await _prioritizer.DefaultPriorityAsync(cancellationToken); + return await _server.CodeAction(ToObject(token), cancellationToken); + } //[JsonRpcMethod("textDocument/codeLens")] //public async Task CodeLens(JToken token, CancellationToken cancellationToken) { diff --git a/src/LanguageServer/Impl/Protocol/Classes.cs b/src/LanguageServer/Impl/Protocol/Classes.cs index bdb39e38a..ab5757f26 100644 --- a/src/LanguageServer/Impl/Protocol/Classes.cs +++ b/src/LanguageServer/Impl/Protocol/Classes.cs @@ -741,4 +741,25 @@ public sealed class PublishDiagnosticsParams { [JsonProperty] public Diagnostic[] diagnostics; } + + // + // Summary: + // A class representing a change that can be performed in code. A CodeAction must + // either set "edit" or "command". + // If both are supplied, the edit will be applied first, then the command will be executed. + [Serializable] + public sealed class CodeAction { + // + // Summary: + // Gets or sets the human readable title for this code action. + public string title; + // + // Summary: + // Gets or sets the workspace edit that this code action performs. + public WorkspaceEdit edit; + // + // Summary: + // Gets or sets the command that this code action executes. + public Command command; + } } diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs new file mode 100644 index 000000000..ddbed66f5 --- /dev/null +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -0,0 +1,332 @@ +// 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, +// MERCHANTABILITY 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 System.Linq; +using System.Runtime.CompilerServices; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Python.Analysis; +using Microsoft.Python.Analysis.Analyzer; +using Microsoft.Python.Analysis.Analyzer.Expressions; +using Microsoft.Python.Analysis.Core.Interpreter; +using Microsoft.Python.Analysis.Diagnostics; +using Microsoft.Python.Analysis.Types; +using Microsoft.Python.Core; +using Microsoft.Python.Core.Collections; +using Microsoft.Python.Core.Text; +using Microsoft.Python.LanguageServer.Protocol; +using Microsoft.Python.Parsing.Ast; +using Range = Microsoft.Python.Core.Text.Range; + +namespace Microsoft.Python.LanguageServer.Sources { + internal sealed class CodeActionSource { + private readonly IServiceContainer _services; + private readonly ImmutableArray _codeActionProviders = + ImmutableArray.Create(MissingImportCodeActionProvider.Instance); + + public CodeActionSource(IServiceContainer services) { + _services = services; + } + + public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, Core.Text.Range range, Diagnostic[] diagnostics, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + + var results = new List(); + + // * NOTE * this always re-calculate whole document for all linters. + // consider creating linter result cache somewhere (MRU?) and + // add error code as optional argument to run only that linter rather + // than always running all linters. + // also, current LintModule implementation always run all linters + // even if the linter option is off to set references in the given + // modules. this code path should use different code path to prevent that. + foreach (var diagnostic in GetMatchingDiagnostics(analysis, diagnostics, cancellationToken)) { + foreach (var codeActionProvider in _codeActionProviders) { + if (codeActionProvider.FixableDiagnostics.Any(code => code == diagnostic.ErrorCode)) { + results.AddRange(await codeActionProvider.GetCodeActionAsync(analysis, diagnostic, cancellationToken)); + } + } + } + + return results.ToArray(); + } + + private IEnumerable GetMatchingDiagnostics(IDocumentAnalysis analysis, Diagnostic[] diagnostics, CancellationToken cancellationToken) { + var analyzer = _services.GetService(); + foreach (var diagnostic in analysis.Diagnostics.Concat(analyzer.LintModule(analysis.Document))) { + if (diagnostics.Any(d => AreEqual(d, diagnostic))) { + yield return diagnostic; + } + } + + bool AreEqual(Diagnostic diagnostic1, DiagnosticsEntry diagnostic2) { + return diagnostic1.code == diagnostic2.ErrorCode && + diagnostic1.range.ToSourceSpan() == diagnostic2.SourceSpan; + } + } + + private interface ICodeActionProvider { + ImmutableArray FixableDiagnostics { get; } + Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellation); + } + + private sealed class MissingImportCodeActionProvider : ICodeActionProvider { + public static readonly ICodeActionProvider Instance = new MissingImportCodeActionProvider(); + + private MissingImportCodeActionProvider() { + } + + public ImmutableArray FixableDiagnostics => ImmutableArray.Create( + ErrorCodes.UndefinedVariable, ErrorCodes.VariableNotDefinedGlobally, ErrorCodes.VariableNotDefinedNonLocal); + + public Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { + // * TODO * for now, complete. since I don't know what each option mean. + var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); + + // * TODO * need to check whether it is expected node kind or add code context check to verify it is a place where module/type can appear + var node = finder.GetExpression(diagnostic.SourceSpan); + if (!(node is NameExpression)) { + return Task.FromResult>(Array.Empty()); + } + + var interpreter = analysis.Document.Interpreter; + var pathResolver = interpreter.ModuleResolution.CurrentPathResolver; + + var name = node.ToCodeString(analysis.Ast); + if (string.IsNullOrEmpty(name)) { + return Task.FromResult>(Array.Empty()); + } + + var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); + var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); + + var visited = new HashSet(); + var fullyQualifiedNames = new HashSet(); + + // find modules matching the given name + var fullModuleNames = pathResolver.GetAllImportableModuleByName(name, includeImplicit); + fullyQualifiedNames.UnionWith(fullModuleNames); + + // find members matching the given name from module already imported. + var nameParts = new List(); + foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { + nameParts.Add(module.Name); + CollectCandidates(module, name, visited, nameParts, fullyQualifiedNames); + nameParts.RemoveAt(nameParts.Count - 1); + } + + var codeActions = new List(); + foreach (var fullyQualifiedName in fullyQualifiedNames) { + codeActions.AddIfNotNull( + CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken), + CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); + } + + return Task.FromResult>(codeActions); + } + + private CodeAction CreateCodeAction(IDocumentAnalysis analysis, + Node node, + string fullyQualifiedName, + bool locallyInserted, + CancellationToken cancellationToken) { + var insertionPoint = GetInsertionPoint(analysis, node, fullyQualifiedName, locallyInserted, cancellationToken); + if (insertionPoint == null) { + return null; + } + + var insertionText = insertionPoint.Value.insertionText; + + // need to localize + var titleText = locallyInserted ? $"import locally \"{insertionText}\"" : insertionText; + + var sb = new StringBuilder(); + sb.Append(insertionPoint.Value.indentation); + sb.Append(insertionPoint.Value.addBlankLine ? insertionText + Environment.NewLine : insertionText); + sb.AppendIf(insertionPoint.Value.range.start == insertionPoint.Value.range.end, Environment.NewLine); + + var changes = new Dictionary {{ + analysis.Document.Uri, + new TextEdit[] { + new TextEdit() { + range = insertionPoint.Value.range, + newText = sb.ToString() + }} + }}; + + return new CodeAction() { + title = titleText, + edit = new WorkspaceEdit() { + changes = changes + } + }; + } + + private (bool addBlankLine, string insertionText, Range range, string indentation)? GetInsertionPoint(IDocumentAnalysis analysis, + Node node, + string fullyQualifiedModuleName, + bool locallyInserted, + CancellationToken cancellationToken) { + var (body, indentation) = GetStartingPoint(analysis, node, locallyInserted, cancellationToken); + if (body == null) { + // no insertion point + return null; + } + + var importNodes = body.GetChildNodes().Where(c => c is ImportStatement || c is FromImportStatement).ToList(); + var lastImportNode = importNodes.LastOrDefault(); + + // first check whether module name is dotted or not + var dotIndex = fullyQualifiedModuleName.LastIndexOf('.'); + if (dotIndex < 0) { + // there can't be existing import since we have the error + return (addBlankLine: lastImportNode == null, + $"import {fullyQualifiedModuleName}", + GetRange(analysis.Ast, body, lastImportNode), + indentation); + } + + // see whether there is existing from * import * statement. + var fromPart = fullyQualifiedModuleName.Substring(startIndex: 0, dotIndex); + var moduleNameToAdd = fullyQualifiedModuleName.Substring(dotIndex + 1); + foreach (var current in importNodes.Reverse().OfType()) { + if (current.Root.MakeString() == fromPart) { + return (addBlankLine: false, + GetInsertionText(current, fromPart, moduleNameToAdd), + current.GetSpan(analysis.Ast), + indentation); + } + } + + // add new from * import * statement + return (addBlankLine: lastImportNode == null, + $"from {fromPart} import {moduleNameToAdd}", + GetRange(analysis.Ast, body, lastImportNode), + indentation); + } + + public string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { + var imports = fromImportStatement.Names.Select(n => n.Name) + .Concat(new string[] { moduleNameToAdd }) + .OrderBy(n => n).ToList(); + return $"from {rootModuleName} import {string.Join(", ", imports)}"; + } + + public Range GetRange(PythonAst ast, Statement body, Node lastImportNode) { + var position = GetPosition(ast, body, lastImportNode); + return new Range() { start = position, end = position }; + } + + private Position GetPosition(PythonAst ast, Statement body, Node lastImportNode) { + if (lastImportNode != null) { + var endLocation = lastImportNode.GetEnd(ast); + return new Position { line = endLocation.Line, character = 0 }; + } + + // firstNode must exist in this context + var firstNode = body.GetChildNodes().First(); + return new Position() { line = firstNode.GetStart(ast).Line - 1, character = 0 }; + } + + private (Statement body, string indentation) GetStartingPoint(IDocumentAnalysis analysis, + Node node, + bool locallyInserted, + CancellationToken cancellationToken) { + if (!locallyInserted) { + return (analysis.Ast.Body, string.Empty); + } + + var candidate = GetParents(analysis.Ast.Body, node, cancellationToken).Where(p => p is FunctionDefinition).LastOrDefault(); + + // for now, only stop at FunctionDefinition. not sure what an appropriate other import scopes are. + // return closest scope from given node + switch (candidate) { + case FunctionDefinition functionDefinition: + return (functionDefinition.Body, GetIndentation(analysis.Ast, functionDefinition.Body)); + default: + // no local scope + return default; + } + } + + private string GetIndentation(PythonAst ast, Statement body) { + // first token must exist in current context + var firstToken = body.GetChildNodes().First(); + + // not sure how to handle a case where user is using "tab" instead of "space" + // for indentation. where can one get tab over indentation option? + return new string(' ', firstToken.GetStart(ast).Column - 1); + } + + private List GetParents(Node root, Node node, CancellationToken cancellationToken) { + var parentChain = new List(); + + // there seems no way to go up the parent chain. always has to go down from the top + while (root != null) { + cancellationToken.ThrowIfCancellationRequested(); + + var temp = root; + root = null; + + // this assumes node is not overlapped and children are ordered from left to right + // in textual position + foreach (var current in temp.GetChildNodes()) { + if (!current.IndexSpan.Contains(node.IndexSpan)) { + continue; + } + + parentChain.Add(current); + root = current; + break; + } + } + + return parentChain; + } + + private void CollectCandidates(IPythonModule module, + string name, + HashSet visited, + List nameParts, + HashSet fullyQualifiedNames) { + if (module == null || !visited.Add(module)) { + return; + } + + foreach (var memberName in module.GetMemberNames()) { + + // for now, skip any protected or private member + if (memberName.StartsWith("_")) { + continue; + } + + nameParts.Add(memberName); + var pythonType = module.GetMember(memberName) as IPythonType; + + if (pythonType != null && string.Equals(memberName, name)) { + fullyQualifiedNames.Add(string.Join('.', nameParts)); + } + + CollectCandidates(pythonType as IPythonModule, name, visited, nameParts, fullyQualifiedNames); + nameParts.RemoveAt(nameParts.Count - 1); + } + } + } + } +} diff --git a/src/Parsing/Impl/Ast/SourceLocationExtensions.cs b/src/Parsing/Impl/Ast/SourceLocationExtensions.cs index 9377fe70f..1c2ba6bd3 100644 --- a/src/Parsing/Impl/Ast/SourceLocationExtensions.cs +++ b/src/Parsing/Impl/Ast/SourceLocationExtensions.cs @@ -19,6 +19,15 @@ namespace Microsoft.Python.Parsing.Ast { public static class SourceLocationExtensions { public static int ToIndex(this SourceLocation location, ILocationConverter lc) => lc.LocationToIndex(location); + + public static SourceLocation ToSourceLocation(this Position position, ILocationConverter lc = null) { + var location = new SourceLocation(position.line + 1, position.character + 1); + if (lc == null) { + return location; + } + + return new SourceLocation(lc.LocationToIndex(location), location.Line, location.Column); + } } public static class SourceSpanExtensions { @@ -26,10 +35,17 @@ public static IndexSpan ToIndexSpan(this SourceSpan span, ILocationConverter lc) => IndexSpan.FromBounds(lc.LocationToIndex(span.Start), lc.LocationToIndex(span.End)); public static IndexSpan ToIndexSpan(this Range range, ILocationConverter lc) => IndexSpan.FromBounds(lc.LocationToIndex(range.start), lc.LocationToIndex(range.end)); + + public static SourceSpan ToSourceSpan(this Range range, ILocationConverter lc = null) { + return new SourceSpan(range.start.ToSourceLocation(lc), range.end.ToSourceLocation(lc)); + } } public static class IndexSpanExtensions { public static SourceSpan ToSourceSpan(this IndexSpan span, ILocationConverter lc) => lc != null ? new SourceSpan(lc.IndexToLocation(span.Start), lc.IndexToLocation(span.End)) : default; + public static bool Contains(this IndexSpan span, IndexSpan other) { + return span.Start <= other.Start && other.End <= span.End; + } } } From 4fa244098f52277af74c7b99beacf42d64559cd7 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 12:56:45 -0700 Subject: [PATCH 02/43] filter out unnecessary imports --- .../Impl/Sources/CodeActionSource.cs | 97 ++++++++++++++++--- 1 file changed, 82 insertions(+), 15 deletions(-) diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs index ddbed66f5..7e3b11ab1 100644 --- a/src/LanguageServer/Impl/Sources/CodeActionSource.cs +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -16,6 +16,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Runtime.CompilerServices; using System.Text; @@ -27,6 +28,7 @@ using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Diagnostics; using Microsoft.Python.Analysis.Types; +using Microsoft.Python.Analysis.Values; using Microsoft.Python.Core; using Microsoft.Python.Core.Collections; using Microsoft.Python.Core.Text; @@ -44,7 +46,7 @@ public CodeActionSource(IServiceContainer services) { _services = services; } - public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, Core.Text.Range range, Diagnostic[] diagnostics, CancellationToken cancellationToken) { + public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, Range range, Diagnostic[] diagnostics, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); var results = new List(); @@ -119,7 +121,7 @@ public Task> GetCodeActionAsync(IDocumentAnalysis analys var visited = new HashSet(); var fullyQualifiedNames = new HashSet(); - // find modules matching the given name + // find modules matching the given name. this will include submodules var fullModuleNames = pathResolver.GetAllImportableModuleByName(name, includeImplicit); fullyQualifiedNames.UnionWith(fullModuleNames); @@ -127,12 +129,14 @@ public Task> GetCodeActionAsync(IDocumentAnalysis analys var nameParts = new List(); foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { nameParts.Add(module.Name); - CollectCandidates(module, name, visited, nameParts, fullyQualifiedNames); + CollectCandidates(module, name, visited, nameParts, fullyQualifiedNames, cancellationToken); nameParts.RemoveAt(nameParts.Count - 1); } var codeActions = new List(); - foreach (var fullyQualifiedName in fullyQualifiedNames) { + foreach (var fullyQualifiedName in fullyQualifiedNames.OrderBy(n => n, ModuleNameComparer.Instance)) { + cancellationToken.ThrowIfCancellationRequested(); + codeActions.AddIfNotNull( CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken), CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); @@ -141,6 +145,26 @@ public Task> GetCodeActionAsync(IDocumentAnalysis analys return Task.FromResult>(codeActions); } + private class ModuleNameComparer : IComparer { + public static readonly ModuleNameComparer Instance = new ModuleNameComparer(); + + private ModuleNameComparer() { } + + public int Compare(string x, string y) { + if (x.StartsWith("_") && y.StartsWith("_")) { + return x.CompareTo(y); + } + if (x.StartsWith("_")) { + return 1; + } + if (y.StartsWith("_")) { + return -1; + } + + return x.CompareTo(y); + } + } + private CodeAction CreateCodeAction(IDocumentAnalysis analysis, Node node, string fullyQualifiedName, @@ -221,14 +245,14 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, indentation); } - public string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { + private string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { var imports = fromImportStatement.Names.Select(n => n.Name) .Concat(new string[] { moduleNameToAdd }) .OrderBy(n => n).ToList(); return $"from {rootModuleName} import {string.Join(", ", imports)}"; } - public Range GetRange(PythonAst ast, Statement body, Node lastImportNode) { + private Range GetRange(PythonAst ast, Statement body, Node lastImportNode) { var position = GetPosition(ast, body, lastImportNode); return new Range() { start = position, end = position }; } @@ -304,29 +328,72 @@ private void CollectCandidates(IPythonModule module, string name, HashSet visited, List nameParts, - HashSet fullyQualifiedNames) { + HashSet fullyQualifiedNames, + CancellationToken cancellationToken) { if (module == null || !visited.Add(module)) { return; } - foreach (var memberName in module.GetMemberNames()) { + // add non module (imported) member + AddNonImportedMemberWithName(module, name, nameParts, fullyQualifiedNames); + + // add module (imported) members if it shows up in __all__ + foreach (var memberName in GetAllVariables(module.Analysis)) { + cancellationToken.ThrowIfCancellationRequested(); - // for now, skip any protected or private member - if (memberName.StartsWith("_")) { + var pythonModule = module.GetMember(memberName) as IPythonModule; + if (pythonModule == null) { continue; } nameParts.Add(memberName); - var pythonType = module.GetMember(memberName) as IPythonType; - - if (pythonType != null && string.Equals(memberName, name)) { - fullyQualifiedNames.Add(string.Join('.', nameParts)); + if (string.Equals(memberName, name)) { + AddNameParts(nameParts, fullyQualifiedNames); } - CollectCandidates(pythonType as IPythonModule, name, visited, nameParts, fullyQualifiedNames); + CollectCandidates(pythonModule, name, visited, nameParts, fullyQualifiedNames, cancellationToken); nameParts.RemoveAt(nameParts.Count - 1); } } + + private void AddNonImportedMemberWithName(IPythonModule module, string name, List nameParts, HashSet fullyQualifiedNames) { + // for now, skip any protected or private member + if (name.StartsWith("_")) { + return; + } + + var pythonType = module.GetMember(name); + if (pythonType == null || pythonType is IPythonModule || pythonType.IsUnknown()) { + return; + } + + // skip any imported member unless it is explicitly on __all__ + if (module.Analysis.GlobalScope.Imported.TryGetVariable(name, out var imported) && + object.Equals(pythonType, imported.Value) && + GetAllVariables(module.Analysis).All(s => !string.Equals(s, name))) { + return; + } + + nameParts.Add(name); + AddNameParts(nameParts, fullyQualifiedNames); + nameParts.RemoveAt(nameParts.Count - 1); + } + + private static bool AddNameParts(List nameParts, HashSet fullyQualifiedNames) { + return fullyQualifiedNames.Add(string.Join('.', nameParts)); + } + + private IEnumerable GetAllVariables(IDocumentAnalysis analysis) { + if (analysis.GlobalScope.Variables.TryGetVariable("__all__", out var variable) && + variable?.Value is IPythonCollection collection) { + return collection.Contents + .OfType() + .Select(c => c.GetString()) + .Where(s => !string.IsNullOrEmpty(s)); + } + + return Array.Empty(); + } } } } From 8d5c968637f876e068e64b694797d58fe86990ca Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 12:57:01 -0700 Subject: [PATCH 03/43] removed unused usings --- src/LanguageServer/Impl/Sources/CodeActionSource.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs index 7e3b11ab1..e73885ae2 100644 --- a/src/LanguageServer/Impl/Sources/CodeActionSource.cs +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -16,9 +16,7 @@ using System; using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; using System.Linq; -using System.Runtime.CompilerServices; using System.Text; using System.Threading; using System.Threading.Tasks; From 9989184d9f41ec5f127013d637777e8861330bd5 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 13:23:59 -0700 Subject: [PATCH 04/43] more refactoring --- .../MissingImportCodeActionProvider.cs | 347 ++++++++++++++++++ .../Impl/Definitions/ICodeActionProvider.cs | 42 +++ src/LanguageServer/Impl/Resources.Designer.cs | 9 + src/LanguageServer/Impl/Resources.resx | 3 + .../Impl/Sources/CodeActionSource.cs | 330 +---------------- 5 files changed, 408 insertions(+), 323 deletions(-) create mode 100644 src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs create mode 100644 src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs new file mode 100644 index 000000000..4526db8ff --- /dev/null +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -0,0 +1,347 @@ +// 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, +// MERCHANTABILITY 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 System.Linq; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Python.Analysis; +using Microsoft.Python.Analysis.Analyzer.Expressions; +using Microsoft.Python.Analysis.Core.Interpreter; +using Microsoft.Python.Analysis.Diagnostics; +using Microsoft.Python.Analysis.Types; +using Microsoft.Python.Analysis.Values; +using Microsoft.Python.Core; +using Microsoft.Python.Core.Collections; +using Microsoft.Python.Core.Text; +using Microsoft.Python.LanguageServer.Protocol; +using Microsoft.Python.Parsing.Ast; +using Range = Microsoft.Python.Core.Text.Range; + +namespace Microsoft.Python.LanguageServer.CodeActions { + internal sealed class MissingImportCodeActionProvider : ICodeActionProvider { + public static readonly ICodeActionProvider Instance = new MissingImportCodeActionProvider(); + + private MissingImportCodeActionProvider() { + } + + public ImmutableArray FixableDiagnostics => ImmutableArray.Create( + ErrorCodes.UndefinedVariable, ErrorCodes.VariableNotDefinedGlobally, ErrorCodes.VariableNotDefinedNonLocal); + + public Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { + // * TODO * for now, complete. since I don't know what each option mean. + var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); + + // * TODO * need to check whether it is expected node kind or add code context check to verify it is a place where module/type can appear + var node = finder.GetExpression(diagnostic.SourceSpan); + if (!(node is NameExpression)) { + return Task.FromResult>(Array.Empty()); + } + + var interpreter = analysis.Document.Interpreter; + var pathResolver = interpreter.ModuleResolution.CurrentPathResolver; + + var name = node.ToCodeString(analysis.Ast); + if (string.IsNullOrEmpty(name)) { + return Task.FromResult>(Array.Empty()); + } + + var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); + var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); + + var visited = new HashSet(); + var fullyQualifiedNames = new HashSet(); + + // find modules matching the given name. this will include submodules + var fullModuleNames = pathResolver.GetAllImportableModuleByName(name, includeImplicit); + fullyQualifiedNames.UnionWith(fullModuleNames); + + // find members matching the given name from module already imported. + var nameParts = new List(); + foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { + nameParts.Add(module.Name); + CollectCandidates(module, name, visited, nameParts, fullyQualifiedNames, cancellationToken); + nameParts.RemoveAt(nameParts.Count - 1); + } + + var codeActions = new List(); + foreach (var fullyQualifiedName in fullyQualifiedNames.OrderBy(n => n, ModuleNameComparer.Instance)) { + cancellationToken.ThrowIfCancellationRequested(); + + codeActions.AddIfNotNull( + CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken), + CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); + } + + return Task.FromResult>(codeActions); + } + + private CodeAction CreateCodeAction(IDocumentAnalysis analysis, + Node node, + string fullyQualifiedName, + bool locallyInserted, + CancellationToken cancellationToken) { + var insertionPoint = GetInsertionPoint(analysis, node, fullyQualifiedName, locallyInserted, cancellationToken); + if (insertionPoint == null) { + return null; + } + + var insertionText = insertionPoint.Value.insertionText; + var titleText = locallyInserted ? string.Format(Resources.ImportLocally, insertionText) : insertionText; + + var sb = new StringBuilder(); + sb.Append(insertionPoint.Value.indentation); + sb.Append(insertionPoint.Value.addBlankLine ? insertionText + Environment.NewLine : insertionText); + sb.AppendIf(insertionPoint.Value.range.start == insertionPoint.Value.range.end, Environment.NewLine); + + var changes = new Dictionary {{ + analysis.Document.Uri, + new TextEdit[] { + new TextEdit() { + range = insertionPoint.Value.range, + newText = sb.ToString() + }} + }}; + + return new CodeAction() { + title = titleText, + edit = new WorkspaceEdit() { + changes = changes + } + }; + } + + private (bool addBlankLine, string insertionText, Range range, string indentation)? GetInsertionPoint(IDocumentAnalysis analysis, + Node node, + string fullyQualifiedModuleName, + bool locallyInserted, + CancellationToken cancellationToken) { + var (body, indentation) = GetStartingPoint(analysis, node, locallyInserted, cancellationToken); + if (body == null) { + // no insertion point + return null; + } + + var importNodes = body.GetChildNodes().Where(c => c is ImportStatement || c is FromImportStatement).ToList(); + var lastImportNode = importNodes.LastOrDefault(); + + // first check whether module name is dotted or not + var dotIndex = fullyQualifiedModuleName.LastIndexOf('.'); + if (dotIndex < 0) { + // there can't be existing import since we have the error + return (addBlankLine: lastImportNode == null, + $"import {fullyQualifiedModuleName}", + GetRange(analysis.Ast, body, lastImportNode), + indentation); + } + + // see whether there is existing from * import * statement. + var fromPart = fullyQualifiedModuleName.Substring(startIndex: 0, dotIndex); + var moduleNameToAdd = fullyQualifiedModuleName.Substring(dotIndex + 1); + foreach (var current in importNodes.Reverse().OfType()) { + if (current.Root.MakeString() == fromPart) { + return (addBlankLine: false, + GetInsertionText(current, fromPart, moduleNameToAdd), + current.GetSpan(analysis.Ast), + indentation); + } + } + + // add new from * import * statement + return (addBlankLine: lastImportNode == null, + $"from {fromPart} import {moduleNameToAdd}", + GetRange(analysis.Ast, body, lastImportNode), + indentation); + } + + private string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { + var imports = fromImportStatement.Names.Select(n => n.Name) + .Concat(new string[] { moduleNameToAdd }) + .OrderBy(n => n).ToList(); + return $"from {rootModuleName} import {string.Join(", ", imports)}"; + } + + private Range GetRange(PythonAst ast, Statement body, Node lastImportNode) { + var position = GetPosition(ast, body, lastImportNode); + return new Range() { start = position, end = position }; + } + + private Position GetPosition(PythonAst ast, Statement body, Node lastImportNode) { + if (lastImportNode != null) { + var endLocation = lastImportNode.GetEnd(ast); + return new Position { line = endLocation.Line, character = 0 }; + } + + // firstNode must exist in this context + var firstNode = body.GetChildNodes().First(); + return new Position() { line = firstNode.GetStart(ast).Line - 1, character = 0 }; + } + + private (Statement body, string indentation) GetStartingPoint(IDocumentAnalysis analysis, + Node node, + bool locallyInserted, + CancellationToken cancellationToken) { + if (!locallyInserted) { + return (analysis.Ast.Body, string.Empty); + } + + var candidate = GetParents(analysis.Ast.Body, node, cancellationToken).Where(p => p is FunctionDefinition).LastOrDefault(); + + // for now, only stop at FunctionDefinition. + // we can expand it to more scope if we want but this seems what other tool also provide as well. + // this will return closest scope from given node + switch (candidate) { + case FunctionDefinition functionDefinition: + return (functionDefinition.Body, GetIndentation(analysis.Ast, functionDefinition.Body)); + default: + // no local scope + return default; + } + } + + private string GetIndentation(PythonAst ast, Statement body) { + // first token must exist in current context + var firstToken = body.GetChildNodes().First(); + + // not sure how to handle a case where user is using "tab" instead of "space" + // for indentation. where can one get tab over indentation option? + return new string(' ', firstToken.GetStart(ast).Column - 1); + } + + private List GetParents(Node root, Node node, CancellationToken cancellationToken) { + var parentChain = new List(); + + // there seems no way to go up the parent chain. always has to go down from the top + while (root != null) { + cancellationToken.ThrowIfCancellationRequested(); + + var temp = root; + root = null; + + // this assumes node is not overlapped and children are ordered from left to right + // in textual position + foreach (var current in temp.GetChildNodes()) { + if (!current.IndexSpan.Contains(node.IndexSpan)) { + continue; + } + + parentChain.Add(current); + root = current; + break; + } + } + + return parentChain; + } + + private void CollectCandidates(IPythonModule module, + string name, + HashSet visited, + List nameParts, + HashSet fullyQualifiedNames, + CancellationToken cancellationToken) { + if (module == null || !visited.Add(module)) { + return; + } + + // add non module (imported) member + AddNonImportedMemberWithName(module, name, nameParts, fullyQualifiedNames); + + // add module (imported) members if it shows up in __all__ + foreach (var memberName in GetAllVariables(module.Analysis)) { + cancellationToken.ThrowIfCancellationRequested(); + + var pythonModule = module.GetMember(memberName) as IPythonModule; + if (pythonModule == null) { + continue; + } + + nameParts.Add(memberName); + if (string.Equals(memberName, name)) { + AddNameParts(nameParts, fullyQualifiedNames); + } + + CollectCandidates(pythonModule, name, visited, nameParts, fullyQualifiedNames, cancellationToken); + nameParts.RemoveAt(nameParts.Count - 1); + } + } + + private void AddNonImportedMemberWithName(IPythonModule module, string name, List nameParts, HashSet fullyQualifiedNames) { + // for now, skip any protected or private member + if (name.StartsWith("_")) { + return; + } + + var pythonType = module.GetMember(name); + if (pythonType == null || pythonType is IPythonModule || pythonType.IsUnknown()) { + return; + } + + // skip any imported member unless it is explicitly on __all__ + if (module.Analysis.GlobalScope.Imported.TryGetVariable(name, out var imported) && + object.Equals(pythonType, imported.Value) && + GetAllVariables(module.Analysis).All(s => !string.Equals(s, name))) { + return; + } + + nameParts.Add(name); + AddNameParts(nameParts, fullyQualifiedNames); + nameParts.RemoveAt(nameParts.Count - 1); + } + + private static bool AddNameParts(List nameParts, HashSet fullyQualifiedNames) { + return fullyQualifiedNames.Add(string.Join('.', nameParts)); + } + + private IEnumerable GetAllVariables(IDocumentAnalysis analysis) { + // this is different than StartImportMemberNames since that only returns something when + // all entries are known. for import, we are fine doing best effort + if (analysis.GlobalScope.Variables.TryGetVariable("__all__", out var variable) && + variable?.Value is IPythonCollection collection) { + return collection.Contents + .OfType() + .Select(c => c.GetString()) + .Where(s => !string.IsNullOrEmpty(s)); + } + + return Array.Empty(); + } + + private class ModuleNameComparer : IComparer { + public static readonly ModuleNameComparer Instance = new ModuleNameComparer(); + + private ModuleNameComparer() { } + + public int Compare(string x, string y) { + // move "_" to back of the list + if (x.StartsWith("_") && y.StartsWith("_")) { + return x.CompareTo(y); + } + if (x.StartsWith("_")) { + return 1; + } + if (y.StartsWith("_")) { + return -1; + } + + return x.CompareTo(y); + } + } + } +} + diff --git a/src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs b/src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs new file mode 100644 index 000000000..95cb4ba0b --- /dev/null +++ b/src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs @@ -0,0 +1,42 @@ +// 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, +// MERCHANTABILITY OR NON-INFRINGEMENT. +// +// See the Apache Version 2.0 License for specific language governing +// permissions and limitations under the License. + + +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Python.Analysis; +using Microsoft.Python.Analysis.Diagnostics; +using Microsoft.Python.Core.Collections; +using Microsoft.Python.LanguageServer.Protocol; + +namespace Microsoft.Python.LanguageServer { + public interface ICodeActionProvider { + /// + /// Returns error code this code action can provide fix for. this error code must be same as ones that are reported to host as diagnostics + /// ex) error code from linter + /// + ImmutableArray FixableDiagnostics { get; } + + /// + /// Returns that can potentially fix given diagnostic + /// + /// of the file where reported + /// that code action is supposed to fix + /// + /// that can fix the given + Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellation); + } +} diff --git a/src/LanguageServer/Impl/Resources.Designer.cs b/src/LanguageServer/Impl/Resources.Designer.cs index b413d0dde..07cdc2c42 100644 --- a/src/LanguageServer/Impl/Resources.Designer.cs +++ b/src/LanguageServer/Impl/Resources.Designer.cs @@ -105,6 +105,15 @@ internal static string Error_InvalidCachePath { } } + /// + /// Looks up a localized string similar to import locally '{0}'. + /// + internal static string ImportLocally { + get { + return ResourceManager.GetString("ImportLocally", resourceCulture); + } + } + /// /// Looks up a localized string similar to Initializing for generic interpreter. /// diff --git a/src/LanguageServer/Impl/Resources.resx b/src/LanguageServer/Impl/Resources.resx index e060c233d..fa819a42d 100644 --- a/src/LanguageServer/Impl/Resources.resx +++ b/src/LanguageServer/Impl/Resources.resx @@ -132,6 +132,9 @@ Specified cache folder does not exist. Switching to default. + + import locally '{0}' + Initializing for generic interpreter diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs index e73885ae2..84d8adc5c 100644 --- a/src/LanguageServer/Impl/Sources/CodeActionSource.cs +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -14,32 +14,27 @@ // permissions and limitations under the License. -using System; using System.Collections.Generic; using System.Linq; -using System.Text; using System.Threading; using System.Threading.Tasks; using Microsoft.Python.Analysis; using Microsoft.Python.Analysis.Analyzer; -using Microsoft.Python.Analysis.Analyzer.Expressions; -using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Diagnostics; -using Microsoft.Python.Analysis.Types; -using Microsoft.Python.Analysis.Values; using Microsoft.Python.Core; using Microsoft.Python.Core.Collections; -using Microsoft.Python.Core.Text; +using Microsoft.Python.LanguageServer.CodeActions; using Microsoft.Python.LanguageServer.Protocol; using Microsoft.Python.Parsing.Ast; using Range = Microsoft.Python.Core.Text.Range; namespace Microsoft.Python.LanguageServer.Sources { - internal sealed class CodeActionSource { - private readonly IServiceContainer _services; - private readonly ImmutableArray _codeActionProviders = + internal sealed partial class CodeActionSource { + private static readonly ImmutableArray _codeActionProviders = ImmutableArray.Create(MissingImportCodeActionProvider.Instance); + private readonly IServiceContainer _services; + public CodeActionSource(IServiceContainer services) { _services = services; } @@ -70,6 +65,8 @@ public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, private IEnumerable GetMatchingDiagnostics(IDocumentAnalysis analysis, Diagnostic[] diagnostics, CancellationToken cancellationToken) { var analyzer = _services.GetService(); foreach (var diagnostic in analysis.Diagnostics.Concat(analyzer.LintModule(analysis.Document))) { + cancellationToken.ThrowIfCancellationRequested(); + if (diagnostics.Any(d => AreEqual(d, diagnostic))) { yield return diagnostic; } @@ -80,318 +77,5 @@ bool AreEqual(Diagnostic diagnostic1, DiagnosticsEntry diagnostic2) { diagnostic1.range.ToSourceSpan() == diagnostic2.SourceSpan; } } - - private interface ICodeActionProvider { - ImmutableArray FixableDiagnostics { get; } - Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellation); - } - - private sealed class MissingImportCodeActionProvider : ICodeActionProvider { - public static readonly ICodeActionProvider Instance = new MissingImportCodeActionProvider(); - - private MissingImportCodeActionProvider() { - } - - public ImmutableArray FixableDiagnostics => ImmutableArray.Create( - ErrorCodes.UndefinedVariable, ErrorCodes.VariableNotDefinedGlobally, ErrorCodes.VariableNotDefinedNonLocal); - - public Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { - // * TODO * for now, complete. since I don't know what each option mean. - var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); - - // * TODO * need to check whether it is expected node kind or add code context check to verify it is a place where module/type can appear - var node = finder.GetExpression(diagnostic.SourceSpan); - if (!(node is NameExpression)) { - return Task.FromResult>(Array.Empty()); - } - - var interpreter = analysis.Document.Interpreter; - var pathResolver = interpreter.ModuleResolution.CurrentPathResolver; - - var name = node.ToCodeString(analysis.Ast); - if (string.IsNullOrEmpty(name)) { - return Task.FromResult>(Array.Empty()); - } - - var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); - var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); - - var visited = new HashSet(); - var fullyQualifiedNames = new HashSet(); - - // find modules matching the given name. this will include submodules - var fullModuleNames = pathResolver.GetAllImportableModuleByName(name, includeImplicit); - fullyQualifiedNames.UnionWith(fullModuleNames); - - // find members matching the given name from module already imported. - var nameParts = new List(); - foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { - nameParts.Add(module.Name); - CollectCandidates(module, name, visited, nameParts, fullyQualifiedNames, cancellationToken); - nameParts.RemoveAt(nameParts.Count - 1); - } - - var codeActions = new List(); - foreach (var fullyQualifiedName in fullyQualifiedNames.OrderBy(n => n, ModuleNameComparer.Instance)) { - cancellationToken.ThrowIfCancellationRequested(); - - codeActions.AddIfNotNull( - CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken), - CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); - } - - return Task.FromResult>(codeActions); - } - - private class ModuleNameComparer : IComparer { - public static readonly ModuleNameComparer Instance = new ModuleNameComparer(); - - private ModuleNameComparer() { } - - public int Compare(string x, string y) { - if (x.StartsWith("_") && y.StartsWith("_")) { - return x.CompareTo(y); - } - if (x.StartsWith("_")) { - return 1; - } - if (y.StartsWith("_")) { - return -1; - } - - return x.CompareTo(y); - } - } - - private CodeAction CreateCodeAction(IDocumentAnalysis analysis, - Node node, - string fullyQualifiedName, - bool locallyInserted, - CancellationToken cancellationToken) { - var insertionPoint = GetInsertionPoint(analysis, node, fullyQualifiedName, locallyInserted, cancellationToken); - if (insertionPoint == null) { - return null; - } - - var insertionText = insertionPoint.Value.insertionText; - - // need to localize - var titleText = locallyInserted ? $"import locally \"{insertionText}\"" : insertionText; - - var sb = new StringBuilder(); - sb.Append(insertionPoint.Value.indentation); - sb.Append(insertionPoint.Value.addBlankLine ? insertionText + Environment.NewLine : insertionText); - sb.AppendIf(insertionPoint.Value.range.start == insertionPoint.Value.range.end, Environment.NewLine); - - var changes = new Dictionary {{ - analysis.Document.Uri, - new TextEdit[] { - new TextEdit() { - range = insertionPoint.Value.range, - newText = sb.ToString() - }} - }}; - - return new CodeAction() { - title = titleText, - edit = new WorkspaceEdit() { - changes = changes - } - }; - } - - private (bool addBlankLine, string insertionText, Range range, string indentation)? GetInsertionPoint(IDocumentAnalysis analysis, - Node node, - string fullyQualifiedModuleName, - bool locallyInserted, - CancellationToken cancellationToken) { - var (body, indentation) = GetStartingPoint(analysis, node, locallyInserted, cancellationToken); - if (body == null) { - // no insertion point - return null; - } - - var importNodes = body.GetChildNodes().Where(c => c is ImportStatement || c is FromImportStatement).ToList(); - var lastImportNode = importNodes.LastOrDefault(); - - // first check whether module name is dotted or not - var dotIndex = fullyQualifiedModuleName.LastIndexOf('.'); - if (dotIndex < 0) { - // there can't be existing import since we have the error - return (addBlankLine: lastImportNode == null, - $"import {fullyQualifiedModuleName}", - GetRange(analysis.Ast, body, lastImportNode), - indentation); - } - - // see whether there is existing from * import * statement. - var fromPart = fullyQualifiedModuleName.Substring(startIndex: 0, dotIndex); - var moduleNameToAdd = fullyQualifiedModuleName.Substring(dotIndex + 1); - foreach (var current in importNodes.Reverse().OfType()) { - if (current.Root.MakeString() == fromPart) { - return (addBlankLine: false, - GetInsertionText(current, fromPart, moduleNameToAdd), - current.GetSpan(analysis.Ast), - indentation); - } - } - - // add new from * import * statement - return (addBlankLine: lastImportNode == null, - $"from {fromPart} import {moduleNameToAdd}", - GetRange(analysis.Ast, body, lastImportNode), - indentation); - } - - private string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { - var imports = fromImportStatement.Names.Select(n => n.Name) - .Concat(new string[] { moduleNameToAdd }) - .OrderBy(n => n).ToList(); - return $"from {rootModuleName} import {string.Join(", ", imports)}"; - } - - private Range GetRange(PythonAst ast, Statement body, Node lastImportNode) { - var position = GetPosition(ast, body, lastImportNode); - return new Range() { start = position, end = position }; - } - - private Position GetPosition(PythonAst ast, Statement body, Node lastImportNode) { - if (lastImportNode != null) { - var endLocation = lastImportNode.GetEnd(ast); - return new Position { line = endLocation.Line, character = 0 }; - } - - // firstNode must exist in this context - var firstNode = body.GetChildNodes().First(); - return new Position() { line = firstNode.GetStart(ast).Line - 1, character = 0 }; - } - - private (Statement body, string indentation) GetStartingPoint(IDocumentAnalysis analysis, - Node node, - bool locallyInserted, - CancellationToken cancellationToken) { - if (!locallyInserted) { - return (analysis.Ast.Body, string.Empty); - } - - var candidate = GetParents(analysis.Ast.Body, node, cancellationToken).Where(p => p is FunctionDefinition).LastOrDefault(); - - // for now, only stop at FunctionDefinition. not sure what an appropriate other import scopes are. - // return closest scope from given node - switch (candidate) { - case FunctionDefinition functionDefinition: - return (functionDefinition.Body, GetIndentation(analysis.Ast, functionDefinition.Body)); - default: - // no local scope - return default; - } - } - - private string GetIndentation(PythonAst ast, Statement body) { - // first token must exist in current context - var firstToken = body.GetChildNodes().First(); - - // not sure how to handle a case where user is using "tab" instead of "space" - // for indentation. where can one get tab over indentation option? - return new string(' ', firstToken.GetStart(ast).Column - 1); - } - - private List GetParents(Node root, Node node, CancellationToken cancellationToken) { - var parentChain = new List(); - - // there seems no way to go up the parent chain. always has to go down from the top - while (root != null) { - cancellationToken.ThrowIfCancellationRequested(); - - var temp = root; - root = null; - - // this assumes node is not overlapped and children are ordered from left to right - // in textual position - foreach (var current in temp.GetChildNodes()) { - if (!current.IndexSpan.Contains(node.IndexSpan)) { - continue; - } - - parentChain.Add(current); - root = current; - break; - } - } - - return parentChain; - } - - private void CollectCandidates(IPythonModule module, - string name, - HashSet visited, - List nameParts, - HashSet fullyQualifiedNames, - CancellationToken cancellationToken) { - if (module == null || !visited.Add(module)) { - return; - } - - // add non module (imported) member - AddNonImportedMemberWithName(module, name, nameParts, fullyQualifiedNames); - - // add module (imported) members if it shows up in __all__ - foreach (var memberName in GetAllVariables(module.Analysis)) { - cancellationToken.ThrowIfCancellationRequested(); - - var pythonModule = module.GetMember(memberName) as IPythonModule; - if (pythonModule == null) { - continue; - } - - nameParts.Add(memberName); - if (string.Equals(memberName, name)) { - AddNameParts(nameParts, fullyQualifiedNames); - } - - CollectCandidates(pythonModule, name, visited, nameParts, fullyQualifiedNames, cancellationToken); - nameParts.RemoveAt(nameParts.Count - 1); - } - } - - private void AddNonImportedMemberWithName(IPythonModule module, string name, List nameParts, HashSet fullyQualifiedNames) { - // for now, skip any protected or private member - if (name.StartsWith("_")) { - return; - } - - var pythonType = module.GetMember(name); - if (pythonType == null || pythonType is IPythonModule || pythonType.IsUnknown()) { - return; - } - - // skip any imported member unless it is explicitly on __all__ - if (module.Analysis.GlobalScope.Imported.TryGetVariable(name, out var imported) && - object.Equals(pythonType, imported.Value) && - GetAllVariables(module.Analysis).All(s => !string.Equals(s, name))) { - return; - } - - nameParts.Add(name); - AddNameParts(nameParts, fullyQualifiedNames); - nameParts.RemoveAt(nameParts.Count - 1); - } - - private static bool AddNameParts(List nameParts, HashSet fullyQualifiedNames) { - return fullyQualifiedNames.Add(string.Join('.', nameParts)); - } - - private IEnumerable GetAllVariables(IDocumentAnalysis analysis) { - if (analysis.GlobalScope.Variables.TryGetVariable("__all__", out var variable) && - variable?.Value is IPythonCollection collection) { - return collection.Contents - .OfType() - .Select(c => c.GetString()) - .Where(s => !string.IsNullOrEmpty(s)); - } - - return Array.Empty(); - } - } } } From c90dfc0d2b20f5f6972216eab809d43fec18ea17 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 14:49:10 -0700 Subject: [PATCH 05/43] addressed PR feedbacks --- .../Ast/Impl/Modules/Resolution/MainModuleResolution.cs | 2 ++ .../Impl/DependencyResolution/PathResolverSnapshot.cs | 8 ++++---- src/Core/Impl/Text/Position.cs | 8 +------- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 2 +- src/LanguageServer/Impl/Implementation/Server.Editor.cs | 5 ----- src/LanguageServer/Impl/Protocol/Messages.cs | 6 ------ 6 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs b/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs index 055c5ad04..0ec74a4af 100644 --- a/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs +++ b/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs @@ -57,12 +57,14 @@ public MainModuleResolution(string root, IServiceContainer services, ImmutableAr public IEnumerable GetImportedModules(CancellationToken cancellationToken) { foreach (var moduleRef in Modules.Values) { + cancellationToken.ThrowIfCancellationRequested(); if (moduleRef.Value != null) { yield return moduleRef.Value; } } foreach (var module in _specialized.Values) { + cancellationToken.ThrowIfCancellationRequested(); yield return module; } } diff --git a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs index 2d17c2ff9..d0bbce3f1 100644 --- a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs +++ b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs @@ -82,14 +82,14 @@ private PathResolverSnapshot(PythonLanguageVersion pythonLanguageVersion, string } public ImmutableArray GetAllImportableModuleNames(bool includeImplicitPackages = true) { - return GetAllImportableModuleFullName(n => !string.IsNullOrEmpty(n.FullModuleName), includeImplicitPackages); + return GetAllImportableModuleFullNames(n => !string.IsNullOrEmpty(n.FullModuleName), includeImplicitPackages); } - public ImmutableArray GetAllImportableModuleByName(string name, bool includeImplicitPackages = true) { - return GetAllImportableModuleFullName(n => string.Equals(n.Name, name), includeImplicitPackages); + public ImmutableArray GetAllImportableModulesByName(string name, bool includeImplicitPackages = true) { + return GetAllImportableModuleFullNames(n => string.Equals(n.Name, name), includeImplicitPackages); } - private ImmutableArray GetAllImportableModuleFullName(Func predicate, bool includeImplicitPackages = true) { + private ImmutableArray GetAllImportableModuleFullNames(Func predicate, bool includeImplicitPackages = true) { var roots = _roots.Prepend(_nonRooted); var items = new Queue(roots); var names = ImmutableArray.Empty; diff --git a/src/Core/Impl/Text/Position.cs b/src/Core/Impl/Text/Position.cs index 08b38569d..5f0a4de11 100644 --- a/src/Core/Impl/Text/Position.cs +++ b/src/Core/Impl/Text/Position.cs @@ -44,13 +44,7 @@ public struct Position : IEquatable { public bool Equals(Position other) => line == other.line && character == other.character; - public override bool Equals(object obj) { - if (obj is Position other) { - return Equals(other); - } - - return false; - } + public override bool Equals(object obj) => obj is Position other ? Equals(other) : false; public override int GetHashCode() => 0; public override string ToString() => $"({line}, {character})"; diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 4526db8ff..48b64340d 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -68,7 +68,7 @@ public Task> GetCodeActionAsync(IDocumentAnalysis analys var fullyQualifiedNames = new HashSet(); // find modules matching the given name. this will include submodules - var fullModuleNames = pathResolver.GetAllImportableModuleByName(name, includeImplicit); + var fullModuleNames = pathResolver.GetAllImportableModulesByName(name, includeImplicit); fullyQualifiedNames.UnionWith(fullModuleNames); // find members matching the given name from module already imported. diff --git a/src/LanguageServer/Impl/Implementation/Server.Editor.cs b/src/LanguageServer/Impl/Implementation/Server.Editor.cs index b566015e6..2112321ad 100644 --- a/src/LanguageServer/Impl/Implementation/Server.Editor.cs +++ b/src/LanguageServer/Impl/Implementation/Server.Editor.cs @@ -121,11 +121,6 @@ public async Task CodeAction(CodeActionParams @params, Cancellatio return Array.Empty(); } - // * NOTE * CodeActionParams.CodeActionContext._version is always null. PublishDiagnostic doesn't send version - // so not sure where that version is supposed to come from. and not sure whether what version it is - // supposed to be. text version? analysis version? - // we might want to check version at some point and return no code action if we are called with - // staled diagnostics var analysis = await Document.GetAnalysisAsync(uri, Services, CompletionAnalysisTimeout, cancellationToken); var codeActions = await new CodeActionSource(Services).GetCodeActionsAsync(analysis, @params.range, @params.context.diagnostics, cancellationToken); return codeActions ?? Array.Empty(); diff --git a/src/LanguageServer/Impl/Protocol/Messages.cs b/src/LanguageServer/Impl/Protocol/Messages.cs index ab253a760..d8270543f 100644 --- a/src/LanguageServer/Impl/Protocol/Messages.cs +++ b/src/LanguageServer/Impl/Protocol/Messages.cs @@ -190,12 +190,6 @@ public struct CodeActionParams { [Serializable] public sealed class CodeActionContext { public Diagnostic[] diagnostics; - - /// - /// The intended version that diagnostic locations apply to. The request may - /// fail if the server cannot map correctly. - /// - public int? _version; } [Serializable] From 2868a1a97a63465ba6d509a69ca133e1308d4756 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 17:12:34 -0700 Subject: [PATCH 06/43] added initial tests --- .../Impl/Diagnostics/DiagnosticExtensions.cs | 45 ++++ .../Impl/Diagnostics/DiagnosticsService.cs | 28 +- .../Impl/Implementation/Server.Editor.cs | 2 +- .../Impl/Sources/CodeActionSource.cs | 2 +- .../Test/MissingImportCodeActionTests.cs | 116 +++++++++ .../Impl/Ast/SourceLocationExtensions.cs | 13 +- src/UnitTests/Core/Impl/MarkupUtils.cs | 244 ++++++++++++++++++ src/UnitTests/Core/Impl/UnitTests.Core.csproj | 3 + 8 files changed, 418 insertions(+), 35 deletions(-) create mode 100644 src/LanguageServer/Impl/Diagnostics/DiagnosticExtensions.cs create mode 100644 src/LanguageServer/Test/MissingImportCodeActionTests.cs create mode 100644 src/UnitTests/Core/Impl/MarkupUtils.cs diff --git a/src/LanguageServer/Impl/Diagnostics/DiagnosticExtensions.cs b/src/LanguageServer/Impl/Diagnostics/DiagnosticExtensions.cs new file mode 100644 index 000000000..6a13be8cf --- /dev/null +++ b/src/LanguageServer/Impl/Diagnostics/DiagnosticExtensions.cs @@ -0,0 +1,45 @@ +// 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, +// MERCHANTABILITY OR NON-INFRINGEMENT. +// +// See the Apache Version 2.0 License for specific language governing +// permissions and limitations under the License. + +using Microsoft.Python.Analysis.Diagnostics; +using Microsoft.Python.LanguageServer.Protocol; +using Microsoft.Python.Parsing; + +namespace Microsoft.Python.LanguageServer.Diagnostics { + internal static class DiagnosticExtensions { + public static Diagnostic ToDiagnostic(this DiagnosticsEntry diagnostic, string source = "Python") { + return new Diagnostic { + range = diagnostic.SourceSpan, + severity = diagnostic.Severity.ToDiagnosticSeverity(), + source = source, + code = diagnostic.ErrorCode, + message = diagnostic.Message, + }; + } + + public static DiagnosticSeverity ToDiagnosticSeverity(this Severity severity) { + switch (severity) { + case Severity.Warning: + return DiagnosticSeverity.Warning; + case Severity.Information: + return DiagnosticSeverity.Information; + case Severity.Hint: + return DiagnosticSeverity.Hint; + default: + return DiagnosticSeverity.Error; + } + } + } +} diff --git a/src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs b/src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs index fd28ecae7..ff72121b8 100644 --- a/src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs +++ b/src/LanguageServer/Impl/Diagnostics/DiagnosticsService.cs @@ -162,7 +162,7 @@ private void PublishDiagnostics() { var parameters = new PublishDiagnosticsParams { uri = kvp.Key, diagnostics = Rdt.GetDocument(kvp.Key)?.IsOpen == true - ? FilterBySeverityMap(kvp.Value).Select(ToDiagnostic).ToArray() + ? FilterBySeverityMap(kvp.Value).Select(d => d.ToDiagnostic()).ToArray() : Array.Empty() }; _clientApp.NotifyWithParameterObjectAsync("textDocument/publishDiagnostics", parameters).DoNotWait(); @@ -176,32 +176,6 @@ private void ClearAllDiagnostics() { } } - private static Diagnostic ToDiagnostic(DiagnosticsEntry e) { - DiagnosticSeverity s; - switch (e.Severity) { - case Severity.Warning: - s = DiagnosticSeverity.Warning; - break; - case Severity.Information: - s = DiagnosticSeverity.Information; - break; - case Severity.Hint: - s = DiagnosticSeverity.Hint; - break; - default: - s = DiagnosticSeverity.Error; - break; - } - - return new Diagnostic { - range = e.SourceSpan, - severity = s, - source = "Python", - code = e.ErrorCode, - message = e.Message, - }; - } - private IEnumerable FilterBySeverityMap(DocumentDiagnostics d) => d.Entries .SelectMany(kvp => kvp.Value) diff --git a/src/LanguageServer/Impl/Implementation/Server.Editor.cs b/src/LanguageServer/Impl/Implementation/Server.Editor.cs index 2112321ad..c593eb69c 100644 --- a/src/LanguageServer/Impl/Implementation/Server.Editor.cs +++ b/src/LanguageServer/Impl/Implementation/Server.Editor.cs @@ -122,7 +122,7 @@ public async Task CodeAction(CodeActionParams @params, Cancellatio } var analysis = await Document.GetAnalysisAsync(uri, Services, CompletionAnalysisTimeout, cancellationToken); - var codeActions = await new CodeActionSource(Services).GetCodeActionsAsync(analysis, @params.range, @params.context.diagnostics, cancellationToken); + var codeActions = await new CodeActionSource(Services).GetCodeActionsAsync(analysis, @params.context.diagnostics, cancellationToken); return codeActions ?? Array.Empty(); } } diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs index 84d8adc5c..01d6e0c68 100644 --- a/src/LanguageServer/Impl/Sources/CodeActionSource.cs +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -39,7 +39,7 @@ public CodeActionSource(IServiceContainer services) { _services = services; } - public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, Range range, Diagnostic[] diagnostics, CancellationToken cancellationToken) { + public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, Diagnostic[] diagnostics, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); var results = new List(); diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs new file mode 100644 index 000000000..5cadddbd6 --- /dev/null +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -0,0 +1,116 @@ +// 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, +// MERCHANTABILITY OR NON-INFRINGEMENT. +// +// See the Apache Version 2.0 License for specific language governing +// permissions and limitations under the License. + +using System; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using FluentAssertions; +using Microsoft.Python.Analysis; +using Microsoft.Python.Analysis.Analyzer; +using Microsoft.Python.Core.Text; +using Microsoft.Python.LanguageServer.Protocol; +using Microsoft.Python.LanguageServer.Sources; +using Microsoft.Python.LanguageServer.Tests.FluentAssertions; +using Microsoft.Python.Parsing.Ast; +using Microsoft.Python.Parsing.Tests; +using Microsoft.Python.UnitTests.Core; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using TestUtilities; +using Microsoft.Python.LanguageServer.Diagnostics; +using Microsoft.Python.LanguageServer.CodeActions; +using System.Collections.Generic; + +namespace Microsoft.Python.LanguageServer.Tests { + [TestClass] + public class MissingImportCodeActionTests : LanguageServerTestBase { + public TestContext TestContext { get; set; } + + [TestInitialize] + public void TestInitialize() + => TestEnvironmentImpl.TestInitialize($"{TestContext.FullyQualifiedTestClassName}.{TestContext.TestName}"); + + [TestCleanup] + public void Cleanup() => TestEnvironmentImpl.TestCleanup(); + + [TestMethod, Priority(0)] + public async Task TopModule() { + const string markup = @"{|insertionSpan:|}{|diagnostic:ntpath|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + var codeAction = codeActions.Single(); + var newText = "import ntpath" + Environment.NewLine + Environment.NewLine; + TestCodeAction(analysis.Document.Uri, codeAction, title: "import ntpath", insertionSpan, newText); + } + + [TestMethod, Priority(0)] + public async Task TopModuleFunctionDefinition() { + const string markup = @"{|insertionSpan:|}def TestMethod(): + {|diagnostic:ntpath|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + codeActions.Should().HaveCount(2); + + var codeAction = codeActions.First(); + var newText = "import ntpath" + Environment.NewLine + Environment.NewLine; + TestCodeAction(analysis.Document.Uri, codeAction, title: "import ntpath", insertionSpan, newText); + } + + [TestMethod, Priority(0)] + public async Task TopModuleFunctionDefinitionLocally() { + const string markup = @"def TestMethod(): +{|insertionSpan:|} {|diagnostic:ntpath|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + codeActions.Should().HaveCount(2); + + var codeAction = codeActions[1]; + var newText = " import ntpath" + Environment.NewLine + Environment.NewLine; + TestCodeAction(analysis.Document.Uri, codeAction, title: string.Format(Resources.ImportLocally, "import ntpath"), insertionSpan, newText); + } + + private async Task<(IDocumentAnalysis analysis, CodeAction[] diagnostics, SourceSpan insertionSpan)> GetAnalysisAndCodeActionsAndSpanAsync( + string markup, IEnumerable codes) { + MarkupUtiles.GetNamedSpans(markup, out var code, out var spans); + + var analysis = await GetAnalysisAsync(code); + var insertionSpan = spans["insertionSpan"].First().ToSourceSpan(analysis.Ast); + + var diagnostics = GetDiagnostics(analysis, spans["diagnostic"].First().ToSourceSpan(analysis.Ast), codes); + var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); + return (analysis, codeActions.ToArray(), insertionSpan); + } + + private static void TestCodeAction(Uri uri, CodeAction codeAction, string title, Core.Text.Range insertedSpan, string newText) { + codeAction.title.Should().Be(title); + codeAction.edit.changes.Should().HaveCount(1); + + var edit = codeAction.edit.changes[uri]; + edit.Single().range.Should().Be(insertedSpan); + edit.Single().newText.Should().Be(newText); + } + + private static Diagnostic[] GetDiagnostics(IDocumentAnalysis analysis, SourceSpan span, IEnumerable codes) { + var analyzer = analysis.ExpressionEvaluator.Services.GetService(); + return analyzer.LintModule(analysis.Document) + .Where(d => d.SourceSpan == span && codes.Any(e => string.Equals(e, d.ErrorCode))) + .Select(d => d.ToDiagnostic()) + .ToArray(); + } + } +} diff --git a/src/Parsing/Impl/Ast/SourceLocationExtensions.cs b/src/Parsing/Impl/Ast/SourceLocationExtensions.cs index 1c2ba6bd3..157c9b127 100644 --- a/src/Parsing/Impl/Ast/SourceLocationExtensions.cs +++ b/src/Parsing/Impl/Ast/SourceLocationExtensions.cs @@ -30,15 +30,16 @@ public static SourceLocation ToSourceLocation(this Position position, ILocationC } } - public static class SourceSpanExtensions { - public static IndexSpan ToIndexSpan(this SourceSpan span, ILocationConverter lc) - => IndexSpan.FromBounds(lc.LocationToIndex(span.Start), lc.LocationToIndex(span.End)); + public static class RangeExtensions { public static IndexSpan ToIndexSpan(this Range range, ILocationConverter lc) => IndexSpan.FromBounds(lc.LocationToIndex(range.start), lc.LocationToIndex(range.end)); + public static SourceSpan ToSourceSpan(this Range range, ILocationConverter lc = null) + => new SourceSpan(range.start.ToSourceLocation(lc), range.end.ToSourceLocation(lc)); + } - public static SourceSpan ToSourceSpan(this Range range, ILocationConverter lc = null) { - return new SourceSpan(range.start.ToSourceLocation(lc), range.end.ToSourceLocation(lc)); - } + public static class SourceSpanExtensions { + public static IndexSpan ToIndexSpan(this SourceSpan span, ILocationConverter lc) + => IndexSpan.FromBounds(lc.LocationToIndex(span.Start), lc.LocationToIndex(span.End)); } public static class IndexSpanExtensions { diff --git a/src/UnitTests/Core/Impl/MarkupUtils.cs b/src/UnitTests/Core/Impl/MarkupUtils.cs new file mode 100644 index 000000000..9a4786ada --- /dev/null +++ b/src/UnitTests/Core/Impl/MarkupUtils.cs @@ -0,0 +1,244 @@ +// 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, +// MERCHANTABILITY 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 System.Linq; +using System.Text; +using System.Text.RegularExpressions; +using Microsoft.Python.Core.Text; + +namespace Microsoft.Python.UnitTests.Core { + /// + /// To aid with testing, we define a special type of text file that can encode additional + /// information in it. This prevents a test writer from having to carry around multiple sources + /// of information that must be reconstituted. For example, instead of having to keep around the + /// contents of a file *and* and the location of the cursor, the tester can just provide a + /// string with the "$" character in it. This allows for easy creation of "FIT" tests where all + /// that needs to be provided are strings that encode every bit of state necessary in the string + /// itself. + /// + /// The current set of encoded features we support are: + /// + /// $$ - The position in the file. There can be at most one of these. + /// + /// [| ... |] - A span of text in the file. There can be many of these and they can be nested + /// and/or overlap the $ position. + /// + /// {|Name: ... |} A span of text in the file annotated with an identifier. There can be many of + /// these, including ones with the same name. + /// + /// Additional encoded features can be added on a case by case basis. + /// + public static class MarkupUtiles { + private const string PositionString = "$$"; + private const string SpanStartString = "[|"; + private const string SpanEndString = "|]"; + private const string NamedSpanStartString = "{|"; + private const string NamedSpanEndString = "|}"; + + private static readonly Regex s_namedSpanStartRegex = new Regex(@"\{\| ([-_.A-Za-z0-9\+]+) \:", + RegexOptions.Multiline | RegexOptions.IgnorePatternWhitespace); + + private static void Parse( + string input, out string output, out int? position, out IDictionary> spans) { + position = null; + var tempSpans = new Dictionary>(); + + var outputBuilder = new StringBuilder(); + + var currentIndexInInput = 0; + var inputOutputOffset = 0; + + // A stack of span starts along with their associated annotation name. [||] spans simply + // have empty string for their annotation name. + var spanStartStack = new Stack>(); + + while (true) { + var matches = new List<(int index, string value)>(); + AddMatch(input, PositionString, currentIndexInInput, matches); + AddMatch(input, SpanStartString, currentIndexInInput, matches); + AddMatch(input, SpanEndString, currentIndexInInput, matches); + AddMatch(input, NamedSpanEndString, currentIndexInInput, matches); + + var namedSpanStartMatch = s_namedSpanStartRegex.Match(input, currentIndexInInput); + if (namedSpanStartMatch.Success) { + matches.Add((namedSpanStartMatch.Index, namedSpanStartMatch.Value)); + } + + if (matches.Count == 0) { + // No more markup to process. + break; + } + + var orderedMatches = matches.OrderBy(t => t.index).ToList(); + if (orderedMatches.Count >= 2 && + spanStartStack.Count > 0 && + matches[0].Item1 == matches[1].Item1 - 1) { + // We have a slight ambiguity with cases like these: + // + // [|] [|} + // + // Is it starting a new match, or ending an existing match. As a workaround, we + // special case these and consider it ending a match if we have something on the + // stack already. + if ((matches[0].Item2 == SpanStartString && matches[1].Item2 == SpanEndString && spanStartStack.Peek().Item2 == string.Empty) || + (matches[0].Item2 == SpanStartString && matches[1].Item2 == NamedSpanEndString && spanStartStack.Peek().Item2 != string.Empty)) { + orderedMatches.RemoveAt(0); + } + } + + // Order the matches by their index + var firstMatch = orderedMatches.First(); + + var matchIndexInInput = firstMatch.Item1; + var matchString = firstMatch.Item2; + + var matchIndexInOutput = matchIndexInInput - inputOutputOffset; + outputBuilder.Append(input.Substring(currentIndexInInput, matchIndexInInput - currentIndexInInput)); + + currentIndexInInput = matchIndexInInput + matchString.Length; + inputOutputOffset += matchString.Length; + + switch (matchString.Substring(0, 2)) { + case PositionString: + if (position.HasValue) { + throw new ArgumentException(string.Format("Saw multiple occurrences of {0}", PositionString)); + } + + position = matchIndexInOutput; + break; + + case SpanStartString: + spanStartStack.Push(Tuple.Create(matchIndexInOutput, string.Empty)); + break; + + case SpanEndString: + if (spanStartStack.Count == 0) { + throw new ArgumentException(string.Format("Saw {0} without matching {1}", SpanEndString, SpanStartString)); + } + + if (spanStartStack.Peek().Item2.Length > 0) { + throw new ArgumentException(string.Format("Saw {0} without matching {1}", NamedSpanStartString, NamedSpanEndString)); + } + + PopSpan(spanStartStack, tempSpans, matchIndexInOutput); + break; + + case NamedSpanStartString: + var name = namedSpanStartMatch.Groups[1].Value; + spanStartStack.Push(Tuple.Create(matchIndexInOutput, name)); + break; + + case NamedSpanEndString: + if (spanStartStack.Count == 0) { + throw new ArgumentException(string.Format("Saw {0} without matching {1}", NamedSpanEndString, NamedSpanStartString)); + } + + if (spanStartStack.Peek().Item2.Length == 0) { + throw new ArgumentException(string.Format("Saw {0} without matching {1}", SpanStartString, SpanEndString)); + } + + PopSpan(spanStartStack, tempSpans, matchIndexInOutput); + break; + + default: + throw new InvalidOperationException(); + } + } + + if (spanStartStack.Count > 0) { + throw new ArgumentException(string.Format("Saw {0} without matching {1}", SpanStartString, SpanEndString)); + } + + // Append the remainder of the string. + outputBuilder.Append(input.Substring(currentIndexInInput)); + output = outputBuilder.ToString(); + spans = tempSpans.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + } + + private static V GetOrAdd(IDictionary dictionary, K key, Func function) { + if (!dictionary.TryGetValue(key, out var value)) { + value = function(key); + dictionary.Add(key, value); + } + + return value; + } + + private static void PopSpan( + Stack> spanStartStack, + IDictionary> spans, + int finalIndex) { + var spanStartTuple = spanStartStack.Pop(); + + var span = IndexSpan.FromBounds(spanStartTuple.Item1, finalIndex); + GetOrAdd(spans, spanStartTuple.Item2, _ => new List()).Add(span); + } + + private static void AddMatch(string input, string value, int currentIndex, List<(int index, string value)> matches) { + var index = input.IndexOf(value, currentIndex, StringComparison.Ordinal); + if (index >= 0) { + matches.Add((index, value)); + } + } + + private static void GetPositionAndSpans( + string input, out string output, out int? cursorPositionOpt, out List spans) { + Parse(input, out output, out cursorPositionOpt, out var dictionary); + + spans = GetOrAdd(dictionary, string.Empty, _ => new List()); + } + + public static void GetPositionAndNamedSpans( + string input, out string output, out int? cursorPositionOpt, out IDictionary> spans) { + Parse(input, out output, out cursorPositionOpt, out spans); + } + + public static void GetNamedSpans(string input, out string output, out IDictionary> spans) + => GetPositionAndNamedSpans(input, out output, out var _, out spans); + + public static void GetPositionAndSpans(string input, out string output, out int cursorPosition, out List spans) { + GetPositionAndSpans(input, out output, out int? pos, out spans); + cursorPosition = pos.Value; + } + + public static void GetPosition(string input, out string output, out int? cursorPosition) + => GetPositionAndSpans(input, out output, out cursorPosition, out List _); + + public static void GetPosition(string input, out string output, out int cursorPosition) + => GetPositionAndSpans(input, out output, out cursorPosition, out var _); + + public static void GetPositionAndSpan(string input, out string output, out int? cursorPosition, out IndexSpan? textSpan) { + GetPositionAndSpans(input, out output, out cursorPosition, out List spans); + textSpan = spans.Count == 0 ? null : (IndexSpan?)spans.Single(); + } + + public static void GetPositionAndSpan(string input, out string output, out int cursorPosition, out IndexSpan textSpan) { + GetPositionAndSpans(input, out output, out cursorPosition, out var spans); + textSpan = spans.Single(); + } + + public static void GetSpans(string input, out string output, out List spans) { + GetPositionAndSpans(input, out output, out int? pos, out spans); + } + + public static void GetSpan(string input, out string output, out IndexSpan textSpan) { + GetSpans(input, out output, out List spans); + textSpan = spans.Single(); + } + } +} diff --git a/src/UnitTests/Core/Impl/UnitTests.Core.csproj b/src/UnitTests/Core/Impl/UnitTests.Core.csproj index b4b821830..801ddd3ae 100644 --- a/src/UnitTests/Core/Impl/UnitTests.Core.csproj +++ b/src/UnitTests/Core/Impl/UnitTests.Core.csproj @@ -15,6 +15,9 @@ runtime; build; native; contentfiles; analyzers + + + \ No newline at end of file From 615f9055c75d3627c413bb771f6d3f294eb19b8d Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 18:02:07 -0700 Subject: [PATCH 07/43] adding more tests --- .../MissingImportCodeActionProvider.cs | 2 +- .../Test/MissingImportCodeActionTests.cs | 57 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 48b64340d..a54dca039 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -105,7 +105,7 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, var titleText = locallyInserted ? string.Format(Resources.ImportLocally, insertionText) : insertionText; var sb = new StringBuilder(); - sb.Append(insertionPoint.Value.indentation); + sb.AppendIf(insertionPoint.Value.range.start == insertionPoint.Value.range.end, insertionPoint.Value.indentation); sb.Append(insertionPoint.Value.addBlankLine ? insertionText + Environment.NewLine : insertionText); sb.AppendIf(insertionPoint.Value.range.start == insertionPoint.Value.range.end, Environment.NewLine); diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 5cadddbd6..8a15cd80d 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -84,6 +84,63 @@ public async Task TopModuleFunctionDefinitionLocally() { TestCodeAction(analysis.Document.Uri, codeAction, title: string.Format(Resources.ImportLocally, "import ntpath"), insertionSpan, newText); } + [TestMethod, Priority(0)] + public async Task SubModule() { + const string markup = @"{|insertionSpan:|}{|diagnostic:util|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + var title = "from ctypes import util"; + var codeAction = codeActions.Single(c => c.title == title); + var newText = "from ctypes import util" + Environment.NewLine + Environment.NewLine; + + TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + } + + [TestMethod, Priority(0)] + public async Task SubModuleUpdate() { + const string markup = @"{|insertionSpan:from ctypes import util|} +{|diagnostic:test|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + var title = "from ctypes import test, util"; + var codeAction = codeActions.Single(c => c.title == title); + var newText = "from ctypes import test, util"; + + TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + } + + [TestMethod, Priority(0)] + public async Task SubModuleFunctionDefinitionUpdateLocally() { + const string markup = @"def TestMethod(): + {|insertionSpan:from ctypes import util|} + {|diagnostic:test|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + var title = string.Format(Resources.ImportLocally, "from ctypes import test, util"); + var codeAction = codeActions.Single(c => c.title == title); + var newText = "from ctypes import test, util"; + + TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + } + + [TestMethod, Priority(0)] + public async Task SubModuleFunctionDefinition() { + const string markup = @"{|insertionSpan:|}def TestMethod(): + from ctypes import util + {|diagnostic:test|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + var title = "from ctypes import test"; + var codeAction = codeActions.Single(c => c.title == title); + var newText = "from ctypes import test" + Environment.NewLine + Environment.NewLine; + + TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + } + private async Task<(IDocumentAnalysis analysis, CodeAction[] diagnostics, SourceSpan insertionSpan)> GetAnalysisAndCodeActionsAndSpanAsync( string markup, IEnumerable codes) { MarkupUtiles.GetNamedSpans(markup, out var code, out var spans); From 7d0325cc03b1a41cca2675ed8be6d0ec25a10330 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 18:05:34 -0700 Subject: [PATCH 08/43] rename --- src/LanguageServer/Test/MissingImportCodeActionTests.cs | 2 +- src/UnitTests/Core/Impl/MarkupUtils.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 8a15cd80d..dd08c495c 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -143,7 +143,7 @@ from ctypes import util private async Task<(IDocumentAnalysis analysis, CodeAction[] diagnostics, SourceSpan insertionSpan)> GetAnalysisAndCodeActionsAndSpanAsync( string markup, IEnumerable codes) { - MarkupUtiles.GetNamedSpans(markup, out var code, out var spans); + MarkupUtils.GetNamedSpans(markup, out var code, out var spans); var analysis = await GetAnalysisAsync(code); var insertionSpan = spans["insertionSpan"].First().ToSourceSpan(analysis.Ast); diff --git a/src/UnitTests/Core/Impl/MarkupUtils.cs b/src/UnitTests/Core/Impl/MarkupUtils.cs index 9a4786ada..42feee415 100644 --- a/src/UnitTests/Core/Impl/MarkupUtils.cs +++ b/src/UnitTests/Core/Impl/MarkupUtils.cs @@ -43,7 +43,7 @@ namespace Microsoft.Python.UnitTests.Core { /// /// Additional encoded features can be added on a case by case basis. /// - public static class MarkupUtiles { + public static class MarkupUtils { private const string PositionString = "$$"; private const string SpanStartString = "[|"; private const string SpanEndString = "|]"; From f409d471b78c019872820664608012b6a8b31c7d Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 9 Oct 2019 23:17:04 -0700 Subject: [PATCH 09/43] simplify test a bit more --- .../Test/MissingImportCodeActionTests.cs | 85 ++++++++++--------- 1 file changed, 43 insertions(+), 42 deletions(-) diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index dd08c495c..7c353304d 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -14,6 +14,7 @@ // permissions and limitations under the License. using System; +using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -21,6 +22,8 @@ using Microsoft.Python.Analysis; using Microsoft.Python.Analysis.Analyzer; using Microsoft.Python.Core.Text; +using Microsoft.Python.LanguageServer.CodeActions; +using Microsoft.Python.LanguageServer.Diagnostics; using Microsoft.Python.LanguageServer.Protocol; using Microsoft.Python.LanguageServer.Sources; using Microsoft.Python.LanguageServer.Tests.FluentAssertions; @@ -29,9 +32,6 @@ using Microsoft.Python.UnitTests.Core; using Microsoft.VisualStudio.TestTools.UnitTesting; using TestUtilities; -using Microsoft.Python.LanguageServer.Diagnostics; -using Microsoft.Python.LanguageServer.CodeActions; -using System.Collections.Generic; namespace Microsoft.Python.LanguageServer.Tests { [TestClass] @@ -45,6 +45,18 @@ public void TestInitialize() [TestCleanup] public void Cleanup() => TestEnvironmentImpl.TestCleanup(); + [TestMethod, Priority(0)] + public async Task Missing() { + MarkupUtils.GetSpan(@"[|missingModule|]", out var code, out var span); + + var analysis = await GetAnalysisAsync(code); + var diagnostics = GetDiagnostics(analysis, span.ToSourceSpan(analysis.Ast), MissingImportCodeActionProvider.Instance.FixableDiagnostics); + diagnostics.Should().NotBeEmpty(); + + var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); + codeActions.Should().BeEmpty(); + } + [TestMethod, Priority(0)] public async Task TopModule() { const string markup = @"{|insertionSpan:|}{|diagnostic:ntpath|}"; @@ -57,7 +69,7 @@ public async Task TopModule() { } [TestMethod, Priority(0)] - public async Task TopModuleFunctionDefinition() { + public async Task TopModuleFromFunctionInsertTop() { const string markup = @"{|insertionSpan:|}def TestMethod(): {|diagnostic:ntpath|}"; @@ -71,7 +83,7 @@ public async Task TopModuleFunctionDefinition() { } [TestMethod, Priority(0)] - public async Task TopModuleFunctionDefinitionLocally() { + public async Task TopModuleLocally() { const string markup = @"def TestMethod(): {|insertionSpan:|} {|diagnostic:ntpath|}"; @@ -86,58 +98,47 @@ public async Task TopModuleFunctionDefinitionLocally() { [TestMethod, Priority(0)] public async Task SubModule() { - const string markup = @"{|insertionSpan:|}{|diagnostic:util|}"; - - var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); - - var title = "from ctypes import util"; - var codeAction = codeActions.Single(c => c.title == title); - var newText = "from ctypes import util" + Environment.NewLine + Environment.NewLine; - - TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + await TestCodeActionAsync( + @"{|insertionSpan:|}{|diagnostic:util|}", + title: "from ctypes import util", + newText: "from ctypes import util" + Environment.NewLine + Environment.NewLine); } [TestMethod, Priority(0)] public async Task SubModuleUpdate() { - const string markup = @"{|insertionSpan:from ctypes import util|} -{|diagnostic:test|}"; - - var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); - - var title = "from ctypes import test, util"; - var codeAction = codeActions.Single(c => c.title == title); - var newText = "from ctypes import test, util"; - - TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + await TestCodeActionAsync( + @"{|insertionSpan:from ctypes import util|} +{|diagnostic:test|}", + title: "from ctypes import test, util", + newText: "from ctypes import test, util"); } [TestMethod, Priority(0)] - public async Task SubModuleFunctionDefinitionUpdateLocally() { - const string markup = @"def TestMethod(): + public async Task SubModuleUpdateLocally() { + await TestCodeActionAsync( + @"def TestMethod(): {|insertionSpan:from ctypes import util|} - {|diagnostic:test|}"; - - var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); - - var title = string.Format(Resources.ImportLocally, "from ctypes import test, util"); - var codeAction = codeActions.Single(c => c.title == title); - var newText = "from ctypes import test, util"; - - TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + {|diagnostic:test|}", + title: string.Format(Resources.ImportLocally, "from ctypes import test, util"), + newText: "from ctypes import test, util"); } [TestMethod, Priority(0)] - public async Task SubModuleFunctionDefinition() { - const string markup = @"{|insertionSpan:|}def TestMethod(): + public async Task SubModuleFromFunctionInsertTop() { + + await TestCodeActionAsync( + @"{|insertionSpan:|}def TestMethod(): from ctypes import util - {|diagnostic:test|}"; + {|diagnostic:test|}", + title: "from ctypes import test", + newText: "from ctypes import test" + Environment.NewLine + Environment.NewLine); + } - var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + private async Task TestCodeActionAsync(string markup, string title, string newText) { + var (analysis, codeActions, insertionSpan) = + await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); - var title = "from ctypes import test"; var codeAction = codeActions.Single(c => c.title == title); - var newText = "from ctypes import test" + Environment.NewLine + Environment.NewLine; - TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); } From 791524166ed5fcab9e48e44ed1f1a0647222bd18 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 10 Oct 2019 00:46:39 -0700 Subject: [PATCH 10/43] added more tests --- .../MissingImportCodeActionProvider.cs | 8 +- .../Test/MissingImportCodeActionTests.cs | 119 +++++++++++++++++- 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index a54dca039..47f65293f 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -80,15 +80,15 @@ public Task> GetCodeActionAsync(IDocumentAnalysis analys } var codeActions = new List(); + var localCodeActions = new List(); foreach (var fullyQualifiedName in fullyQualifiedNames.OrderBy(n => n, ModuleNameComparer.Instance)) { cancellationToken.ThrowIfCancellationRequested(); - codeActions.AddIfNotNull( - CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken), - CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); + codeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken)); + localCodeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); } - return Task.FromResult>(codeActions); + return Task.FromResult(codeActions.Concat(localCodeActions)); } private CodeAction CreateCodeAction(IDocumentAnalysis analysis, diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 7c353304d..00064c515 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -125,7 +125,6 @@ await TestCodeActionAsync( [TestMethod, Priority(0)] public async Task SubModuleFromFunctionInsertTop() { - await TestCodeActionAsync( @"{|insertionSpan:|}def TestMethod(): from ctypes import util @@ -134,6 +133,124 @@ from ctypes import util newText: "from ctypes import test" + Environment.NewLine + Environment.NewLine); } + [TestMethod, Priority(0)] + public async Task AfterExistingImport() { + await TestCodeActionAsync( + @"from os import path +{|insertionSpan:|} +{|diagnostic:util|}", + title: "from ctypes import util", + newText: "from ctypes import util" + Environment.NewLine); + } + + [TestMethod, Priority(0)] + public async Task ReplaceExistingImport() { + await TestCodeActionAsync( + @"from os import path +{|insertionSpan:from ctypes import test|} +import socket + +{|diagnostic:util|}", + title: "from ctypes import test, util", + newText: "from ctypes import test, util"); + } + + [TestMethod, Priority(0)] + public async Task AfterExistingImportLocally() { + await TestCodeActionAsync( + @"def TestMethod(): + from os import path +{|insertionSpan:|} + {|diagnostic:util|}", + title: string.Format(Resources.ImportLocally, "from ctypes import util"), + newText: " from ctypes import util" + Environment.NewLine); + } + + [TestMethod, Priority(0)] + public async Task ReplaceExistingImportLocally() { + await TestCodeActionAsync( + @"def TestMethod(): + from os import path + {|insertionSpan:from ctypes import test|} + import socket + + {|diagnostic:util|}", + title: string.Format(Resources.ImportLocally, "from ctypes import test, util"), + newText: "from ctypes import test, util"); + } + + [TestMethod, Priority(0)] + public async Task CodeActionOrdering() { + MarkupUtils.GetSpan(@"def TestMethod(): + [|test|]", out var code, out var span); + + var analysis = await GetAnalysisAsync(code); + var diagnostics = GetDiagnostics(analysis, span.ToSourceSpan(analysis.Ast), MissingImportCodeActionProvider.Instance.FixableDiagnostics); + diagnostics.Should().NotBeEmpty(); + + var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); + + var list = codeActions.Select(c => c.title).ToList(); + var zipList = Enumerable.Range(0, list.Count).Zip(list); + + var locallyImportedPrefix = Resources.ImportLocally.Substring(0, Resources.ImportLocally.IndexOf("'")); + var maxIndexOfTopAddImports = zipList.Where(t => !t.Second.StartsWith(locallyImportedPrefix)).Max(t => t.First); + var minIndexOfLocalAddImports = zipList.Where(t => t.Second.StartsWith(locallyImportedPrefix)).Min(t => t.First); + + maxIndexOfTopAddImports.Should().BeLessThan(minIndexOfLocalAddImports); + } + + [TestMethod, Priority(0)] + public async Task PreserveComment() { + await TestCodeActionAsync( + @"{|insertionSpan:from os import pathconf|} # test + +{|diagnostic:path|}", + title: "from os import path, pathconf", + newText: "from os import path, pathconf"); + } + + [TestMethod, Priority(0)] + public async Task MemberSymbol() { + await TestCodeActionAsync( + @"from os import path +{|insertionSpan:|} +{|diagnostic:socket|}", + title: "from socket import socket", + newText: "from socket import socket" + Environment.NewLine); + } + + [TestMethod, Priority(0)] + public async Task NoMemberSymbol() { + var markup = @"{|insertionSpan:|}{|diagnostic:socket|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + codeActions.Select(c => c.title).Should().NotContain("from socket import socket"); + + var title = "import socket"; + var codeAction = codeActions.Single(c => c.title == title); + var newText = "import socket" + Environment.NewLine + Environment.NewLine; + TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + } + + [TestMethod, Priority(0)] + public async Task SymbolOrdering() { + var markup = @"from os import path +{|insertionSpan:|} +{|diagnostic:socket|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + var list = codeActions.Select(c => c.title).ToList(); + var zipList = Enumerable.Range(0, list.Count).Zip(list); + + var maxIndexOfPublicSymbol = zipList.Where(t => !t.Second.StartsWith("from _")).Max(t => t.First); + var minIndexOfPrivateSymbol = zipList.Where(t => t.Second.StartsWith("from _")).Min(t => t.First); + + maxIndexOfPublicSymbol.Should().BeLessThan(minIndexOfPrivateSymbol); + } + private async Task TestCodeActionAsync(string markup, string title, string newText) { var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); From 3881d24bdaaa92c379e8d26f5a2654a6ac3511d5 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 10 Oct 2019 10:55:42 -0700 Subject: [PATCH 11/43] changed codeaction source to use diagnostic service which should have diagnostic cached for latest analysis --- .../Impl/Sources/CodeActionSource.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs index 01d6e0c68..3e48442ce 100644 --- a/src/LanguageServer/Impl/Sources/CodeActionSource.cs +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -14,6 +14,7 @@ // permissions and limitations under the License. +using System; using System.Collections.Generic; using System.Linq; using System.Threading; @@ -44,13 +45,6 @@ public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, var results = new List(); - // * NOTE * this always re-calculate whole document for all linters. - // consider creating linter result cache somewhere (MRU?) and - // add error code as optional argument to run only that linter rather - // than always running all linters. - // also, current LintModule implementation always run all linters - // even if the linter option is off to set references in the given - // modules. this code path should use different code path to prevent that. foreach (var diagnostic in GetMatchingDiagnostics(analysis, diagnostics, cancellationToken)) { foreach (var codeActionProvider in _codeActionProviders) { if (codeActionProvider.FixableDiagnostics.Any(code => code == diagnostic.ErrorCode)) { @@ -63,8 +57,14 @@ public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, } private IEnumerable GetMatchingDiagnostics(IDocumentAnalysis analysis, Diagnostic[] diagnostics, CancellationToken cancellationToken) { - var analyzer = _services.GetService(); - foreach (var diagnostic in analysis.Diagnostics.Concat(analyzer.LintModule(analysis.Document))) { + var diagnosticService = _services.GetService(); + + // we assume diagnostic service has the latest results + if (diagnosticService == null || !diagnosticService.Diagnostics.TryGetValue(analysis.Document.Uri, out var latestDiagnostics)) { + yield break; + } + + foreach (var diagnostic in latestDiagnostics) { cancellationToken.ThrowIfCancellationRequested(); if (diagnostics.Any(d => AreEqual(d, diagnostic))) { From f94461a3a76bfd3fb004a2c3c38345ee8a169d52 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 10 Oct 2019 10:55:57 -0700 Subject: [PATCH 12/43] removed unused usings --- src/LanguageServer/Impl/Sources/CodeActionSource.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs index 3e48442ce..11f02f29a 100644 --- a/src/LanguageServer/Impl/Sources/CodeActionSource.cs +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -14,20 +14,17 @@ // permissions and limitations under the License. -using System; using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.Python.Analysis; -using Microsoft.Python.Analysis.Analyzer; using Microsoft.Python.Analysis.Diagnostics; using Microsoft.Python.Core; using Microsoft.Python.Core.Collections; using Microsoft.Python.LanguageServer.CodeActions; using Microsoft.Python.LanguageServer.Protocol; using Microsoft.Python.Parsing.Ast; -using Range = Microsoft.Python.Core.Text.Range; namespace Microsoft.Python.LanguageServer.Sources { internal sealed partial class CodeActionSource { From 3d1813122dab805337c33a0a2d9c43c127e6cb71 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 14 Oct 2019 10:54:42 -0700 Subject: [PATCH 13/43] add ability to index library to IndexManager --- .../PathResolverSnapshot.cs | 26 ++++-- .../MissingImportCodeActionProvider.cs | 40 +++++++-- .../Impl/Implementation/Server.cs | 2 +- .../Impl/Indexing/IIndexManager.cs | 4 +- .../Impl/Indexing/IndexManager.cs | 89 +++++++++++++++---- .../Indexing/MostRecentDocumentSymbols.cs | 6 +- .../Impl/Indexing/SymbolIndex.cs | 10 ++- .../Impl/Indexing/SymbolIndexWalker.cs | 47 ++++++++-- src/LanguageServer/Impl/Indexing/Symbols.cs | 3 + 9 files changed, 179 insertions(+), 48 deletions(-) diff --git a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs index 0439b2d4c..09fa12fbb 100644 --- a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs +++ b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs @@ -82,23 +82,27 @@ private PathResolverSnapshot(PythonLanguageVersion pythonLanguageVersion, string } public ImmutableArray GetAllImportableModuleNames(bool includeImplicitPackages = true) { - return GetAllImportableModuleFullNames(n => !string.IsNullOrEmpty(n.FullModuleName), includeImplicitPackages); + return GetAllImportableModuleInfo(n => !string.IsNullOrEmpty(n.FullModuleName), n => n.FullModuleName, includeImplicitPackages); } public ImmutableArray GetAllImportableModulesByName(string name, bool includeImplicitPackages = true) { - return GetAllImportableModuleFullNames(n => string.Equals(n.Name, name), includeImplicitPackages); + return GetAllImportableModuleInfo(n => string.Equals(n.Name, name), n => n.FullModuleName, includeImplicitPackages); } - private ImmutableArray GetAllImportableModuleFullNames(Func predicate, bool includeImplicitPackages = true) { + public ImmutableArray GetAllImportableModuleFilePaths(bool includeImplicitPackages = true) { + return GetAllImportableModuleInfo(n => !string.IsNullOrEmpty(n.ModulePath), n => n.ModulePath, includeImplicitPackages); + } + + private ImmutableArray GetAllImportableModuleInfo(Func predicate, Func valueGetter, bool includeImplicitPackages = true) { var roots = _roots.Prepend(_nonRooted); var items = new Queue(roots); - var names = ImmutableArray.Empty; + var stringValues = ImmutableArray.Empty; while (items.Count > 0) { var item = items.Dequeue(); if (item != null) { if (predicate(item) && (item.IsModule || includeImplicitPackages)) { - names = names.Add(item.FullModuleName); + stringValues = stringValues.Add(valueGetter(item)); } foreach (var child in item.Children.ExcludeDefault()) { @@ -107,13 +111,21 @@ private ImmutableArray GetAllImportableModuleFullNames(Func } } - return names.AddRange( + return stringValues.AddRange( _builtins.Children .Where(b => predicate(b)) - .Select(b => b.FullModuleName) + .Select(b => valueGetter(b)) ); } + public string GetModuleNameByPath(string modulePath) { + if (TryFindModule(modulePath, out var edge, out _)) { + return edge.End.FullModuleName; + } + + return null; + } + public ModuleImport GetModuleImportFromModuleName(in string fullModuleName) { for (var rootIndex = 0; rootIndex < _roots.Count; rootIndex++) { if (TryFindModuleByName(rootIndex, fullModuleName, out var lastEdge) && TryCreateModuleImport(lastEdge, out var moduleImports)) { diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 47f65293f..ce876ba72 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -21,6 +21,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.Python.Analysis; +using Microsoft.Python.Analysis.Analyzer; using Microsoft.Python.Analysis.Analyzer.Expressions; using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Diagnostics; @@ -29,6 +30,7 @@ using Microsoft.Python.Core; using Microsoft.Python.Core.Collections; using Microsoft.Python.Core.Text; +using Microsoft.Python.LanguageServer.Indexing; using Microsoft.Python.LanguageServer.Protocol; using Microsoft.Python.Parsing.Ast; using Range = Microsoft.Python.Core.Text.Range; @@ -43,14 +45,14 @@ private MissingImportCodeActionProvider() { public ImmutableArray FixableDiagnostics => ImmutableArray.Create( ErrorCodes.UndefinedVariable, ErrorCodes.VariableNotDefinedGlobally, ErrorCodes.VariableNotDefinedNonLocal); - public Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { + public async Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { // * TODO * for now, complete. since I don't know what each option mean. var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); // * TODO * need to check whether it is expected node kind or add code context check to verify it is a place where module/type can appear var node = finder.GetExpression(diagnostic.SourceSpan); if (!(node is NameExpression)) { - return Task.FromResult>(Array.Empty()); + return Enumerable.Empty(); } var interpreter = analysis.Document.Interpreter; @@ -58,20 +60,21 @@ public Task> GetCodeActionAsync(IDocumentAnalysis analys var name = node.ToCodeString(analysis.Ast); if (string.IsNullOrEmpty(name)) { - return Task.FromResult>(Array.Empty()); + return Enumerable.Empty(); } - var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); - var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); + // find path to module that might have symbols for us + await EnsureCandidateModulesAsync(analysis, name, cancellationToken); - var visited = new HashSet(); var fullyQualifiedNames = new HashSet(); // find modules matching the given name. this will include submodules - var fullModuleNames = pathResolver.GetAllImportableModulesByName(name, includeImplicit); - fullyQualifiedNames.UnionWith(fullModuleNames); + var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); + var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); + fullyQualifiedNames.UnionWith(pathResolver.GetAllImportableModulesByName(name, includeImplicit)); // find members matching the given name from module already imported. + var visited = new HashSet(); var nameParts = new List(); foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { nameParts.Add(module.Name); @@ -88,7 +91,26 @@ public Task> GetCodeActionAsync(IDocumentAnalysis analys localCodeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); } - return Task.FromResult(codeActions.Concat(localCodeActions)); + return codeActions.Concat(localCodeActions); + } + + private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis, + string name, + CancellationToken cancellationToken) { + var indexManager = analysis.ExpressionEvaluator.Services.GetService(); + var symbolsIncludingName = await indexManager.WorkspaceSymbolsAsync(name, maxLength: int.MaxValue, includeLibraries: true, cancellationToken); + var symbolsWithName = symbolsIncludingName.Where(s => s.Name == name && s.Kind != Indexing.SymbolKind.Variable); + + var analyzer = analysis.ExpressionEvaluator.Services.GetService(); + var pathResolver = analysis.Document.Interpreter.ModuleResolution.CurrentPathResolver; + foreach (var moduleName in symbolsWithName.Select(s => s.DocumentPath).Distinct().Select(p => pathResolver.GetModuleNameByPath(p))) { + var module = analysis.Document.Interpreter.ModuleResolution.GetOrLoadModule(moduleName); + + // TODO - module never get analyzed. it stays in "analyzing" state forever. the reason looks liek a version checking code in analyzer bail out + // living things in analyzing state (bug?) + // it is comparing analysis version against graph version which seems controlled independently so not sure how two version can be compared? + // need to figure out how to make the module analyzed + } } private CodeAction CreateCodeAction(IDocumentAnalysis analysis, diff --git a/src/LanguageServer/Impl/Implementation/Server.cs b/src/LanguageServer/Impl/Implementation/Server.cs index 3a2139983..74cf3320b 100644 --- a/src/LanguageServer/Impl/Implementation/Server.cs +++ b/src/LanguageServer/Impl/Implementation/Server.cs @@ -161,7 +161,7 @@ public async Task InitializedAsync(InitializedParams @params, CancellationToken initializationOptions?.includeFiles, initializationOptions?.excludeFiles, _services.GetService()); - _indexManager.IndexWorkspace().DoNotWait(); + _indexManager.IndexWorkspace(_interpreter.ModuleResolution.CurrentPathResolver).DoNotWait(); _services.AddService(_indexManager); _disposableBag.Add(_indexManager); diff --git a/src/LanguageServer/Impl/Indexing/IIndexManager.cs b/src/LanguageServer/Impl/Indexing/IIndexManager.cs index 9edaa18e0..1c4e7d871 100644 --- a/src/LanguageServer/Impl/Indexing/IIndexManager.cs +++ b/src/LanguageServer/Impl/Indexing/IIndexManager.cs @@ -17,16 +17,18 @@ using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; +using Microsoft.Python.Analysis.Core.DependencyResolution; using Microsoft.Python.Analysis.Documents; namespace Microsoft.Python.LanguageServer.Indexing { internal interface IIndexManager : IDisposable { - Task IndexWorkspace(CancellationToken ct = default); + Task IndexWorkspace(PathResolverSnapshot snapshot = null, CancellationToken ct = default); void ProcessNewFile(string path, IDocument doc); void ProcessClosedFile(string path); void ReIndexFile(string path, IDocument doc); void AddPendingDoc(IDocument doc); Task> HierarchicalDocumentSymbolsAsync(string path, CancellationToken cancellationToken = default); Task> WorkspaceSymbolsAsync(string query, int maxLength, CancellationToken cancellationToken = default); + Task> WorkspaceSymbolsAsync(string query, int maxLength, bool includeLibraries, CancellationToken cancellationToken = default); } } diff --git a/src/LanguageServer/Impl/Indexing/IndexManager.cs b/src/LanguageServer/Impl/Indexing/IndexManager.cs index d6ab967f8..8d3595d5b 100644 --- a/src/LanguageServer/Impl/Indexing/IndexManager.cs +++ b/src/LanguageServer/Impl/Indexing/IndexManager.cs @@ -16,9 +16,11 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Threading; using System.Threading.Tasks; +using Microsoft.Python.Analysis.Core.DependencyResolution; using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Documents; using Microsoft.Python.Core.Diagnostics; @@ -30,13 +32,15 @@ namespace Microsoft.Python.LanguageServer.Indexing { internal class IndexManager : IIndexManager { private const int DefaultReIndexDelay = 350; - private readonly ISymbolIndex _symbolIndex; + private readonly PythonLanguageVersion _version; + private readonly ISymbolIndex _userCodeSymbolIndex; + private readonly ISymbolIndex _libraryCodeSymbolIndex; private readonly IFileSystem _fileSystem; private readonly string _workspaceRootPath; private readonly string[] _includeFiles; private readonly string[] _excludeFiles; private readonly DisposableBag _disposables = new DisposableBag(nameof(IndexManager)); - private readonly ConcurrentDictionary _pendingDocs = new ConcurrentDictionary(new UriDocumentComparer()); + private readonly ConcurrentDictionary _pendingDocs = new ConcurrentDictionary(UriDocumentComparer.Instance); private readonly DisposeToken _disposeToken = DisposeToken.Create(); public IndexManager(IFileSystem fileSystem, PythonLanguageVersion version, string rootPath, string[] includeFiles, @@ -46,35 +50,47 @@ public IndexManager(IFileSystem fileSystem, PythonLanguageVersion version, strin Check.ArgumentNotNull(nameof(excludeFiles), excludeFiles); Check.ArgumentNotNull(nameof(idleTimeService), idleTimeService); + _version = version; _fileSystem = fileSystem; _workspaceRootPath = rootPath; _includeFiles = includeFiles; _excludeFiles = excludeFiles; - _symbolIndex = new SymbolIndex(_fileSystem, version); + _userCodeSymbolIndex = new SymbolIndex(_fileSystem, version); + _libraryCodeSymbolIndex = new SymbolIndex(_fileSystem, version, libraryMode: true); + idleTimeService.Idle += OnIdle; _disposables - .Add(_symbolIndex) + .Add(_userCodeSymbolIndex) + .Add(_libraryCodeSymbolIndex) .Add(() => _disposeToken.TryMarkDisposed()) .Add(() => idleTimeService.Idle -= OnIdle); } public int ReIndexingDelay { get; set; } = DefaultReIndexDelay; - public Task IndexWorkspace(CancellationToken ct = default) { + public Task IndexWorkspace(PathResolverSnapshot snapshot = null, CancellationToken ct = default) { var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(ct, _disposeToken.CancellationToken); var linkedCt = linkedCts.Token; return Task.Run(() => { - foreach (var fileInfo in WorkspaceFiles()) { - linkedCt.ThrowIfCancellationRequested(); - if (ModulePath.IsPythonSourceFile(fileInfo.FullName)) { - _symbolIndex.Parse(fileInfo.FullName); - } - } + var userFiles = WorkspaceFiles(); + CreateIndices(userFiles, _userCodeSymbolIndex, linkedCt); + + // index library files if asked + CreateIndices(LibraryFiles(snapshot).Except(userFiles, FileSystemInfoComparer.Instance), _libraryCodeSymbolIndex, linkedCt); }, linkedCt).ContinueWith(_ => linkedCts.Dispose()); } + private void CreateIndices(IEnumerable files, ISymbolIndex symbolIndex, CancellationToken cancellationToken) { + foreach (var fileInfo in files) { + cancellationToken.ThrowIfCancellationRequested(); + if (ModulePath.IsPythonSourceFile(fileInfo.FullName)) { + symbolIndex.Parse(fileInfo.FullName); + } + } + } + private IEnumerable WorkspaceFiles() { if (string.IsNullOrEmpty(_workspaceRootPath)) { return Enumerable.Empty(); @@ -82,11 +98,20 @@ private IEnumerable WorkspaceFiles() { return _fileSystem.GetDirectoryInfo(_workspaceRootPath).EnumerateFileSystemInfos(_includeFiles, _excludeFiles); } + private IEnumerable LibraryFiles(PathResolverSnapshot snapshot) { + if (snapshot == null) { + return Enumerable.Empty(); + } + + var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(_version.ToVersion()); + return snapshot.GetAllImportableModuleFilePaths(includeImplicit).Select(p => new FileInfoProxy(new FileInfo(p))); + } + public void ProcessClosedFile(string path) { if (IsFileOnWorkspace(path)) { - _symbolIndex.Parse(path); + _userCodeSymbolIndex.Parse(path); } else { - _symbolIndex.Delete(path); + _userCodeSymbolIndex.Delete(path); } } @@ -99,28 +124,43 @@ private bool IsFileOnWorkspace(string path) { } public void ProcessNewFile(string path, IDocument doc) { - _symbolIndex.Add(path, doc); + _userCodeSymbolIndex.Add(path, doc); } public void ReIndexFile(string path, IDocument doc) { - _symbolIndex.ReIndex(path, doc); + _userCodeSymbolIndex.ReIndex(path, doc); } public void Dispose() { _disposables.TryDispose(); } - public Task> HierarchicalDocumentSymbolsAsync(string path, CancellationToken cancellationToken = default) { - return _symbolIndex.HierarchicalDocumentSymbolsAsync(path, cancellationToken); + public async Task> HierarchicalDocumentSymbolsAsync(string path, CancellationToken cancellationToken = default) { + var result = await _userCodeSymbolIndex.HierarchicalDocumentSymbolsAsync(path, cancellationToken); + if (result.Count > 0) { + return result; + } + + return await _libraryCodeSymbolIndex.HierarchicalDocumentSymbolsAsync(path, cancellationToken); } public Task> WorkspaceSymbolsAsync(string query, int maxLength, CancellationToken cancellationToken = default) { - return _symbolIndex.WorkspaceSymbolsAsync(query, maxLength, cancellationToken); + return WorkspaceSymbolsAsync(query, maxLength, includeLibraries: false, cancellationToken); + } + + public async Task> WorkspaceSymbolsAsync(string query, int maxLength, bool includeLibraries, CancellationToken cancellationToken = default) { + var userCodeResult = await _userCodeSymbolIndex.WorkspaceSymbolsAsync(query, maxLength, cancellationToken); + if (includeLibraries == false) { + return userCodeResult; + } + + var libraryCodeResult = await _libraryCodeSymbolIndex.WorkspaceSymbolsAsync(query, maxLength, cancellationToken); + return userCodeResult.Concat(libraryCodeResult).ToList(); } public void AddPendingDoc(IDocument doc) { _pendingDocs.TryAdd(doc, DateTime.Now); - _symbolIndex.MarkAsPending(doc.Uri.AbsolutePath); + _userCodeSymbolIndex.MarkAsPending(doc.Uri.AbsolutePath); } private void OnIdle(object sender, EventArgs _) { @@ -137,10 +177,21 @@ private void ReIndexPendingDocsAsync() { } private class UriDocumentComparer : IEqualityComparer { + public static readonly UriDocumentComparer Instance = new UriDocumentComparer(); + + private UriDocumentComparer() { } public bool Equals(IDocument x, IDocument y) => x.Uri.Equals(y.Uri); public int GetHashCode(IDocument obj) => obj.Uri.GetHashCode(); } + + private class FileSystemInfoComparer : IEqualityComparer { + public static readonly FileSystemInfoComparer Instance = new FileSystemInfoComparer(); + + private FileSystemInfoComparer() { } + public bool Equals(IFileSystemInfo x, IFileSystemInfo y) => x?.FullName == y?.FullName; + public int GetHashCode(IFileSystemInfo obj) => obj.FullName.GetHashCode(); + } } } diff --git a/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs b/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs index fe8b6969b..1460ee856 100644 --- a/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs +++ b/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs @@ -14,15 +14,17 @@ class MostRecentDocumentSymbols : IMostRecentDocumentSymbols { private readonly object _syncObj = new object(); private readonly IIndexParser _indexParser; private readonly string _path; + private readonly bool _library; private CancellationTokenSource _fileCts = new CancellationTokenSource(); private TaskCompletionSource> _fileTcs = new TaskCompletionSource>(); private WorkQueueState state = WorkQueueState.WaitingForWork; - public MostRecentDocumentSymbols(string path, IIndexParser indexParser) { + public MostRecentDocumentSymbols(string path, IIndexParser indexParser, bool library) { _path = path; _indexParser = indexParser; + _library = library; } public void Parse() { @@ -136,7 +138,7 @@ private async Task> IndexAsync(IDocument doc, } indexCt.ThrowIfCancellationRequested(); - var walker = new SymbolIndexWalker(ast); + var walker = new SymbolIndexWalker(ast, _library); ast.Walk(walker); return walker.Symbols; } diff --git a/src/LanguageServer/Impl/Indexing/SymbolIndex.cs b/src/LanguageServer/Impl/Indexing/SymbolIndex.cs index 07628af36..831526735 100644 --- a/src/LanguageServer/Impl/Indexing/SymbolIndex.cs +++ b/src/LanguageServer/Impl/Indexing/SymbolIndex.cs @@ -29,11 +29,13 @@ internal sealed class SymbolIndex : ISymbolIndex { private readonly DisposableBag _disposables = new DisposableBag(nameof(SymbolIndex)); private readonly ConcurrentDictionary _index; private readonly IIndexParser _indexParser; + private readonly bool _libraryMode; - public SymbolIndex(IFileSystem fileSystem, PythonLanguageVersion version) { - var comparer = PathEqualityComparer.Instance; - _index = new ConcurrentDictionary(comparer); + public SymbolIndex(IFileSystem fileSystem, PythonLanguageVersion version, bool libraryMode = false) { + _index = new ConcurrentDictionary(PathEqualityComparer.Instance); _indexParser = new IndexParser(fileSystem, version); + _libraryMode = libraryMode; + _disposables .Add(_indexParser) .Add(() => { @@ -106,7 +108,7 @@ private IEnumerable WorkspaceSymbolsQuery(string path, string query, } private IMostRecentDocumentSymbols MakeMostRecentDocSymbols(string path) { - return new MostRecentDocumentSymbols(path, _indexParser); + return new MostRecentDocumentSymbols(path, _indexParser, _libraryMode); } public void Dispose() { diff --git a/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs b/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs index e4f68a8c0..0ce628a19 100644 --- a/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs +++ b/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs @@ -25,10 +25,12 @@ internal class SymbolIndexWalker : PythonWalker { private static readonly Regex ConstantLike = new Regex(@"^[\p{Lu}\p{N}_]+$", RegexOptions.Compiled); private readonly PythonAst _ast; + private readonly bool _libraryMode; private readonly SymbolStack _stack = new SymbolStack(); - public SymbolIndexWalker(PythonAst ast) { + public SymbolIndexWalker(PythonAst ast, bool libraryMode = false) { _ast = ast; + _libraryMode = libraryMode; } public IReadOnlyList Symbols => _stack.Root; @@ -50,12 +52,24 @@ public override bool Walk(ClassDefinition node) { return false; } + private void WalkIfNotLibraryMode(Node node) { + if (_libraryMode) { + return; + } + + node?.Walk(this); + } + public override bool Walk(FunctionDefinition node) { _stack.Enter(SymbolKind.Function); foreach (var p in node.Parameters) { AddVarSymbol(p.NameExpression); } - node.Body?.Walk(this); + + // don't bother to walk down locals for libraries + // we don't care those for libraries + WalkIfNotLibraryMode(node.Body); + var children = _stack.Exit(); var span = node.GetSpan(_ast); @@ -123,7 +137,8 @@ public override bool Walk(FromImportStatement node) { } public override bool Walk(AssignmentStatement node) { - node.Right?.Walk(this); + WalkIfNotLibraryMode(node.Right); + foreach (var exp in node.Left) { AddVarSymbolRecursive(exp); } @@ -132,18 +147,24 @@ public override bool Walk(AssignmentStatement node) { } public override bool Walk(NamedExpression node) { - node.Value?.Walk(this); + WalkIfNotLibraryMode(node.Value); + AddVarSymbolRecursive(node.Target); return false; } public override bool Walk(AugmentedAssignStatement node) { - node.Right?.Walk(this); + WalkIfNotLibraryMode(node.Right); + AddVarSymbol(node.Left as NameExpression); return false; } public override bool Walk(IfStatement node) { + if (_libraryMode) { + return false; + } + WalkAndDeclareAll(node.Tests); WalkAndDeclare(node.ElseStatement); @@ -151,6 +172,10 @@ public override bool Walk(IfStatement node) { } public override bool Walk(TryStatement node) { + if (_libraryMode) { + return false; + } + WalkAndDeclare(node.Body); WalkAndDeclareAll(node.Handlers); WalkAndDeclare(node.Else); @@ -160,6 +185,10 @@ public override bool Walk(TryStatement node) { } public override bool Walk(ForStatement node) { + if (_libraryMode) { + return false; + } + _stack.EnterDeclared(); AddVarSymbolRecursive(node.Left); node.List?.Walk(this); @@ -174,11 +203,19 @@ public override bool Walk(ForStatement node) { } public override bool Walk(ComprehensionFor node) { + if (_libraryMode) { + return false; + } + AddVarSymbolRecursive(node.Left); return base.Walk(node); } public override bool Walk(ListComprehension node) { + if (_libraryMode) { + return false; + } + _stack.Enter(SymbolKind.None); return base.Walk(node); } diff --git a/src/LanguageServer/Impl/Indexing/Symbols.cs b/src/LanguageServer/Impl/Indexing/Symbols.cs index 0cdac218d..6fbeb08e4 100644 --- a/src/LanguageServer/Impl/Indexing/Symbols.cs +++ b/src/LanguageServer/Impl/Indexing/Symbols.cs @@ -14,6 +14,7 @@ // permissions and limitations under the License. using System.Collections.Generic; +using System.Diagnostics; using Microsoft.Python.Core.Text; namespace Microsoft.Python.LanguageServer.Indexing { @@ -58,6 +59,7 @@ internal class FunctionKind { } // Analagous to LSP's DocumentSymbol. + [DebuggerDisplay("{Name}, {Kind}")] internal class HierarchicalSymbol { public string Name; public string Detail; @@ -87,6 +89,7 @@ public HierarchicalSymbol( } // Analagous to LSP's SymbolInformation. + [DebuggerDisplay("{ContainerName}:{Name}, {Kind}")] internal class FlatSymbol { public string Name; public SymbolKind Kind; From 8af626e2ad9cabe315ef670a6807720473b7f793 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 14 Oct 2019 11:40:48 -0700 Subject: [PATCH 14/43] trying GetAnalysisAsync --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index ce876ba72..22e8ebea9 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -105,11 +105,13 @@ private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis var pathResolver = analysis.Document.Interpreter.ModuleResolution.CurrentPathResolver; foreach (var moduleName in symbolsWithName.Select(s => s.DocumentPath).Distinct().Select(p => pathResolver.GetModuleNameByPath(p))) { var module = analysis.Document.Interpreter.ModuleResolution.GetOrLoadModule(moduleName); - + // TODO - module never get analyzed. it stays in "analyzing" state forever. the reason looks liek a version checking code in analyzer bail out // living things in analyzing state (bug?) // it is comparing analysis version against graph version which seems controlled independently so not sure how two version can be compared? // need to figure out how to make the module analyzed + + await analyzer.GetAnalysisAsync(module, cancellationToken: cancellationToken); } } From 5e0c5bff6aa60b56262b104e6fe2fc3a75ce62bc Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 14 Oct 2019 14:39:07 -0700 Subject: [PATCH 15/43] add closed module support for add imports. --- .../Impl/Dependencies/DependencyResolver.cs | 15 +++++++++-- .../MissingImportCodeActionProvider.cs | 25 ++++++++++++++----- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/Analysis/Ast/Impl/Dependencies/DependencyResolver.cs b/src/Analysis/Ast/Impl/Dependencies/DependencyResolver.cs index 5ef5f018a..747adf52b 100644 --- a/src/Analysis/Ast/Impl/Dependencies/DependencyResolver.cs +++ b/src/Analysis/Ast/Impl/Dependencies/DependencyResolver.cs @@ -23,6 +23,9 @@ namespace Microsoft.Python.Analysis.Dependencies { internal sealed class DependencyResolver : IDependencyResolver { + // optimization to only analyze one that is reachable from root + private readonly bool _checkVertexReachability = true; + private readonly Dictionary _keys = new Dictionary(); private readonly List> _vertices = new List>(); private readonly object _syncObj = new object(); @@ -287,7 +290,7 @@ private bool TryCreateWalkingGraph(in ImmutableArray>.Create(nodesByVertexIndex.Values); return true; + + bool ReachableFromRoot(ImmutableArray reachable, int index) { + const int inaccessibleFromRoot = -1; + + // one of usage case for this optimization is not analyzing module that is not reachable + // from user code + return _checkVertexReachability && reachable[index] != inaccessibleFromRoot; + } } private static ImmutableArray CalculateDepths(in ImmutableArray> vertices) { diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 22e8ebea9..dd01be017 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -37,6 +37,8 @@ namespace Microsoft.Python.LanguageServer.CodeActions { internal sealed class MissingImportCodeActionProvider : ICodeActionProvider { + private const int ModuleLoadTimeout = 10 * 1000; // 10 seconds + public static readonly ICodeActionProvider Instance = new MissingImportCodeActionProvider(); private MissingImportCodeActionProvider() { @@ -103,15 +105,26 @@ private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis var analyzer = analysis.ExpressionEvaluator.Services.GetService(); var pathResolver = analysis.Document.Interpreter.ModuleResolution.CurrentPathResolver; + + var modules = ImmutableArray.Empty; foreach (var moduleName in symbolsWithName.Select(s => s.DocumentPath).Distinct().Select(p => pathResolver.GetModuleNameByPath(p))) { var module = analysis.Document.Interpreter.ModuleResolution.GetOrLoadModule(moduleName); + if (module.Analysis is EmptyAnalysis) { + // once module is analyzed, this analysis result will be kept in memory + // even if graph for current document (modified) changed. + // so this will let us to analyze these modules only once + modules = modules.Add(module); + } + } - // TODO - module never get analyzed. it stays in "analyzing" state forever. the reason looks liek a version checking code in analyzer bail out - // living things in analyzing state (bug?) - // it is comparing analysis version against graph version which seems controlled independently so not sure how two version can be compared? - // need to figure out how to make the module analyzed + if (modules.Count > 0) { + // declares given modules to be dependent to current document. otherwise, our optimization will drop + // analyzing these modules. this dependency will go away when user change the document + analyzer.EnqueueDocumentForAnalysis(analysis.Document, modules); - await analyzer.GetAnalysisAsync(module, cancellationToken: cancellationToken); + // this will make those modules to be loaded. we don't care about return analysis since we only + // care those modules being loaded and analyzed. but not new current document's analysis + await analysis.Document.GetAnalysisAsync(ModuleLoadTimeout, cancellationToken); } } @@ -316,7 +329,7 @@ private void AddNonImportedMemberWithName(IPythonModule module, string name, Lis return; } - // skip any imported member unless it is explicitly on __all__ + // skip any imported member (non module member) unless it is explicitly on __all__ if (module.Analysis.GlobalScope.Imported.TryGetVariable(name, out var imported) && object.Equals(pythonType, imported.Value) && GetAllVariables(module.Analysis).All(s => !string.Equals(s, name))) { From 8e73bbbfe1b2cf493b1fcc730614f73c3b9a114e Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 14 Oct 2019 15:24:04 -0700 Subject: [PATCH 16/43] removed assert that keep crashing debug bit LS the reason it was assert was due to code like below that defines 2 functions with same signature and we only keep 1 symbol out of it. ... def f(x): class C: x = 12 def m(self): return x locals() return C self.assertEqual(f(1).x, 12) def f(x): class C: y = x def m(self): return x z = list(locals()) return C ... --- .../Impl/Analyzer/Symbols/SymbolCollector.cs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs b/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs index a58f9f9b8..5fbcddeb5 100644 --- a/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs +++ b/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs @@ -57,6 +57,12 @@ public override bool Walk(ClassDefinition cd) { if (!string.IsNullOrEmpty(cd.NameExpression?.Name)) { var classInfo = CreateClass(cd); + if (classInfo == null) { + // we can't create class info for this node. + // don't walk down + return false; + } + // The variable is transient (non-user declared) hence it does not have location. // Class type is tracking locations for references and renaming. _eval.DeclareVariable(cd.Name, classInfo, VariableSource.Declaration); @@ -95,9 +101,14 @@ public override void PostWalk(FunctionDefinition fd) { private PythonClassType CreateClass(ClassDefinition cd) { PythonType declaringType = null; - if(!(cd.Parent is PythonAst)) { - Debug.Assert(_typeMap.ContainsKey(cd.Parent)); - _typeMap.TryGetValue(cd.Parent, out declaringType); + if (!(cd.Parent is PythonAst)) { + if (!_typeMap.TryGetValue(cd.Parent, out declaringType)) { + // we can get into this situation if parent is defined twice and we preserve + // only one of them. + // for example, code has function definition with exact same signature + // and class is defined under one of that function + return null; + } } var cls = new PythonClassType(cd, declaringType, _eval.GetLocationOfName(cd), _eval.SuppressBuiltinLookup ? BuiltinTypeId.Unknown : BuiltinTypeId.Type); @@ -120,6 +131,10 @@ private void AddFunction(FunctionDefinition fd, PythonType declaringType) { f = new PythonFunctionType(fd, declaringType, _eval.GetLocationOfName(fd)); // The variable is transient (non-user declared) hence it does not have location. // Function type is tracking locations for references and renaming. + + // if there are multiple functions with same name exist, only the very first one will be + // maintained in the scope. we should improve this if possible. + // https://github.com/microsoft/python-language-server/issues/1693 _eval.DeclareVariable(fd.Name, f, VariableSource.Declaration); _typeMap[fd] = f; declaringType?.AddMember(f.Name, f, overwrite: true); From 282ffc09f403bf312c437f7947bb36b8a7dae14a Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 14 Oct 2019 16:02:49 -0700 Subject: [PATCH 17/43] add closed module test. added just one since it is very expensive. --- .../MissingImportCodeActionProvider.cs | 5 +++ .../Test/MissingImportCodeActionTests.cs | 36 +++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index dd01be017..99605c35d 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -100,6 +100,11 @@ private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis string name, CancellationToken cancellationToken) { var indexManager = analysis.ExpressionEvaluator.Services.GetService(); + if (indexManager == null) { + // indexing is not supported + return; + } + var symbolsIncludingName = await indexManager.WorkspaceSymbolsAsync(name, maxLength: int.MaxValue, includeLibraries: true, cancellationToken); var symbolsWithName = symbolsIncludingName.Where(s => s.Name == name && s.Kind != Indexing.SymbolKind.Variable); diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 00064c515..bc8a3212b 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -21,9 +21,13 @@ using FluentAssertions; using Microsoft.Python.Analysis; using Microsoft.Python.Analysis.Analyzer; +using Microsoft.Python.Core.Idle; +using Microsoft.Python.Core.IO; +using Microsoft.Python.Core.Services; using Microsoft.Python.Core.Text; using Microsoft.Python.LanguageServer.CodeActions; using Microsoft.Python.LanguageServer.Diagnostics; +using Microsoft.Python.LanguageServer.Indexing; using Microsoft.Python.LanguageServer.Protocol; using Microsoft.Python.LanguageServer.Sources; using Microsoft.Python.LanguageServer.Tests.FluentAssertions; @@ -251,19 +255,45 @@ public async Task SymbolOrdering() { maxIndexOfPublicSymbol.Should().BeLessThan(minIndexOfPrivateSymbol); } - private async Task TestCodeActionAsync(string markup, string title, string newText) { + [TestMethod, Priority(0)] + public async Task ModuleNotReachableFromUserDocument() { + await TestCodeActionAsync( + @"{|insertionSpan:|}{|diagnostic:path|}", + title: "from os import path", + newText: "from os import path" + Environment.NewLine + Environment.NewLine, + enableIndexManager: true); + } + + private async Task TestCodeActionAsync(string markup, string title, string newText, bool enableIndexManager = false) { var (analysis, codeActions, insertionSpan) = - await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics, enableIndexManager); var codeAction = codeActions.Single(c => c.title == title); TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); } private async Task<(IDocumentAnalysis analysis, CodeAction[] diagnostics, SourceSpan insertionSpan)> GetAnalysisAndCodeActionsAndSpanAsync( - string markup, IEnumerable codes) { + string markup, IEnumerable codes, bool enableIndexManager = false) { MarkupUtils.GetNamedSpans(markup, out var code, out var spans); var analysis = await GetAnalysisAsync(code); + + if (enableIndexManager) { + var serviceManager = (IServiceManager)analysis.ExpressionEvaluator.Services; + var indexManager = new IndexManager( + serviceManager.GetService(), + analysis.Document.Interpreter.LanguageVersion, + rootPath: null, + Array.Empty(), + Array.Empty(), + serviceManager.GetService()); + + // make sure index is done + await indexManager.IndexWorkspace(analysis.Document.Interpreter.ModuleResolution.CurrentPathResolver); + + serviceManager.AddService(indexManager); + } + var insertionSpan = spans["insertionSpan"].First().ToSourceSpan(analysis.Ast); var diagnostics = GetDiagnostics(analysis, spans["diagnostic"].First().ToSourceSpan(analysis.Ast), codes); From 18735af94020f33dab8ff36a945043a1e4fb671a Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 14 Oct 2019 16:41:19 -0700 Subject: [PATCH 18/43] use explicit type rather than tuple since it has many members --- .../MissingImportCodeActionProvider.cs | 66 +++++++++++-------- 1 file changed, 40 insertions(+), 26 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 99605c35d..5073ba879 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -138,24 +138,24 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, string fullyQualifiedName, bool locallyInserted, CancellationToken cancellationToken) { - var insertionPoint = GetInsertionPoint(analysis, node, fullyQualifiedName, locallyInserted, cancellationToken); + var insertionPoint = GetInsertionInfo(analysis, node, fullyQualifiedName, locallyInserted, cancellationToken); if (insertionPoint == null) { return null; } - var insertionText = insertionPoint.Value.insertionText; + var insertionText = insertionPoint.Value.InsertionText; var titleText = locallyInserted ? string.Format(Resources.ImportLocally, insertionText) : insertionText; var sb = new StringBuilder(); - sb.AppendIf(insertionPoint.Value.range.start == insertionPoint.Value.range.end, insertionPoint.Value.indentation); - sb.Append(insertionPoint.Value.addBlankLine ? insertionText + Environment.NewLine : insertionText); - sb.AppendIf(insertionPoint.Value.range.start == insertionPoint.Value.range.end, Environment.NewLine); + sb.AppendIf(insertionPoint.Value.Range.start == insertionPoint.Value.Range.end, insertionPoint.Value.Indentation); + sb.Append(insertionPoint.Value.AddBlankLine ? insertionText + Environment.NewLine : insertionText); + sb.AppendIf(insertionPoint.Value.Range.start == insertionPoint.Value.Range.end, Environment.NewLine); var changes = new Dictionary {{ analysis.Document.Uri, new TextEdit[] { new TextEdit() { - range = insertionPoint.Value.range, + range = insertionPoint.Value.Range, newText = sb.ToString() }} }}; @@ -168,11 +168,11 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, }; } - private (bool addBlankLine, string insertionText, Range range, string indentation)? GetInsertionPoint(IDocumentAnalysis analysis, - Node node, - string fullyQualifiedModuleName, - bool locallyInserted, - CancellationToken cancellationToken) { + private InsertionInfo? GetInsertionInfo(IDocumentAnalysis analysis, + Node node, + string fullyQualifiedName, + bool locallyInserted, + CancellationToken cancellationToken) { var (body, indentation) = GetStartingPoint(analysis, node, locallyInserted, cancellationToken); if (body == null) { // no insertion point @@ -183,32 +183,32 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, var lastImportNode = importNodes.LastOrDefault(); // first check whether module name is dotted or not - var dotIndex = fullyQualifiedModuleName.LastIndexOf('.'); + var dotIndex = fullyQualifiedName.LastIndexOf('.'); if (dotIndex < 0) { // there can't be existing import since we have the error - return (addBlankLine: lastImportNode == null, - $"import {fullyQualifiedModuleName}", - GetRange(analysis.Ast, body, lastImportNode), - indentation); + return new InsertionInfo(addBlankLine: lastImportNode == null, + $"import {fullyQualifiedName}", + GetRange(analysis.Ast, body, lastImportNode), + indentation); } // see whether there is existing from * import * statement. - var fromPart = fullyQualifiedModuleName.Substring(startIndex: 0, dotIndex); - var moduleNameToAdd = fullyQualifiedModuleName.Substring(dotIndex + 1); + var fromPart = fullyQualifiedName.Substring(startIndex: 0, dotIndex); + var nameToAdd = fullyQualifiedName.Substring(dotIndex + 1); foreach (var current in importNodes.Reverse().OfType()) { if (current.Root.MakeString() == fromPart) { - return (addBlankLine: false, - GetInsertionText(current, fromPart, moduleNameToAdd), - current.GetSpan(analysis.Ast), - indentation); + return new InsertionInfo(addBlankLine: false, + GetInsertionText(current, fromPart, nameToAdd), + current.GetSpan(analysis.Ast), + indentation); } } // add new from * import * statement - return (addBlankLine: lastImportNode == null, - $"from {fromPart} import {moduleNameToAdd}", - GetRange(analysis.Ast, body, lastImportNode), - indentation); + return new InsertionInfo(addBlankLine: lastImportNode == null, + $"from {fromPart} import {nameToAdd}", + GetRange(analysis.Ast, body, lastImportNode), + indentation); } private string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { @@ -384,6 +384,20 @@ public int Compare(string x, string y) { return x.CompareTo(y); } } + + private struct InsertionInfo { + public bool AddBlankLine; + public string InsertionText; + public Range Range; + public string Indentation; + + public InsertionInfo(bool addBlankLine, string insertionText, Range range, string indentation) { + AddBlankLine = addBlankLine; + InsertionText = insertionText; + Range = range; + Indentation = indentation; + } + } } } From 7b681c173b1a61f4f028af4a3c2d3ac30f097cf4 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 14 Oct 2019 16:41:39 -0700 Subject: [PATCH 19/43] add readonly --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 5073ba879..cd6ebd72f 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -386,10 +386,10 @@ public int Compare(string x, string y) { } private struct InsertionInfo { - public bool AddBlankLine; - public string InsertionText; - public Range Range; - public string Indentation; + public readonly bool AddBlankLine; + public readonly string InsertionText; + public readonly Range Range; + public readonly string Indentation; public InsertionInfo(bool addBlankLine, string insertionText, Range range, string indentation) { AddBlankLine = addBlankLine; From 6823f7e0a52bc03d3b3e14897e9f8cf875b77e4c Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Tue, 15 Oct 2019 14:24:04 -0700 Subject: [PATCH 20/43] use existing formatUI to format localizable string --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index cd6ebd72f..133baef32 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -144,7 +144,7 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, } var insertionText = insertionPoint.Value.InsertionText; - var titleText = locallyInserted ? string.Format(Resources.ImportLocally, insertionText) : insertionText; + var titleText = locallyInserted ? Resources.ImportLocally.FormatUI(insertionText) : insertionText; var sb = new StringBuilder(); sb.AppendIf(insertionPoint.Value.Range.start == insertionPoint.Value.Range.end, insertionPoint.Value.Indentation); From 50055f5b395818d6a89015b625e010bee6bd3cee Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 16 Oct 2019 12:00:57 -0700 Subject: [PATCH 21/43] disabled locally imported code action for now --- .../CodeActions/MissingImportCodeActionProvider.cs | 5 +---- .../Test/MissingImportCodeActionTests.cs | 12 ++++++------ 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 133baef32..11cb77ed2 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -85,15 +85,12 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis } var codeActions = new List(); - var localCodeActions = new List(); foreach (var fullyQualifiedName in fullyQualifiedNames.OrderBy(n => n, ModuleNameComparer.Instance)) { cancellationToken.ThrowIfCancellationRequested(); - codeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken)); - localCodeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: true, cancellationToken)); } - return codeActions.Concat(localCodeActions); + return codeActions; } private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis, diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index bc8a3212b..382f170dd 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -72,7 +72,7 @@ public async Task TopModule() { TestCodeAction(analysis.Document.Uri, codeAction, title: "import ntpath", insertionSpan, newText); } - [TestMethod, Priority(0)] + [TestMethod, Priority(0), Ignore] public async Task TopModuleFromFunctionInsertTop() { const string markup = @"{|insertionSpan:|}def TestMethod(): {|diagnostic:ntpath|}"; @@ -86,7 +86,7 @@ public async Task TopModuleFromFunctionInsertTop() { TestCodeAction(analysis.Document.Uri, codeAction, title: "import ntpath", insertionSpan, newText); } - [TestMethod, Priority(0)] + [TestMethod, Priority(0), Ignore] public async Task TopModuleLocally() { const string markup = @"def TestMethod(): {|insertionSpan:|} {|diagnostic:ntpath|}"; @@ -117,7 +117,7 @@ await TestCodeActionAsync( newText: "from ctypes import test, util"); } - [TestMethod, Priority(0)] + [TestMethod, Priority(0), Ignore] public async Task SubModuleUpdateLocally() { await TestCodeActionAsync( @"def TestMethod(): @@ -159,7 +159,7 @@ import socket newText: "from ctypes import test, util"); } - [TestMethod, Priority(0)] + [TestMethod, Priority(0), Ignore] public async Task AfterExistingImportLocally() { await TestCodeActionAsync( @"def TestMethod(): @@ -170,7 +170,7 @@ from os import path newText: " from ctypes import util" + Environment.NewLine); } - [TestMethod, Priority(0)] + [TestMethod, Priority(0), Ignore] public async Task ReplaceExistingImportLocally() { await TestCodeActionAsync( @"def TestMethod(): @@ -183,7 +183,7 @@ import socket newText: "from ctypes import test, util"); } - [TestMethod, Priority(0)] + [TestMethod, Priority(0), Ignore] public async Task CodeActionOrdering() { MarkupUtils.GetSpan(@"def TestMethod(): [|test|]", out var code, out var span); From 87e8eef725846b7ad984e22c81eafdab2fbfd373 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 17 Oct 2019 15:10:04 -0700 Subject: [PATCH 22/43] fixed a bug where it didn't handle circular import properly, and added more comment --- .../MissingImportCodeActionProvider.cs | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 11cb77ed2..343bded3f 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -16,6 +16,8 @@ using System; using System.Collections.Generic; +using System.Data; +using System.Diagnostics; using System.Linq; using System.Text; using System.Threading; @@ -25,6 +27,7 @@ using Microsoft.Python.Analysis.Analyzer.Expressions; using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Diagnostics; +using Microsoft.Python.Analysis.Modules; using Microsoft.Python.Analysis.Types; using Microsoft.Python.Analysis.Values; using Microsoft.Python.Core; @@ -65,7 +68,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis return Enumerable.Empty(); } - // find path to module that might have symbols for us + // find path to module that might have symbols for us and load them await EnsureCandidateModulesAsync(analysis, name, cancellationToken); var fullyQualifiedNames = new HashSet(); @@ -79,9 +82,17 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis var visited = new HashSet(); var nameParts = new List(); foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { + if (module.ModuleType == ModuleType.Unresolved) { + continue; + } + + // module name is full module name that you can use in import xxxx directly nameParts.Add(module.Name); CollectCandidates(module, name, visited, nameParts, fullyQualifiedNames, cancellationToken); nameParts.RemoveAt(nameParts.Count - 1); + + Debug.Assert(visited.Count == 0); + Debug.Assert(nameParts.Count == 0); } var codeActions = new List(); @@ -295,6 +306,7 @@ private void CollectCandidates(IPythonModule module, HashSet fullyQualifiedNames, CancellationToken cancellationToken) { if (module == null || !visited.Add(module)) { + // bail out on circular imports return; } @@ -302,6 +314,15 @@ private void CollectCandidates(IPythonModule module, AddNonImportedMemberWithName(module, name, nameParts, fullyQualifiedNames); // add module (imported) members if it shows up in __all__ + // + // we are doing recursive dig down rather than just going through all modules loaded linearly + // since path to how to get to a module is important. + // for example, "join" is defined in "ntpath" or "macpath" and etc, but users are supposed to + // use it through "os.path" which will automatically point to right module ex, "ntpath" based on + // environment rather than "ntpath" directly. if we just go through module in flat list, then + // we can miss "os.path" since it won't show in the module list. + // for these modules that are supposed to be used with indirect path (imported name of the module), + // we need to dig down to collect those with right path. foreach (var memberName in GetAllVariables(module.Analysis)) { cancellationToken.ThrowIfCancellationRequested(); @@ -318,6 +339,11 @@ private void CollectCandidates(IPythonModule module, CollectCandidates(pythonModule, name, visited, nameParts, fullyQualifiedNames, cancellationToken); nameParts.RemoveAt(nameParts.Count - 1); } + + // pop this module out so we can get to this module from + // different path. + // ex) A -> B -> [C] and A -> D -> [C] + visited.Remove(module); } private void AddNonImportedMemberWithName(IPythonModule module, string name, List nameParts, HashSet fullyQualifiedNames) { From c1a8eec86d63b2a57c6878b2174f317f49d52e66 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 01:17:07 -0700 Subject: [PATCH 23/43] handle circular imports better. some refactorings and etc. --- .../MissingImportCodeActionProvider.cs | 160 +++++++++++++----- 1 file changed, 118 insertions(+), 42 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 343bded3f..57089041c 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -71,34 +71,39 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis // find path to module that might have symbols for us and load them await EnsureCandidateModulesAsync(analysis, name, cancellationToken); - var fullyQualifiedNames = new HashSet(); + var moduleFullNameMap = new Dictionary(); // find modules matching the given name. this will include submodules var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); - fullyQualifiedNames.UnionWith(pathResolver.GetAllImportableModulesByName(name, includeImplicit)); + foreach (var moduleFullName in pathResolver.GetAllImportableModulesByName(name, includeImplicit)) { + var module = interpreter.ModuleResolution.GetOrLoadModule(moduleFullName); + if (module == null) { + continue; + } + + moduleFullNameMap[moduleFullName] = new ImportInfo(moduleImported: false, memberImported: false, module); + } // find members matching the given name from module already imported. - var visited = new HashSet(); - var nameParts = new List(); + var moduleInfo = new ModuleInfo(module: null); foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { if (module.ModuleType == ModuleType.Unresolved) { continue; } // module name is full module name that you can use in import xxxx directly - nameParts.Add(module.Name); - CollectCandidates(module, name, visited, nameParts, fullyQualifiedNames, cancellationToken); - nameParts.RemoveAt(nameParts.Count - 1); - - Debug.Assert(visited.Count == 0); - Debug.Assert(nameParts.Count == 0); + CollectCandidates(moduleInfo.Reset(module), name, moduleFullNameMap, cancellationToken); + Debug.Assert(moduleInfo.NameParts.Count == 1 && moduleInfo.NameParts[0] == module.Name); } + // TODO: improve on hueristic on how to order code fixes + // var groups = moduleFullNameMap.GroupBy(kv => kv.Value.Symbol.Definition, LocationInfo.FullComparer).ToDictionary(g => g.Key, g => g.ToDictionary(kv => kv.Key, kv => kv.Value)); + var codeActions = new List(); - foreach (var fullyQualifiedName in fullyQualifiedNames.OrderBy(n => n, ModuleNameComparer.Instance)) { + foreach (var kv in moduleFullNameMap.OrderBy(n => n, ModuleNameComparer.Instance)) { cancellationToken.ThrowIfCancellationRequested(); - codeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullyQualifiedName, locallyInserted: false, cancellationToken)); + codeActions.AddIfNotNull(CreateCodeAction(analysis, node, kv.Key, locallyInserted: false, cancellationToken)); } return codeActions; @@ -143,10 +148,10 @@ private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis private CodeAction CreateCodeAction(IDocumentAnalysis analysis, Node node, - string fullyQualifiedName, + string moduleFullName, bool locallyInserted, CancellationToken cancellationToken) { - var insertionPoint = GetInsertionInfo(analysis, node, fullyQualifiedName, locallyInserted, cancellationToken); + var insertionPoint = GetInsertionInfo(analysis, node, moduleFullName, locallyInserted, cancellationToken); if (insertionPoint == null) { return null; } @@ -299,19 +304,17 @@ private List GetParents(Node root, Node node, CancellationToken cancellati return parentChain; } - private void CollectCandidates(IPythonModule module, + private void CollectCandidates(ModuleInfo moduleInfo, string name, - HashSet visited, - List nameParts, - HashSet fullyQualifiedNames, + Dictionary moduleFullNameMap, CancellationToken cancellationToken) { - if (module == null || !visited.Add(module)) { + if (!moduleInfo.CheckCircularImports()) { // bail out on circular imports return; } // add non module (imported) member - AddNonImportedMemberWithName(module, name, nameParts, fullyQualifiedNames); + AddNonImportedMemberWithName(moduleInfo, name, moduleFullNameMap); // add module (imported) members if it shows up in __all__ // @@ -323,54 +326,63 @@ private void CollectCandidates(IPythonModule module, // we can miss "os.path" since it won't show in the module list. // for these modules that are supposed to be used with indirect path (imported name of the module), // we need to dig down to collect those with right path. - foreach (var memberName in GetAllVariables(module.Analysis)) { + foreach (var memberName in GetAllVariables(moduleInfo.Analysis)) { cancellationToken.ThrowIfCancellationRequested(); - var pythonModule = module.GetMember(memberName) as IPythonModule; + var pythonModule = moduleInfo.Module.GetMember(memberName) as IPythonModule; if (pythonModule == null) { continue; } - nameParts.Add(memberName); + moduleInfo.AddName(memberName); if (string.Equals(memberName, name)) { - AddNameParts(nameParts, fullyQualifiedNames); + // nested module are all imported + AddNameParts(moduleInfo.FullName, moduleImported: true, memberImported: true, pythonModule, moduleFullNameMap); } - CollectCandidates(pythonModule, name, visited, nameParts, fullyQualifiedNames, cancellationToken); - nameParts.RemoveAt(nameParts.Count - 1); + CollectCandidates(moduleInfo.With(pythonModule), name, moduleFullNameMap, cancellationToken); + moduleInfo.PopName(); } // pop this module out so we can get to this module from // different path. // ex) A -> B -> [C] and A -> D -> [C] - visited.Remove(module); + moduleInfo.ForgetModule(); } - private void AddNonImportedMemberWithName(IPythonModule module, string name, List nameParts, HashSet fullyQualifiedNames) { + private void AddNonImportedMemberWithName(ModuleInfo moduleInfo, string name, Dictionary moduleFullNameMap) { // for now, skip any protected or private member if (name.StartsWith("_")) { return; } - var pythonType = module.GetMember(name); + var pythonType = moduleInfo.Module.GetMember(name); if (pythonType == null || pythonType is IPythonModule || pythonType.IsUnknown()) { return; } // skip any imported member (non module member) unless it is explicitly on __all__ - if (module.Analysis.GlobalScope.Imported.TryGetVariable(name, out var imported) && - object.Equals(pythonType, imported.Value) && - GetAllVariables(module.Analysis).All(s => !string.Equals(s, name))) { + if (moduleInfo.Analysis.GlobalScope.Imported.TryGetVariable(name, out var importedVariable) && + object.Equals(pythonType, importedVariable.Value) && + GetAllVariables(moduleInfo.Analysis).All(s => !string.Equals(s, name))) { return; } - nameParts.Add(name); - AddNameParts(nameParts, fullyQualifiedNames); - nameParts.RemoveAt(nameParts.Count - 1); + moduleInfo.AddName(name); + AddNameParts(moduleInfo.FullName, moduleInfo.ModuleImported, importedVariable != null, pythonType, moduleFullNameMap); + moduleInfo.PopName(); } - private static bool AddNameParts(List nameParts, HashSet fullyQualifiedNames) { - return fullyQualifiedNames.Add(string.Join('.', nameParts)); + private static void AddNameParts( + string fullName, bool moduleImported, bool memberImported, IPythonType symbol, Dictionary moduleFullNameMap) { + // one of case this can happen is if module's fullname is "a.b.c" and module "a.b" also import module "a.b.c" as "c" making + // fullname same "a.b.c". in this case, we mark it as "imported" since we refer one explicily shown in "__all__" to show + // higher rank than others + if (moduleFullNameMap.TryGetValue(fullName, out var info)) { + moduleImported |= info.ModuleImported; + } + + moduleFullNameMap[fullName] = new ImportInfo(moduleImported, memberImported, symbol); } private IEnumerable GetAllVariables(IDocumentAnalysis analysis) { @@ -387,20 +399,26 @@ private IEnumerable GetAllVariables(IDocumentAnalysis analysis) { return Array.Empty(); } - private class ModuleNameComparer : IComparer { + private class ModuleNameComparer : IComparer> { public static readonly ModuleNameComparer Instance = new ModuleNameComparer(); private ModuleNameComparer() { } - public int Compare(string x, string y) { + public int Compare(KeyValuePair x, KeyValuePair y) { + return CompareModuleFullName(x.Key, y.Key); + } + + private int CompareModuleFullName(string x, string y) { + const string underscore = "_"; + // move "_" to back of the list - if (x.StartsWith("_") && y.StartsWith("_")) { + if (x.StartsWith(underscore) && y.StartsWith(underscore)) { return x.CompareTo(y); } - if (x.StartsWith("_")) { + if (x.StartsWith(underscore)) { return 1; } - if (y.StartsWith("_")) { + if (y.StartsWith(underscore)) { return -1; } @@ -421,6 +439,64 @@ public InsertionInfo(bool addBlankLine, string insertionText, Range range, strin Indentation = indentation; } } + + private struct ModuleInfo { + public readonly IPythonModule Module; + public readonly List NameParts; + public readonly bool ModuleImported; + + private readonly HashSet _visited; + + public IDocumentAnalysis Analysis => Module.Analysis; + public string FullName => string.Join('.', NameParts); + + public ModuleInfo(IPythonModule module) : + this(module, new List(), moduleImported: false) { + } + + private ModuleInfo(IPythonModule module, List nameParts, bool moduleImported) : + this() { + Module = module; + NameParts = nameParts; + ModuleImported = moduleImported; + + _visited = new HashSet(); + } + + public bool CheckCircularImports() => Module != null && _visited.Add(Module); + public void ForgetModule() => _visited.Remove(Module); + + public void AddName(string memberName) => NameParts.Add(memberName); + public void PopName() => NameParts.RemoveAt(NameParts.Count - 1); + + public ModuleInfo With(IPythonModule module) { + return new ModuleInfo(module, NameParts, moduleImported: true); + } + + public ModuleInfo Reset(IPythonModule module) { + Debug.Assert(_visited.Count == 0); + + NameParts.Clear(); + NameParts.Add(module.Name); + + return new ModuleInfo(module, NameParts, moduleImported: false); + } + } + + [DebuggerDisplay("{Symbol.Name} {Symbol.MemberType} {ModuleImported} {MemberImported}")] + private struct ImportInfo { + // only one that shows up in "__all__" will be imported + public readonly bool ModuleImported; + public readonly bool MemberImported; + + public readonly IPythonType Symbol; + + public ImportInfo(bool moduleImported, bool memberImported, IPythonType symbol) { + ModuleImported = moduleImported; + MemberImported = memberImported; + Symbol = symbol; + } + } } } From bc854e1daede612f1cd4760d6ca66dbb17b8e35a Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 02:46:24 -0700 Subject: [PATCH 24/43] tweaked code action ordering a bit --- .../MissingImportCodeActionProvider.cs | 99 ++++++++++++++----- 1 file changed, 77 insertions(+), 22 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 57089041c..609d253a8 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -71,7 +71,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis // find path to module that might have symbols for us and load them await EnsureCandidateModulesAsync(analysis, name, cancellationToken); - var moduleFullNameMap = new Dictionary(); + var importFullNameMap = new Dictionary(); // find modules matching the given name. this will include submodules var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); @@ -82,7 +82,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis continue; } - moduleFullNameMap[moduleFullName] = new ImportInfo(moduleImported: false, memberImported: false, module); + importFullNameMap[moduleFullName] = new ImportInfo(moduleImported: false, memberImported: false, module); } // find members matching the given name from module already imported. @@ -93,22 +93,81 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis } // module name is full module name that you can use in import xxxx directly - CollectCandidates(moduleInfo.Reset(module), name, moduleFullNameMap, cancellationToken); + CollectCandidates(moduleInfo.Reset(module), name, importFullNameMap, cancellationToken); Debug.Assert(moduleInfo.NameParts.Count == 1 && moduleInfo.NameParts[0] == module.Name); } - // TODO: improve on hueristic on how to order code fixes - // var groups = moduleFullNameMap.GroupBy(kv => kv.Value.Symbol.Definition, LocationInfo.FullComparer).ToDictionary(g => g.Key, g => g.ToDictionary(kv => kv.Key, kv => kv.Value)); - var codeActions = new List(); - foreach (var kv in moduleFullNameMap.OrderBy(n => n, ModuleNameComparer.Instance)) { + foreach (var fullName in OrderFullNames(importFullNameMap)) { cancellationToken.ThrowIfCancellationRequested(); - codeActions.AddIfNotNull(CreateCodeAction(analysis, node, kv.Key, locallyInserted: false, cancellationToken)); + codeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullName, locallyInserted: false, cancellationToken)); } return codeActions; } + private IEnumerable OrderFullNames(Dictionary importFullNameMap) { + // use some heuristic to improve code fix ordering + + // heuristic is we put entries with decl without any exports at the top + // such as array + var sourceDeclarationFullNames = importFullNameMap.GroupBy(kv => kv.Value.Symbol.Definition, LocationInfo.FullComparer) + .Where(FilterSourceDeclarations) + .Select(g => g.First().Key); + + foreach (var fullName in OrderImportNames(sourceDeclarationFullNames)) { + importFullNameMap.Remove(fullName); + yield return fullName; + } + + // put modules that are imported next + foreach (var fullName in OrderImportNames(importFullNameMap.Where(FilterImportedModules).Select(kv => kv.Key))) { + importFullNameMap.Remove(fullName); + yield return fullName; + } + + // put members that are imported next + foreach (var fullName in OrderImportNames(importFullNameMap.Where(FilterImportedMembers).Select(kv => kv.Key))) { + importFullNameMap.Remove(fullName); + yield return fullName; + } + + // put members whose module is imported next + foreach (var fullName in OrderImportNames(importFullNameMap.Where(FilterImportedModuleMembers).Select(kv => kv.Key))) { + importFullNameMap.Remove(fullName); + yield return fullName; + } + + // put things left here. + foreach (var fullName in OrderImportNames(importFullNameMap.Select(kv => kv.Key))) { + yield return fullName; + } + + List OrderImportNames(IEnumerable fullNames) { + return fullNames.OrderBy(n => n, ImportNameComparer.Instance).ToList(); + } + + bool FilterImportedMembers(KeyValuePair kv) => kv.Value.Symbol.MemberType != PythonMemberType.Module && kv.Value.MemberImported; + bool FilterImportedModuleMembers(KeyValuePair kv) => kv.Value.Symbol.MemberType != PythonMemberType.Module && kv.Value.ModuleImported; + bool FilterImportedModules(KeyValuePair kv) => kv.Value.Symbol.MemberType == PythonMemberType.Module && kv.Value.MemberImported; + + bool FilterSourceDeclarations(IGrouping> group) { + var count = 0; + foreach (var entry in group) { + if (count++ > 0) { + return false; + } + + var value = entry.Value; + if (value.ModuleImported || value.ModuleImported) { + return false; + } + } + + return true; + } + } + private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis, string name, CancellationToken cancellationToken) { @@ -306,7 +365,7 @@ private List GetParents(Node root, Node node, CancellationToken cancellati private void CollectCandidates(ModuleInfo moduleInfo, string name, - Dictionary moduleFullNameMap, + Dictionary importFullNameMap, CancellationToken cancellationToken) { if (!moduleInfo.CheckCircularImports()) { // bail out on circular imports @@ -314,7 +373,7 @@ private void CollectCandidates(ModuleInfo moduleInfo, } // add non module (imported) member - AddNonImportedMemberWithName(moduleInfo, name, moduleFullNameMap); + AddNonImportedMemberWithName(moduleInfo, name, importFullNameMap); // add module (imported) members if it shows up in __all__ // @@ -337,10 +396,10 @@ private void CollectCandidates(ModuleInfo moduleInfo, moduleInfo.AddName(memberName); if (string.Equals(memberName, name)) { // nested module are all imported - AddNameParts(moduleInfo.FullName, moduleImported: true, memberImported: true, pythonModule, moduleFullNameMap); + AddNameParts(moduleInfo.FullName, moduleImported: true, memberImported: true, pythonModule, importFullNameMap); } - CollectCandidates(moduleInfo.With(pythonModule), name, moduleFullNameMap, cancellationToken); + CollectCandidates(moduleInfo.With(pythonModule), name, importFullNameMap, cancellationToken); moduleInfo.PopName(); } @@ -350,7 +409,7 @@ private void CollectCandidates(ModuleInfo moduleInfo, moduleInfo.ForgetModule(); } - private void AddNonImportedMemberWithName(ModuleInfo moduleInfo, string name, Dictionary moduleFullNameMap) { + private void AddNonImportedMemberWithName(ModuleInfo moduleInfo, string name, Dictionary importFullNameMap) { // for now, skip any protected or private member if (name.StartsWith("_")) { return; @@ -369,7 +428,7 @@ private void AddNonImportedMemberWithName(ModuleInfo moduleInfo, string name, Di } moduleInfo.AddName(name); - AddNameParts(moduleInfo.FullName, moduleInfo.ModuleImported, importedVariable != null, pythonType, moduleFullNameMap); + AddNameParts(moduleInfo.FullName, moduleInfo.ModuleImported, importedVariable != null, pythonType, importFullNameMap); moduleInfo.PopName(); } @@ -399,16 +458,12 @@ private IEnumerable GetAllVariables(IDocumentAnalysis analysis) { return Array.Empty(); } - private class ModuleNameComparer : IComparer> { - public static readonly ModuleNameComparer Instance = new ModuleNameComparer(); - - private ModuleNameComparer() { } + private class ImportNameComparer : IComparer { + public static readonly ImportNameComparer Instance = new ImportNameComparer(); - public int Compare(KeyValuePair x, KeyValuePair y) { - return CompareModuleFullName(x.Key, y.Key); - } + private ImportNameComparer() { } - private int CompareModuleFullName(string x, string y) { + public int Compare(string x, string y) { const string underscore = "_"; // move "_" to back of the list From f5d3b2503fce92a5e9f2c20b28ffe972c8bcd359 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 12:07:37 -0700 Subject: [PATCH 25/43] fix push/pop mismatch due to class decl being duplicated issue --- src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs b/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs index 5fbcddeb5..27f104229 100644 --- a/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs +++ b/src/Analysis/Ast/Impl/Analyzer/Symbols/SymbolCollector.cs @@ -74,7 +74,9 @@ public override bool Walk(ClassDefinition cd) { } public override void PostWalk(ClassDefinition cd) { - if (!IsDeprecated(cd) && !string.IsNullOrEmpty(cd.NameExpression?.Name)) { + if (!IsDeprecated(cd) && + !string.IsNullOrEmpty(cd.NameExpression?.Name) && + _typeMap.ContainsKey(cd)) { _scopes.Pop().Dispose(); } base.PostWalk(cd); From 6aa8c94b902d174a0f165eccb4e19b21200a6307 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 12:07:45 -0700 Subject: [PATCH 26/43] added test for symbol ordering --- .../Test/MissingImportCodeActionTests.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 382f170dd..0d9b112f0 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -255,6 +255,23 @@ public async Task SymbolOrdering() { maxIndexOfPublicSymbol.Should().BeLessThan(minIndexOfPrivateSymbol); } + [TestMethod, Priority(0)] + public async Task SymbolOrdering2() { + var markup = @"{|insertionSpan:|}{|diagnostic:join|}"; + + var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics, enableIndexManager: true); + + var list = codeActions.Select(c => c.title).ToList(); + var zipList = Enumerable.Range(0, list.Count).Zip(list); + + var sourceDeclIndex = zipList.First(t => t.Second == "from macpath import join").First; + var importedMemberIndex = zipList.First(t => t.Second == "from os.path import join").First; + var restIndex = zipList.First(t => t.Second == "from ntpath import join").First; + + sourceDeclIndex.Should().BeLessThan(importedMemberIndex); + importedMemberIndex.Should().BeLessThan(restIndex); + } + [TestMethod, Priority(0)] public async Task ModuleNotReachableFromUserDocument() { await TestCodeActionAsync( From ade67a47d2613ca0556eb7a85c80d25e99f9bcbc Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 13:39:41 -0700 Subject: [PATCH 27/43] added abbreviation for well known module --- .../MissingImportCodeActionProvider.cs | 52 ++++++++++++------- .../Test/MissingImportCodeActionTests.cs | 29 +++++++++++ 2 files changed, 63 insertions(+), 18 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 609d253a8..a65c6e8b3 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -25,6 +25,7 @@ using Microsoft.Python.Analysis; using Microsoft.Python.Analysis.Analyzer; using Microsoft.Python.Analysis.Analyzer.Expressions; +using Microsoft.Python.Analysis.Caching; using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Diagnostics; using Microsoft.Python.Analysis.Modules; @@ -44,6 +45,14 @@ internal sealed class MissingImportCodeActionProvider : ICodeActionProvider { public static readonly ICodeActionProvider Instance = new MissingImportCodeActionProvider(); + // right now, it is a static. in future, we might consider giving an option to users to customize this list + // also, right now, it is text based. so if module has same name, they will get same suggestion even if + // the module is not something user expected + private static readonly Dictionary WellKnownAbbreviationMap = new Dictionary() { + { "numpy", "np" }, + { "pandas", "pd" } + }; + private MissingImportCodeActionProvider() { } @@ -223,21 +232,15 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, sb.Append(insertionPoint.Value.AddBlankLine ? insertionText + Environment.NewLine : insertionText); sb.AppendIf(insertionPoint.Value.Range.start == insertionPoint.Value.Range.end, Environment.NewLine); - var changes = new Dictionary {{ - analysis.Document.Uri, - new TextEdit[] { - new TextEdit() { - range = insertionPoint.Value.Range, - newText = sb.ToString() - }} - }}; - - return new CodeAction() { - title = titleText, - edit = new WorkspaceEdit() { - changes = changes - } - }; + var textEdits = new List(); + textEdits.Add(new TextEdit() { range = insertionPoint.Value.Range, newText = sb.ToString() }); + + if (insertionPoint.Value.AbbreviationOpt != null) { + textEdits.Add(new TextEdit() { range = node.GetSpan(analysis.Ast), newText = insertionPoint.Value.AbbreviationOpt }); + } + + var changes = new Dictionary { { analysis.Document.Uri, textEdits.ToArray() } }; + return new CodeAction() { title = titleText, edit = new WorkspaceEdit() { changes = changes } }; } private InsertionInfo? GetInsertionInfo(IDocumentAnalysis analysis, @@ -258,10 +261,13 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, var dotIndex = fullyQualifiedName.LastIndexOf('.'); if (dotIndex < 0) { // there can't be existing import since we have the error + WellKnownAbbreviationMap.TryGetValue(fullyQualifiedName, out var abbreviation); + return new InsertionInfo(addBlankLine: lastImportNode == null, - $"import {fullyQualifiedName}", + GetInsertionText(fullyQualifiedName, abbreviation), GetRange(analysis.Ast, body, lastImportNode), - indentation); + indentation, + abbreviation); } // see whether there is existing from * import * statement. @@ -283,6 +289,14 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, indentation); } + private static string GetInsertionText(string module, string abbreviation) { + if (abbreviation == null) { + return $"import {module}"; + } + + return $"import {module} as {abbreviation}"; + } + private string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { var imports = fromImportStatement.Names.Select(n => n.Name) .Concat(new string[] { moduleNameToAdd }) @@ -486,12 +500,14 @@ private struct InsertionInfo { public readonly string InsertionText; public readonly Range Range; public readonly string Indentation; + public readonly string AbbreviationOpt; - public InsertionInfo(bool addBlankLine, string insertionText, Range range, string indentation) { + public InsertionInfo(bool addBlankLine, string insertionText, Range range, string indentation, string abbreviationOpt = null) { AddBlankLine = addBlankLine; InsertionText = insertionText; Range = range; Indentation = indentation; + AbbreviationOpt = abbreviationOpt; } } diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 0d9b112f0..17a076bb5 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -21,6 +21,7 @@ using FluentAssertions; using Microsoft.Python.Analysis; using Microsoft.Python.Analysis.Analyzer; +using Microsoft.Python.Analysis.Documents; using Microsoft.Python.Core.Idle; using Microsoft.Python.Core.IO; using Microsoft.Python.Core.Services; @@ -281,6 +282,34 @@ await TestCodeActionAsync( enableIndexManager: true); } + [TestMethod, Priority(0)] + public async Task SuggestAbbreviationForKnownModule() { + MarkupUtils.GetNamedSpans(@"{|insertionSpan:|}{|diagnostic:pandas|}", out var code, out var spans); + + // get main analysis and add mock pandas module + var analysis = await GetAnalysisAsync(code); + await GetAnalysisAsync("", analysis.ExpressionEvaluator.Services, modulePath: TestData.GetTestSpecificPath("pandas.py")); + + // calculate actions + var diagnostics = GetDiagnostics(analysis, spans["diagnostic"].First().ToSourceSpan(analysis.Ast), MissingImportCodeActionProvider.Instance.FixableDiagnostics); + var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); + + var docTable = analysis.ExpressionEvaluator.Services.GetService(); + + // verify results + var codeAction = codeActions.Single(c => c.title == "import pandas as pd"); + codeAction.edit.changes.Should().HaveCount(1); + + var edits = codeAction.edit.changes[analysis.Document.Uri]; + edits.Should().HaveCount(2); + + var invocationEdit = edits.Single(e => e.newText == "pd"); + invocationEdit.range.Should().Be(spans["diagnostic"].First().ToSourceSpan(analysis.Ast)); + + var insertEdit = edits.Single(e => e.newText == "import pandas as pd" + Environment.NewLine + Environment.NewLine); + insertEdit.range.Should().Be(spans["insertionSpan"].First().ToSourceSpan(analysis.Ast)); + } + private async Task TestCodeActionAsync(string markup, string title, string newText, bool enableIndexManager = false) { var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics, enableIndexManager); From 79fa68fc3f7633d63b24faa2ebddc862a75e477d Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 14:00:39 -0700 Subject: [PATCH 28/43] added some comments --- .../MissingImportCodeActionProvider.cs | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index a65c6e8b3..88dbbedcd 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -60,10 +60,7 @@ private MissingImportCodeActionProvider() { ErrorCodes.UndefinedVariable, ErrorCodes.VariableNotDefinedGlobally, ErrorCodes.VariableNotDefinedNonLocal); public async Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { - // * TODO * for now, complete. since I don't know what each option mean. var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); - - // * TODO * need to check whether it is expected node kind or add code context check to verify it is a place where module/type can appear var node = finder.GetExpression(diagnostic.SourceSpan); if (!(node is NameExpression)) { return Enumerable.Empty(); @@ -77,12 +74,11 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis return Enumerable.Empty(); } - // find path to module that might have symbols for us and load them - await EnsureCandidateModulesAsync(analysis, name, cancellationToken); + await EnsureCandidateModulesLoadedAsync(analysis, name, cancellationToken); var importFullNameMap = new Dictionary(); - // find modules matching the given name. this will include submodules + // find installed modules matching the given name. this will include submodules var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); foreach (var moduleFullName in pathResolver.GetAllImportableModulesByName(name, includeImplicit)) { @@ -94,7 +90,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis importFullNameMap[moduleFullName] = new ImportInfo(moduleImported: false, memberImported: false, module); } - // find members matching the given name from module already imported. + // find members matching the given name from modules already loaded. var moduleInfo = new ModuleInfo(module: null); foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { if (module.ModuleType == ModuleType.Unresolved) { @@ -106,6 +102,9 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis Debug.Assert(moduleInfo.NameParts.Count == 1 && moduleInfo.NameParts[0] == module.Name); } + // filter member found based on context + + // this will create actual code fix with certain orders var codeActions = new List(); foreach (var fullName in OrderFullNames(importFullNameMap)) { cancellationToken.ThrowIfCancellationRequested(); @@ -119,7 +118,13 @@ private IEnumerable OrderFullNames(Dictionary import // use some heuristic to improve code fix ordering // heuristic is we put entries with decl without any exports at the top - // such as array + // such as array. another example will be chararray. + // this will make numpy chararray at the top and numpy defchararray at the bottom. + // if we want, we can add more info to hide intermediate ones. + // for example, numpy.chararry is __all__.extended from numpy.core.chararray and etc. + // so we could leave only numpy.chararray and remove ones like numpy.core.chararray and etc. but for now, + // we show all those but in certain order so that numpy.chararray shows up top + // this heuristic still has issue with something like os.path.join since no one import macpath, macpath join shows up high var sourceDeclarationFullNames = importFullNameMap.GroupBy(kv => kv.Value.Symbol.Definition, LocationInfo.FullComparer) .Where(FilterSourceDeclarations) .Select(g => g.First().Key); @@ -177,9 +182,9 @@ bool FilterSourceDeclarations(IGrouping(); if (indexManager == null) { // indexing is not supported @@ -187,6 +192,8 @@ private static async Task EnsureCandidateModulesAsync(IDocumentAnalysis analysis } var symbolsIncludingName = await indexManager.WorkspaceSymbolsAsync(name, maxLength: int.MaxValue, includeLibraries: true, cancellationToken); + + // we only consider exact matches rather than partial matches var symbolsWithName = symbolsIncludingName.Where(s => s.Name == name && s.Kind != Indexing.SymbolKind.Variable); var analyzer = analysis.ExpressionEvaluator.Services.GetService(); @@ -557,7 +564,9 @@ public ModuleInfo Reset(IPythonModule module) { [DebuggerDisplay("{Symbol.Name} {Symbol.MemberType} {ModuleImported} {MemberImported}")] private struct ImportInfo { // only one that shows up in "__all__" will be imported + // containing module is imported public readonly bool ModuleImported; + // containing symbol is imported public readonly bool MemberImported; public readonly IPythonType Symbol; From df49da7b8b7deea395ee494ad44088028af1427c Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 14:50:06 -0700 Subject: [PATCH 29/43] added context based filtering --- .../MissingImportCodeActionProvider.cs | 33 +++++++++++++++++-- .../Test/MissingImportCodeActionTests.cs | 19 +++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 88dbbedcd..9f5c3ac81 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -25,7 +25,6 @@ using Microsoft.Python.Analysis; using Microsoft.Python.Analysis.Analyzer; using Microsoft.Python.Analysis.Analyzer.Expressions; -using Microsoft.Python.Analysis.Caching; using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Diagnostics; using Microsoft.Python.Analysis.Modules; @@ -82,6 +81,8 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); foreach (var moduleFullName in pathResolver.GetAllImportableModulesByName(name, includeImplicit)) { + cancellationToken.ThrowIfCancellationRequested(); + var module = interpreter.ModuleResolution.GetOrLoadModule(moduleFullName); if (module == null) { continue; @@ -103,6 +104,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis } // filter member found based on context + FilterOnConext(analysis, node, importFullNameMap, cancellationToken); // this will create actual code fix with certain orders var codeActions = new List(); @@ -114,6 +116,29 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis return codeActions; } + private void FilterOnConext(IDocumentAnalysis analysis, Node node, Dictionary importFullNameMap, CancellationToken cancellationToken) { + var ancestors = GetAncestorsOrThis(analysis.Ast.Body, node, cancellationToken); + var index = ancestors.LastIndexOf(node); + if (index <= 0) { + // nothing to filter on + return; + } + + var parent = ancestors[index - 1]; + if (!(parent is CallExpression)) { + // nothing to filter on + return; + } + + // do simple filtering + // remove all modules from candidates + foreach (var kv in importFullNameMap.ToList()) { + if (kv.Value.Symbol.MemberType == PythonMemberType.Module) { + importFullNameMap.Remove(kv.Key); + } + } + } + private IEnumerable OrderFullNames(Dictionary importFullNameMap) { // use some heuristic to improve code fix ordering @@ -201,6 +226,8 @@ private static async Task EnsureCandidateModulesLoadedAsync(IDocumentAnalysis an var modules = ImmutableArray.Empty; foreach (var moduleName in symbolsWithName.Select(s => s.DocumentPath).Distinct().Select(p => pathResolver.GetModuleNameByPath(p))) { + cancellationToken.ThrowIfCancellationRequested(); + var module = analysis.Document.Interpreter.ModuleResolution.GetOrLoadModule(moduleName); if (module.Analysis is EmptyAnalysis) { // once module is analyzed, this analysis result will be kept in memory @@ -335,7 +362,7 @@ private Position GetPosition(PythonAst ast, Statement body, Node lastImportNode) return (analysis.Ast.Body, string.Empty); } - var candidate = GetParents(analysis.Ast.Body, node, cancellationToken).Where(p => p is FunctionDefinition).LastOrDefault(); + var candidate = GetAncestorsOrThis(analysis.Ast.Body, node, cancellationToken).Where(p => p is FunctionDefinition).LastOrDefault(); // for now, only stop at FunctionDefinition. // we can expand it to more scope if we want but this seems what other tool also provide as well. @@ -358,7 +385,7 @@ private string GetIndentation(PythonAst ast, Statement body) { return new string(' ', firstToken.GetStart(ast).Column - 1); } - private List GetParents(Node root, Node node, CancellationToken cancellationToken) { + private List GetAncestorsOrThis(Node root, Node node, CancellationToken cancellationToken) { var parentChain = new List(); // there seems no way to go up the parent chain. always has to go down from the top diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 17a076bb5..b126f3adb 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -310,6 +310,25 @@ public async Task SuggestAbbreviationForKnownModule() { insertEdit.range.Should().Be(spans["insertionSpan"].First().ToSourceSpan(analysis.Ast)); } + [TestMethod, Priority(0)] + public async Task ContextBasedSuggestion() { + var markup = + @"from os import path +{|insertionSpan:|} +{|diagnostic:socket|}()"; + + var (analysis, codeActions, insertionSpan) = + await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + + codeActions.Should().NotContain(c => c.title == "import socket"); + + var title = "from socket import socket"; + var newText = "from socket import socket" + Environment.NewLine; + + var codeAction = codeActions.Single(c => c.title == title); + TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); + } + private async Task TestCodeActionAsync(string markup, string title, string newText, bool enableIndexManager = false) { var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics, enableIndexManager); From 14ae66c5ebc22e84d8a0d4cc24dbbacf507aa454 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 14:55:57 -0700 Subject: [PATCH 30/43] clean up --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 9f5c3ac81..9ec938b0d 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -103,8 +103,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis Debug.Assert(moduleInfo.NameParts.Count == 1 && moduleInfo.NameParts[0] == module.Name); } - // filter member found based on context - FilterOnConext(analysis, node, importFullNameMap, cancellationToken); + FilterCandidatesBasedOnContext(analysis, node, importFullNameMap, cancellationToken); // this will create actual code fix with certain orders var codeActions = new List(); @@ -116,7 +115,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis return codeActions; } - private void FilterOnConext(IDocumentAnalysis analysis, Node node, Dictionary importFullNameMap, CancellationToken cancellationToken) { + private void FilterCandidatesBasedOnContext(IDocumentAnalysis analysis, Node node, Dictionary importFullNameMap, CancellationToken cancellationToken) { var ancestors = GetAncestorsOrThis(analysis.Ast.Body, node, cancellationToken); var index = ancestors.LastIndexOf(node); if (index <= 0) { From bf14ff6f7bcbf45fefafec15fc4dcffeb391fb09 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 15:50:51 -0700 Subject: [PATCH 31/43] make sure we don't suggest imports that can be used in import statement --- .../MissingImportCodeActionProvider.cs | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 9ec938b0d..208f88439 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -25,6 +25,7 @@ using Microsoft.Python.Analysis; using Microsoft.Python.Analysis.Analyzer; using Microsoft.Python.Analysis.Analyzer.Expressions; +using Microsoft.Python.Analysis.Core.DependencyResolution; using Microsoft.Python.Analysis.Core.Interpreter; using Microsoft.Python.Analysis.Diagnostics; using Microsoft.Python.Analysis.Modules; @@ -92,7 +93,7 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis } // find members matching the given name from modules already loaded. - var moduleInfo = new ModuleInfo(module: null); + var moduleInfo = new ModuleInfo(analysis); foreach (var module in interpreter.ModuleResolution.GetImportedModules(cancellationToken)) { if (module.ModuleType == ModuleType.Unresolved) { continue; @@ -440,6 +441,18 @@ private void CollectCandidates(ModuleInfo moduleInfo, continue; } + // make sure we dig down modules only if we can use it from imports + var result = AstUtilities.FindImports( + moduleInfo.CurrentFileAnalysis.Document.Interpreter.ModuleResolution.CurrentPathResolver, + moduleInfo.CurrentFileAnalysis.Document.FilePath, + GetRootNames($"{moduleInfo.FullName}.{memberName}"), + dotCount: 0, + forceAbsolute: true); + + if (result is ImportNotFound) { + continue; + } + moduleInfo.AddName(memberName); if (string.Equals(memberName, name)) { // nested module are all imported @@ -456,6 +469,10 @@ private void CollectCandidates(ModuleInfo moduleInfo, moduleInfo.ForgetModule(); } + private IEnumerable GetRootNames(string fullName) { + return fullName.Split('.'); + } + private void AddNonImportedMemberWithName(ModuleInfo moduleInfo, string name, Dictionary importFullNameMap) { // for now, skip any protected or private member if (name.StartsWith("_")) { @@ -545,6 +562,7 @@ public InsertionInfo(bool addBlankLine, string insertionText, Range range, strin } private struct ModuleInfo { + public readonly IDocumentAnalysis CurrentFileAnalysis; public readonly IPythonModule Module; public readonly List NameParts; public readonly bool ModuleImported; @@ -554,12 +572,13 @@ private struct ModuleInfo { public IDocumentAnalysis Analysis => Module.Analysis; public string FullName => string.Join('.', NameParts); - public ModuleInfo(IPythonModule module) : - this(module, new List(), moduleImported: false) { + public ModuleInfo(IDocumentAnalysis document) : + this(document, module: null, new List(), moduleImported: false) { } - private ModuleInfo(IPythonModule module, List nameParts, bool moduleImported) : + private ModuleInfo(IDocumentAnalysis document, IPythonModule module, List nameParts, bool moduleImported) : this() { + CurrentFileAnalysis = document; Module = module; NameParts = nameParts; ModuleImported = moduleImported; @@ -574,7 +593,7 @@ private ModuleInfo(IPythonModule module, List nameParts, bool moduleImpo public void PopName() => NameParts.RemoveAt(NameParts.Count - 1); public ModuleInfo With(IPythonModule module) { - return new ModuleInfo(module, NameParts, moduleImported: true); + return new ModuleInfo(CurrentFileAnalysis, module, NameParts, moduleImported: true); } public ModuleInfo Reset(IPythonModule module) { @@ -583,7 +602,7 @@ public ModuleInfo Reset(IPythonModule module) { NameParts.Clear(); NameParts.Add(module.Name); - return new ModuleInfo(module, NameParts, moduleImported: false); + return new ModuleInfo(CurrentFileAnalysis, module, NameParts, moduleImported: false); } } From 6535f2cd53b843bca81fb04af09301d0699202f3 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 18 Oct 2019 17:16:08 -0700 Subject: [PATCH 32/43] added support for quick fix in hover --- .../MissingImportCodeActionProvider.cs | 7 +- .../Impl/Implementation/Server.cs | 2 +- src/LanguageServer/Impl/Protocol/Classes.cs | 50 ++++++++++++- .../Impl/Protocol/Diagnostic.cs | 72 +++++++++++++++++++ src/LanguageServer/Impl/Protocol/Messages.cs | 10 +++ 5 files changed, 136 insertions(+), 5 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 208f88439..9679257f7 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -34,6 +34,7 @@ using Microsoft.Python.Core; using Microsoft.Python.Core.Collections; using Microsoft.Python.Core.Text; +using Microsoft.Python.LanguageServer.Diagnostics; using Microsoft.Python.LanguageServer.Indexing; using Microsoft.Python.LanguageServer.Protocol; using Microsoft.Python.Parsing.Ast; @@ -107,10 +108,11 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis FilterCandidatesBasedOnContext(analysis, node, importFullNameMap, cancellationToken); // this will create actual code fix with certain orders + var diagnostics = new[] { diagnostic.ToDiagnostic() }; var codeActions = new List(); foreach (var fullName in OrderFullNames(importFullNameMap)) { cancellationToken.ThrowIfCancellationRequested(); - codeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullName, locallyInserted: false, cancellationToken)); + codeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullName, diagnostics, locallyInserted: false, cancellationToken)); } return codeActions; @@ -251,6 +253,7 @@ private static async Task EnsureCandidateModulesLoadedAsync(IDocumentAnalysis an private CodeAction CreateCodeAction(IDocumentAnalysis analysis, Node node, string moduleFullName, + Diagnostic[] diagnostics, bool locallyInserted, CancellationToken cancellationToken) { var insertionPoint = GetInsertionInfo(analysis, node, moduleFullName, locallyInserted, cancellationToken); @@ -274,7 +277,7 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, } var changes = new Dictionary { { analysis.Document.Uri, textEdits.ToArray() } }; - return new CodeAction() { title = titleText, edit = new WorkspaceEdit() { changes = changes } }; + return new CodeAction() { title = titleText, kind = CodeActionKind.QuickFix, diagnostics = diagnostics, edit = new WorkspaceEdit() { changes = changes } }; } private InsertionInfo? GetInsertionInfo(IDocumentAnalysis analysis, diff --git a/src/LanguageServer/Impl/Implementation/Server.cs b/src/LanguageServer/Impl/Implementation/Server.cs index 74cf3320b..824d8fd7a 100644 --- a/src/LanguageServer/Impl/Implementation/Server.cs +++ b/src/LanguageServer/Impl/Implementation/Server.cs @@ -98,7 +98,7 @@ public Server(IServiceManager services) { firstTriggerCharacter = "\n", moreTriggerCharacter = new[] { ";", ":" } }, - codeActionProvider = true, + codeActionProvider = new CodeActionOptions() { codeActionKinds = new string[] { CodeActionKind.QuickFix } }, } }; diff --git a/src/LanguageServer/Impl/Protocol/Classes.cs b/src/LanguageServer/Impl/Protocol/Classes.cs index ab5757f26..88842f6e6 100644 --- a/src/LanguageServer/Impl/Protocol/Classes.cs +++ b/src/LanguageServer/Impl/Protocol/Classes.cs @@ -363,7 +363,32 @@ public sealed class DefinitionCapabilities { public bool dynamicRegistration; } public DefinitionCapabilities definition; [Serializable] - public sealed class CodeActionCapabilities { public bool dynamicRegistration; } + public sealed class CodeActionCapabilities { + public bool dynamicRegistration; + // + // The client support code action literals as a valid + // response of the `textDocument/codeAction` request. + // + // Since 3.8.0 + // + public class CodeActionLiteralSupport { + // + // The code action kind is support with the following value + // set. + // + public class CodeActionKind { + // + // The code action kind values the client supports. When this + // property exists the client also guarantees that it will + // handle values outside its set gracefully and falls back + // to a default value when unknown. + // + public string[] valueSet; + } + public CodeActionKind codeActionKind; + } + public CodeActionLiteralSupport codeActionLiteralSupport; + } public CodeActionCapabilities codeAction; [Serializable] @@ -412,6 +437,17 @@ public sealed class CodeLensOptions { public bool resolveProvider; } + [Serializable] + public sealed class CodeActionOptions { + // + // CodeActionKinds that this server may return. + // + // The list of kinds may be generic, such as `CodeActionKind.Refactor`, or the server + // may list out every specific kind they provide. + // + public string[] codeActionKinds; + } + [Serializable] public sealed class DocumentOnTypeFormattingOptions { public string firstTriggerCharacter; @@ -456,7 +492,7 @@ public sealed class ServerCapabilities { public bool documentHighlightProvider; public bool documentSymbolProvider; public bool workspaceSymbolProvider; - public bool codeActionProvider; + public CodeActionOptions codeActionProvider; public CodeLensOptions codeLensProvider; public bool documentFormattingProvider; public bool documentRangeFormattingProvider; @@ -753,6 +789,16 @@ public sealed class CodeAction { // Summary: // Gets or sets the human readable title for this code action. public string title; + // + // The kind of the code action. + // + // Used to filter code actions. + // + public string kind; + // + // The diagnostics that this code action resolves. + // + public Diagnostic[] diagnostics; // // Summary: // Gets or sets the workspace edit that this code action performs. diff --git a/src/LanguageServer/Impl/Protocol/Diagnostic.cs b/src/LanguageServer/Impl/Protocol/Diagnostic.cs index 68dcac78b..9b1913792 100644 --- a/src/LanguageServer/Impl/Protocol/Diagnostic.cs +++ b/src/LanguageServer/Impl/Protocol/Diagnostic.cs @@ -51,4 +51,76 @@ public enum DiagnosticSeverity : int { Information = 3, Hint = 4 } + + // + // The kind of a code action. + // + // Kinds are a hierarchical list of identifiers separated by `.`, e.g. `"refactor.extract.function"`. + // + // The set of kinds is open and client needs to announce the kinds it supports to the server during + // initialization. + // + // + // A set of predefined code action kinds + // + public static class CodeActionKind { + // + // Empty kind. + // + public const string Empty = ""; + // + // Base kind for quickfix actions: 'quickfix' + // + public const string QuickFix = "quickfix"; + // + // Base kind for refactoring actions: 'refactor' + // + public const string Refactor = "refactor"; + // + // Base kind for refactoring extraction actions: 'refactor.extract' + // + // Example extract actions: + // + // - Extract method + // - Extract function + // - Extract variable + // - Extract interface from class + // - ... + // + public const string RefactorExtract = "refactor.extract"; + // + // Base kind for refactoring inline actions: 'refactor.inline' + // + // Example inline actions: + // + // - Inline function + // - Inline variable + // - Inline constant + // - ... + // + public const string RefactorInline = "refactor.inline"; + // + // Base kind for refactoring rewrite actions: 'refactor.rewrite' + // + // Example rewrite actions: + // + // - Convert JavaScript function to class + // - Add or remove parameter + // - Encapsulate field + // - Make method static + // - Move method to base class + // - ... + // + public const string RefactorRewrite = "refactor.rewrite"; + // + // Base kind for source actions: `source` + // + // Source code actions apply to the entire file. + // + public const string Source = "source"; + // + // Base kind for an organize imports source action: `source.organizeImports` + // + public const string SourceOrganizeImports = "source.organizeImports"; + } } diff --git a/src/LanguageServer/Impl/Protocol/Messages.cs b/src/LanguageServer/Impl/Protocol/Messages.cs index d8270543f..35c0f6f2f 100644 --- a/src/LanguageServer/Impl/Protocol/Messages.cs +++ b/src/LanguageServer/Impl/Protocol/Messages.cs @@ -189,7 +189,17 @@ public struct CodeActionParams { [Serializable] public sealed class CodeActionContext { + // + // An array of diagnostics. + // public Diagnostic[] diagnostics; + // + // Requested kind of actions to return. + // + // Actions not of this kind are filtered out by the client before being shown. So servers + // can omit computing them. + // + public string[] only; } [Serializable] From 26c1dbac3a565218a8ef6f5bc9af2d754e48f64e Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Mon, 21 Oct 2019 11:40:41 -0700 Subject: [PATCH 33/43] added one more test and fixed one bug --- .../MissingImportCodeActionProvider.cs | 17 +++++++++++------ .../Test/MissingImportCodeActionTests.cs | 10 ++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 9679257f7..ff2be6f20 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -444,11 +444,21 @@ private void CollectCandidates(ModuleInfo moduleInfo, continue; } + var fullName = $"{moduleInfo.FullName}.{memberName}"; + if (string.Equals(memberName, name)) { + // nested module are all imported + AddNameParts(fullName, moduleImported: true, memberImported: true, pythonModule, importFullNameMap); + } + // make sure we dig down modules only if we can use it from imports + // for example, user can do "from numpy import char" to import char [defchararray] module + // but user can not do "from numpy.char import x" since it is not one of known modules to us. + // in contrast, users can do "from os import path" to import path [ntpath] module + // but also can do "from os.path import x" since "os.path" is one of known moudles to us. var result = AstUtilities.FindImports( moduleInfo.CurrentFileAnalysis.Document.Interpreter.ModuleResolution.CurrentPathResolver, moduleInfo.CurrentFileAnalysis.Document.FilePath, - GetRootNames($"{moduleInfo.FullName}.{memberName}"), + GetRootNames(fullName), dotCount: 0, forceAbsolute: true); @@ -457,11 +467,6 @@ private void CollectCandidates(ModuleInfo moduleInfo, } moduleInfo.AddName(memberName); - if (string.Equals(memberName, name)) { - // nested module are all imported - AddNameParts(moduleInfo.FullName, moduleImported: true, memberImported: true, pythonModule, importFullNameMap); - } - CollectCandidates(moduleInfo.With(pythonModule), name, importFullNameMap, cancellationToken); moduleInfo.PopName(); } diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index b126f3adb..a7a42356b 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -329,6 +329,16 @@ public async Task ContextBasedSuggestion() { TestCodeAction(analysis.Document.Uri, codeAction, title, insertionSpan, newText); } + [TestMethod, Priority(0)] + public async Task ValidToBeUsedInImport() { + await TestCodeActionAsync( + @"from os import path +{|insertionSpan:|} +{|diagnostic:join|}", + title: "from os.path import join", + newText: "from os.path import join" + Environment.NewLine); + } + private async Task TestCodeActionAsync(string markup, string title, string newText, bool enableIndexManager = false) { var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics, enableIndexManager); From 3e7a0148a876cb9b42aac17b3f1410e92e20835b Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Tue, 22 Oct 2019 16:02:47 -0700 Subject: [PATCH 34/43] addressed easy feedback first --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 4 ++-- src/LanguageServer/Impl/Implementation/Server.cs | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index ff2be6f20..5bf9e658a 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -63,14 +63,14 @@ private MissingImportCodeActionProvider() { public async Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); var node = finder.GetExpression(diagnostic.SourceSpan); - if (!(node is NameExpression)) { + if (!(node is NameExpression nex)) { return Enumerable.Empty(); } var interpreter = analysis.Document.Interpreter; var pathResolver = interpreter.ModuleResolution.CurrentPathResolver; - var name = node.ToCodeString(analysis.Ast); + var name = nex.Name; if (string.IsNullOrEmpty(name)) { return Enumerable.Empty(); } diff --git a/src/LanguageServer/Impl/Implementation/Server.cs b/src/LanguageServer/Impl/Implementation/Server.cs index 824d8fd7a..6e155e29f 100644 --- a/src/LanguageServer/Impl/Implementation/Server.cs +++ b/src/LanguageServer/Impl/Implementation/Server.cs @@ -191,12 +191,11 @@ public Task Shutdown() { public void DidChangeConfiguration(DidChangeConfigurationParams @params, CancellationToken cancellationToken) { _disposableBag.ThrowIfDisposed(); switch (@params.settings) { - case ServerSettings settings: { + case ServerSettings settings: Settings = settings; _symbolHierarchyMaxSymbols = Settings.analysis.symbolsHierarchyMaxSymbols; _completionSource.Options = Settings.completion; break; - } default: _log?.Log(TraceEventType.Error, "change configuration notification sent unsupported settings"); break; From 883dfd4a86b5f9cce12aef35c6c0dddd7f299ceb Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 23 Oct 2019 17:31:49 -0700 Subject: [PATCH 35/43] made add import not to load modules implicitly and only use modules already loaded by users code. for closed files that are never loaded in the editor before, it will use SymbolIndexer to find candidates. but since the indexer is syntax based, it wont be able to provide information that require semantic info such as imported members ex) from os import * or __all__.extend(core.__all__) which means we will not propose from os.path import join if os module is not already loaded, instead we will propose ntpath or mac/posixpath since that is where join is actually defined. unfortunately we can't load modules just for add import, it is too expensive. we might in future add another index that is based on analyzed data which could have correct info saved. but for now, this is what we decided. --- .../MissingImportCodeActionProvider.cs | 111 ++++++++++++------ .../Impl/Indexing/AllVariableCollector.cs | 107 +++++++++++++++++ .../Indexing/MostRecentDocumentSymbols.cs | 6 +- .../Impl/Indexing/SymbolIndex.cs | 2 +- .../Impl/Indexing/SymbolIndexWalker.cs | 104 ++++++++++------ src/LanguageServer/Impl/Indexing/Symbols.cs | 43 ++++--- .../Test/MissingImportCodeActionTests.cs | 6 +- 7 files changed, 278 insertions(+), 101 deletions(-) create mode 100644 src/LanguageServer/Impl/Indexing/AllVariableCollector.cs diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 5bf9e658a..f259b4ab3 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -75,22 +75,15 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis return Enumerable.Empty(); } - await EnsureCandidateModulesLoadedAsync(analysis, name, cancellationToken); - var importFullNameMap = new Dictionary(); + await AddCandidatesFromIndexAsync(analysis, name, importFullNameMap, cancellationToken); // find installed modules matching the given name. this will include submodules var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); foreach (var moduleFullName in pathResolver.GetAllImportableModulesByName(name, includeImplicit)) { cancellationToken.ThrowIfCancellationRequested(); - - var module = interpreter.ModuleResolution.GetOrLoadModule(moduleFullName); - if (module == null) { - continue; - } - - importFullNameMap[moduleFullName] = new ImportInfo(moduleImported: false, memberImported: false, module); + importFullNameMap[moduleFullName] = new ImportInfo(moduleImported: false, memberImported: false, isModule: true); } // find members matching the given name from modules already loaded. @@ -135,7 +128,7 @@ private void FilterCandidatesBasedOnContext(IDocumentAnalysis analysis, Node nod // do simple filtering // remove all modules from candidates foreach (var kv in importFullNameMap.ToList()) { - if (kv.Value.Symbol.MemberType == PythonMemberType.Module) { + if (kv.Value.IsModule) { importFullNameMap.Remove(kv.Key); } } @@ -144,6 +137,12 @@ private void FilterCandidatesBasedOnContext(IDocumentAnalysis analysis, Node nod private IEnumerable OrderFullNames(Dictionary importFullNameMap) { // use some heuristic to improve code fix ordering + // put simple name module at the top + foreach (var fullName in OrderImportNames(importFullNameMap.Where(FilterSimpleName).Select(kv => kv.Key))) { + importFullNameMap.Remove(fullName); + yield return fullName; + } + // heuristic is we put entries with decl without any exports at the top // such as array. another example will be chararray. // this will make numpy chararray at the top and numpy defchararray at the bottom. @@ -152,7 +151,8 @@ private IEnumerable OrderFullNames(Dictionary import // so we could leave only numpy.chararray and remove ones like numpy.core.chararray and etc. but for now, // we show all those but in certain order so that numpy.chararray shows up top // this heuristic still has issue with something like os.path.join since no one import macpath, macpath join shows up high - var sourceDeclarationFullNames = importFullNameMap.GroupBy(kv => kv.Value.Symbol.Definition, LocationInfo.FullComparer) + var sourceDeclarationFullNames = importFullNameMap.Where(kv => kv.Value.Symbol != null) + .GroupBy(kv => kv.Value.Symbol.Definition, LocationInfo.FullComparer) .Where(FilterSourceDeclarations) .Select(g => g.First().Key); @@ -188,9 +188,10 @@ List OrderImportNames(IEnumerable fullNames) { return fullNames.OrderBy(n => n, ImportNameComparer.Instance).ToList(); } - bool FilterImportedMembers(KeyValuePair kv) => kv.Value.Symbol.MemberType != PythonMemberType.Module && kv.Value.MemberImported; - bool FilterImportedModuleMembers(KeyValuePair kv) => kv.Value.Symbol.MemberType != PythonMemberType.Module && kv.Value.ModuleImported; - bool FilterImportedModules(KeyValuePair kv) => kv.Value.Symbol.MemberType == PythonMemberType.Module && kv.Value.MemberImported; + bool FilterSimpleName(KeyValuePair kv) => kv.Key.IndexOf(".") < 0; + bool FilterImportedMembers(KeyValuePair kv) => !kv.Value.IsModule && kv.Value.MemberImported; + bool FilterImportedModuleMembers(KeyValuePair kv) => !kv.Value.IsModule && kv.Value.ModuleImported; + bool FilterImportedModules(KeyValuePair kv) => kv.Value.IsModule && kv.Value.MemberImported; bool FilterSourceDeclarations(IGrouping> group) { var count = 0; @@ -209,9 +210,10 @@ bool FilterSourceDeclarations(IGrouping importFullNameMap, + CancellationToken cancellationToken) { var indexManager = analysis.ExpressionEvaluator.Services.GetService(); if (indexManager == null) { // indexing is not supported @@ -221,33 +223,59 @@ private static async Task EnsureCandidateModulesLoadedAsync(IDocumentAnalysis an var symbolsIncludingName = await indexManager.WorkspaceSymbolsAsync(name, maxLength: int.MaxValue, includeLibraries: true, cancellationToken); // we only consider exact matches rather than partial matches - var symbolsWithName = symbolsIncludingName.Where(s => s.Name == name && s.Kind != Indexing.SymbolKind.Variable); + var symbolsWithName = symbolsIncludingName.Where(Include); var analyzer = analysis.ExpressionEvaluator.Services.GetService(); var pathResolver = analysis.Document.Interpreter.ModuleResolution.CurrentPathResolver; var modules = ImmutableArray.Empty; - foreach (var moduleName in symbolsWithName.Select(s => s.DocumentPath).Distinct().Select(p => pathResolver.GetModuleNameByPath(p))) { + foreach (var symbolAndModuleName in symbolsWithName.Select(s => (symbol: s, moduleName: pathResolver.GetModuleNameByPath(s.DocumentPath)))) { cancellationToken.ThrowIfCancellationRequested(); - var module = analysis.Document.Interpreter.ModuleResolution.GetOrLoadModule(moduleName); - if (module.Analysis is EmptyAnalysis) { - // once module is analyzed, this analysis result will be kept in memory - // even if graph for current document (modified) changed. - // so this will let us to analyze these modules only once - modules = modules.Add(module); + var key = $"{symbolAndModuleName.moduleName}.{symbolAndModuleName.symbol.Name}"; + var symbol = symbolAndModuleName.symbol; + + importFullNameMap.TryGetValue(key, out var existing); + + // we don't actually know whether this is a module. all we know is it appeared at + // Import statement. but most likely module, so we mark it as module for now. + // later when we check loaded module, if this happen to be loaded, this will get + // updated with more accurate data. + // if there happen to be multiple symbols with same name, we refer to mark it as module + var isModule = symbol.Kind == Indexing.SymbolKind.Module || existing.IsModule; + + // any symbol marked "Module" by indexer is imported. + importFullNameMap[key] = new ImportInfo( + moduleImported: isModule, + memberImported: isModule, + isModule); + } + + bool Include(FlatSymbol symbol) { + // we only suggest symbols that exist in __all__ + // otherwise, we show gigantic list from index + return symbol._existInAllVariable && + symbol.ContainerName == null && + CheckKind(symbol.Kind) && + symbol.Name == name; + } + + bool CheckKind(Indexing.SymbolKind kind) { + switch (kind) { + case Indexing.SymbolKind.Module: + case Indexing.SymbolKind.Namespace: + case Indexing.SymbolKind.Package: + case Indexing.SymbolKind.Class: + case Indexing.SymbolKind.Enum: + case Indexing.SymbolKind.Interface: + case Indexing.SymbolKind.Function: + case Indexing.SymbolKind.Constant: + case Indexing.SymbolKind.Struct: + return true; + default: + return false; } } - - if (modules.Count > 0) { - // declares given modules to be dependent to current document. otherwise, our optimization will drop - // analyzing these modules. this dependency will go away when user change the document - analyzer.EnqueueDocumentForAnalysis(analysis.Document, modules); - - // this will make those modules to be loaded. we don't care about return analysis since we only - // care those modules being loaded and analyzed. but not new current document's analysis - await analysis.Document.GetAnalysisAsync(ModuleLoadTimeout, cancellationToken); - } } private CodeAction CreateCodeAction(IDocumentAnalysis analysis, @@ -614,7 +642,7 @@ public ModuleInfo Reset(IPythonModule module) { } } - [DebuggerDisplay("{Symbol.Name} {Symbol.MemberType} {ModuleImported} {MemberImported}")] + [DebuggerDisplay("{Symbol?.Name} Module:{IsModule} ({ModuleImported} {MemberImported})")] private struct ImportInfo { // only one that shows up in "__all__" will be imported // containing module is imported @@ -622,12 +650,19 @@ private struct ImportInfo { // containing symbol is imported public readonly bool MemberImported; + public readonly bool IsModule; public readonly IPythonType Symbol; - public ImportInfo(bool moduleImported, bool memberImported, IPythonType symbol) { + public ImportInfo(bool moduleImported, bool memberImported, IPythonType symbol) : + this(moduleImported, memberImported, symbol.MemberType == PythonMemberType.Module) { + Symbol = symbol; + } + + public ImportInfo(bool moduleImported, bool memberImported, bool isModule) { ModuleImported = moduleImported; MemberImported = memberImported; - Symbol = symbol; + IsModule = isModule; + Symbol = null; } } } diff --git a/src/LanguageServer/Impl/Indexing/AllVariableCollector.cs b/src/LanguageServer/Impl/Indexing/AllVariableCollector.cs new file mode 100644 index 000000000..0e1137816 --- /dev/null +++ b/src/LanguageServer/Impl/Indexing/AllVariableCollector.cs @@ -0,0 +1,107 @@ +// 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, +// MERCHANTABILITY OR NON-INFRINGEMENT. +// +// See the Apache Version 2.0 License for specific language governing +// permissions and limitations under the License. + +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using Microsoft.Python.Parsing.Ast; + +namespace Microsoft.Python.LanguageServer.Indexing { + /// + /// This is a poor man's __all__ values collector. it uses only syntactic information to gather values. + /// + /// unlike the real one that actually binds expressions and + /// uses semantic data to build up __all__ information, this one's purpose is gathering cheap and fast but might be incorrect data + /// until more expensive analysis is done. + /// + internal class AllVariableCollector : PythonWalker { + private const string AllVariableName = "__all__"; + private readonly CancellationToken _cancellationToken; + + /// + /// names assigned to __all__ + /// + public readonly HashSet Names; + + public AllVariableCollector(CancellationToken cancellationToken) { + _cancellationToken = cancellationToken; + Names = new HashSet(); + } + + public override bool Walk(AssignmentStatement node) { + _cancellationToken.ThrowIfCancellationRequested(); + + // make sure we are dealing with __all__ assignment + if (node.Left.OfType().Any(n => n.Name == AllVariableName)) { + AddNames(node.Right as ListExpression); + } + + return base.Walk(node); + } + + public override bool Walk(AugmentedAssignStatement node) { + _cancellationToken.ThrowIfCancellationRequested(); + + if (node.Operator == Parsing.PythonOperator.Add && + node.Left is NameExpression nex && + nex.Name == AllVariableName) { + AddNames(node.Right as ListExpression); + } + + return base.Walk(node); + } + + public override bool Walk(CallExpression node) { + _cancellationToken.ThrowIfCancellationRequested(); + + if (node.Args.Count > 0 && + node.Target is MemberExpression me && + me.Target is NameExpression nex && + nex.Name == AllVariableName) { + var arg = node.Args[0].Expression; + + switch (me.Name) { + case "append": + AddName(arg); + break; + case "extend": + AddNames(arg as ListExpression); + break; + } + } + + return base.Walk(node); + } + + private void AddName(Expression item) { + if (item is ConstantExpression con && + con.Value is string name && + !string.IsNullOrEmpty(name)) { + Names.Add(name); + } + } + + private void AddNames(ListExpression list) { + // only support the form of __all__ = [ ... ] + if (list == null) { + return; + } + + foreach (var item in list.Items) { + AddName(item); + } + } + } +} diff --git a/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs b/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs index d2da1a0f2..a8023c6b2 100644 --- a/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs +++ b/src/LanguageServer/Impl/Indexing/MostRecentDocumentSymbols.cs @@ -97,7 +97,7 @@ public void MarkAsPending() { public void Dispose() { lock (_lock) { _tcs.TrySetCanceled(); - + try { _workCts?.Dispose(); } catch (ObjectDisposedException) { @@ -136,7 +136,7 @@ private async Task> IndexAsync(IDocument doc, } cancellationToken.ThrowIfCancellationRequested(); - var walker = new SymbolIndexWalker(ast, _library); + var walker = new SymbolIndexWalker(ast, _library, cancellationToken); ast.Walk(walker); return walker.Symbols; } @@ -145,7 +145,7 @@ private async Task> ParseAsync(CancellationTok try { var ast = await _indexParser.ParseAsync(_path, cancellationToken); cancellationToken.ThrowIfCancellationRequested(); - var walker = new SymbolIndexWalker(ast); + var walker = new SymbolIndexWalker(ast, _library, cancellationToken); ast.Walk(walker); return walker.Symbols; } catch (Exception e) when (e is IOException || e is UnauthorizedAccessException) { diff --git a/src/LanguageServer/Impl/Indexing/SymbolIndex.cs b/src/LanguageServer/Impl/Indexing/SymbolIndex.cs index 831526735..7b97905b7 100644 --- a/src/LanguageServer/Impl/Indexing/SymbolIndex.cs +++ b/src/LanguageServer/Impl/Indexing/SymbolIndex.cs @@ -99,7 +99,7 @@ private IEnumerable WorkspaceSymbolsQuery(string path, string query, return DecorateWithParentsName((sym.Children ?? Enumerable.Empty()).ToList(), sym.Name); }); return treeSymbols.Where(sym => sym.symbol.Name.ContainsOrdinal(query, ignoreCase: true)) - .Select(sym => new FlatSymbol(sym.symbol.Name, sym.symbol.Kind, path, sym.symbol.SelectionRange, sym.parentName)); + .Select(sym => new FlatSymbol(sym.symbol.Name, sym.symbol.Kind, path, sym.symbol.SelectionRange, sym.parentName, sym.symbol._existInAllVariable)); } private static IEnumerable<(HierarchicalSymbol symbol, string parentName)> DecorateWithParentsName( diff --git a/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs b/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs index 0ce628a19..05d00859c 100644 --- a/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs +++ b/src/LanguageServer/Impl/Indexing/SymbolIndexWalker.cs @@ -16,6 +16,7 @@ using System.Collections.Generic; using System.Linq; using System.Text.RegularExpressions; +using System.Threading; using Microsoft.Python.Core; using Microsoft.Python.Parsing.Ast; @@ -27,17 +28,25 @@ internal class SymbolIndexWalker : PythonWalker { private readonly PythonAst _ast; private readonly bool _libraryMode; private readonly SymbolStack _stack = new SymbolStack(); + private readonly HashSet _namesInAllVariable; - public SymbolIndexWalker(PythonAst ast, bool libraryMode = false) { + public SymbolIndexWalker(PythonAst ast, bool libraryMode = false, CancellationToken cancellationToken = default) { _ast = ast; _libraryMode = libraryMode; + + var collector = new AllVariableCollector(cancellationToken); + _ast.Walk(collector); + + _namesInAllVariable = collector.Names; } public IReadOnlyList Symbols => _stack.Root; public override bool Walk(ClassDefinition node) { _stack.Enter(SymbolKind.Class); - node.Body?.Walk(this); + + WalkIfNotLibraryMode(node.Body); + var children = _stack.Exit(); _stack.AddSymbol(new HierarchicalSymbol( @@ -46,20 +55,13 @@ public override bool Walk(ClassDefinition node) { node.GetSpan(_ast), node.NameExpression.GetSpan(_ast), children, - FunctionKind.Class + FunctionKind.Class, + ExistInAllVariable(node.Name) )); return false; } - private void WalkIfNotLibraryMode(Node node) { - if (_libraryMode) { - return; - } - - node?.Walk(this); - } - public override bool Walk(FunctionDefinition node) { _stack.Enter(SymbolKind.Function); foreach (var p in node.Parameters) { @@ -72,52 +74,60 @@ public override bool Walk(FunctionDefinition node) { var children = _stack.Exit(); + SymbolKind symbolKind; + string functionKind; + GetKinds(node, out symbolKind, out functionKind); + var span = node.GetSpan(_ast); var ds = new HierarchicalSymbol( node.Name, - SymbolKind.Function, + symbolKind, span, node.IsLambda ? span : node.NameExpression.GetSpan(_ast), children, - FunctionKind.Function + functionKind, + ExistInAllVariable(node.Name) ); + _stack.AddSymbol(ds); + return false; + } + + private void GetKinds(FunctionDefinition node, out SymbolKind symbolKind, out string functionKind) { + symbolKind = SymbolKind.Function; + functionKind = FunctionKind.Function; + if (_stack.Parent == SymbolKind.Class) { - switch (ds.Name) { + switch (node.Name) { case "__init__": - ds.Kind = SymbolKind.Constructor; + symbolKind = SymbolKind.Constructor; break; case var name when DoubleUnderscore.IsMatch(name): - ds.Kind = SymbolKind.Operator; + symbolKind = SymbolKind.Operator; break; default: - ds.Kind = SymbolKind.Method; + symbolKind = SymbolKind.Method; if (node.Decorators != null) { foreach (var dec in node.Decorators.Decorators) { var maybeKind = DecoratorExpressionToKind(dec); if (maybeKind.HasValue) { - ds.Kind = maybeKind.Value.kind; - ds._functionKind = maybeKind.Value.functionKind; + symbolKind = maybeKind.Value.kind; + functionKind = maybeKind.Value.functionKind; break; } } } - break; } } - - _stack.AddSymbol(ds); - - return false; } public override bool Walk(ImportStatement node) { foreach (var (nameNode, nameString) in node.Names.Zip(node.AsNames, (name, asName) => asName != null ? (asName, asName.Name) : ((Node)name, name.MakeString()))) { var span = nameNode.GetSpan(_ast); - _stack.AddSymbol(new HierarchicalSymbol(nameString, SymbolKind.Module, span)); + _stack.AddSymbol(new HierarchicalSymbol(nameString, SymbolKind.Module, span, existInAllVariable: ExistInAllVariable(nameString))); } return false; @@ -130,7 +140,7 @@ public override bool Walk(FromImportStatement node) { foreach (var name in node.Names.Zip(node.AsNames, (name, asName) => asName ?? name)) { var span = name.GetSpan(_ast); - _stack.AddSymbol(new HierarchicalSymbol(name.Name, SymbolKind.Module, span)); + _stack.AddSymbol(new HierarchicalSymbol(name.Name, SymbolKind.Module, span, existInAllVariable: ExistInAllVariable(name.Name))); } return false; @@ -161,10 +171,6 @@ public override bool Walk(AugmentedAssignStatement node) { } public override bool Walk(IfStatement node) { - if (_libraryMode) { - return false; - } - WalkAndDeclareAll(node.Tests); WalkAndDeclare(node.ElseStatement); @@ -172,10 +178,6 @@ public override bool Walk(IfStatement node) { } public override bool Walk(TryStatement node) { - if (_libraryMode) { - return false; - } - WalkAndDeclare(node.Body); WalkAndDeclareAll(node.Handlers); WalkAndDeclare(node.Else); @@ -185,10 +187,6 @@ public override bool Walk(TryStatement node) { } public override bool Walk(ForStatement node) { - if (_libraryMode) { - return false; - } - _stack.EnterDeclared(); AddVarSymbolRecursive(node.Left); node.List?.Walk(this); @@ -223,6 +221,10 @@ public override bool Walk(ListComprehension node) { public override void PostWalk(ListComprehension node) => ExitComprehension(node); public override bool Walk(DictionaryComprehension node) { + if (_libraryMode) { + return false; + } + _stack.Enter(SymbolKind.None); return base.Walk(node); } @@ -230,6 +232,10 @@ public override bool Walk(DictionaryComprehension node) { public override void PostWalk(DictionaryComprehension node) => ExitComprehension(node); public override bool Walk(SetComprehension node) { + if (_libraryMode) { + return false; + } + _stack.Enter(SymbolKind.None); return base.Walk(node); } @@ -237,6 +243,10 @@ public override bool Walk(SetComprehension node) { public override void PostWalk(SetComprehension node) => ExitComprehension(node); public override bool Walk(GeneratorExpression node) { + if (_libraryMode) { + return false; + } + _stack.Enter(SymbolKind.None); return base.Walk(node); } @@ -244,6 +254,10 @@ public override bool Walk(GeneratorExpression node) { public override void PostWalk(GeneratorExpression node) => ExitComprehension(node); private void ExitComprehension(Comprehension node) { + if (_libraryMode) { + return; + } + var children = _stack.Exit(); var span = node.GetSpan(_ast); @@ -274,7 +288,7 @@ private void AddVarSymbol(NameExpression node) { var span = node.GetSpan(_ast); - _stack.AddSymbol(new HierarchicalSymbol(node.Name, kind, span)); + _stack.AddSymbol(new HierarchicalSymbol(node.Name, kind, span, existInAllVariable: ExistInAllVariable(node.Name))); } private void AddVarSymbolRecursive(Expression node) { @@ -318,6 +332,14 @@ private void AddVarSymbolRecursive(Expression node) { return null; } + private void WalkIfNotLibraryMode(Node node) { + if (_libraryMode) { + return; + } + + node?.Walk(this); + } + private bool NameIsProperty(string name) => name == "property" || name == "abstractproperty" @@ -348,6 +370,10 @@ private void WalkAndDeclareAll(IEnumerable nodes) { } } + private bool ExistInAllVariable(string name) { + return _namesInAllVariable.Contains(name); + } + private class SymbolStack { private readonly Stack<(SymbolKind? kind, List symbols)> _symbols; private readonly Stack> _declared = new Stack>(new[] { new HashSet() }); diff --git a/src/LanguageServer/Impl/Indexing/Symbols.cs b/src/LanguageServer/Impl/Indexing/Symbols.cs index 6fbeb08e4..eecb55725 100644 --- a/src/LanguageServer/Impl/Indexing/Symbols.cs +++ b/src/LanguageServer/Impl/Indexing/Symbols.cs @@ -60,16 +60,17 @@ internal class FunctionKind { // Analagous to LSP's DocumentSymbol. [DebuggerDisplay("{Name}, {Kind}")] - internal class HierarchicalSymbol { - public string Name; - public string Detail; - public SymbolKind Kind; - public bool? Deprecated; - public SourceSpan Range; - public SourceSpan SelectionRange; - public IList Children; + internal sealed class HierarchicalSymbol { + public readonly string Name; + public readonly string Detail; + public readonly SymbolKind Kind; + public readonly bool? Deprecated; + public readonly SourceSpan Range; + public readonly SourceSpan SelectionRange; + public readonly IList Children; - public string _functionKind; + public readonly string _functionKind; + public readonly bool _existInAllVariable; public HierarchicalSymbol( string name, @@ -77,7 +78,8 @@ public HierarchicalSymbol( SourceSpan range, SourceSpan? selectionRange = null, IList children = null, - string functionKind = FunctionKind.None + string functionKind = FunctionKind.None, + bool existInAllVariable = false ) { Name = name; Kind = kind; @@ -85,31 +87,36 @@ public HierarchicalSymbol( SelectionRange = selectionRange ?? range; Children = children; _functionKind = functionKind; + _existInAllVariable = existInAllVariable; } } // Analagous to LSP's SymbolInformation. [DebuggerDisplay("{ContainerName}:{Name}, {Kind}")] - internal class FlatSymbol { - public string Name; - public SymbolKind Kind; - public bool? Deprecated; - public string DocumentPath; - public SourceSpan Range; - public string ContainerName; + internal sealed class FlatSymbol { + public readonly string Name; + public readonly SymbolKind Kind; + public readonly bool? Deprecated; + public readonly string DocumentPath; + public readonly SourceSpan Range; + public readonly string ContainerName; + + public readonly bool _existInAllVariable; public FlatSymbol( string name, SymbolKind kind, string documentPath, SourceSpan range, - string containerName = null + string containerName = null, + bool existInAllVariable = false ) { Name = name; Kind = kind; DocumentPath = documentPath; Range = range; ContainerName = containerName; + _existInAllVariable = existInAllVariable; } } } diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index a7a42356b..d75d1230e 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -258,14 +258,16 @@ public async Task SymbolOrdering() { [TestMethod, Priority(0)] public async Task SymbolOrdering2() { - var markup = @"{|insertionSpan:|}{|diagnostic:join|}"; + var markup = @"from os import path +{|insertionSpan:|} +{|diagnostic:join|}"; var (analysis, codeActions, insertionSpan) = await GetAnalysisAndCodeActionsAndSpanAsync(markup, MissingImportCodeActionProvider.Instance.FixableDiagnostics, enableIndexManager: true); var list = codeActions.Select(c => c.title).ToList(); var zipList = Enumerable.Range(0, list.Count).Zip(list); - var sourceDeclIndex = zipList.First(t => t.Second == "from macpath import join").First; + var sourceDeclIndex = zipList.First(t => t.Second == "from posixpath import join").First; var importedMemberIndex = zipList.First(t => t.Second == "from os.path import join").First; var restIndex = zipList.First(t => t.Second == "from ntpath import join").First; From f964c4aef5e71c9fed2a1d9feacbf5ff43031325 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 23 Oct 2019 18:32:56 -0700 Subject: [PATCH 36/43] added more abbreviation supports --- .../MissingImportCodeActionProvider.cs | 42 ++++++---- .../Test/MissingImportCodeActionTests.cs | 79 ++++++++++++++----- 2 files changed, 84 insertions(+), 37 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index f259b4ab3..876c8fc4a 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -51,7 +51,13 @@ internal sealed class MissingImportCodeActionProvider : ICodeActionProvider { // the module is not something user expected private static readonly Dictionary WellKnownAbbreviationMap = new Dictionary() { { "numpy", "np" }, - { "pandas", "pd" } + { "pandas", "pd" }, + { "tensorflow", "tf" }, + { "matplotlib.pyplot", "plt" }, + { "matplotlib", "mpl" }, + { "math", "m" }, + { "scipy.io", "spio" }, + { "scipy", "sp" }, }; private MissingImportCodeActionProvider() { @@ -322,14 +328,14 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, var importNodes = body.GetChildNodes().Where(c => c is ImportStatement || c is FromImportStatement).ToList(); var lastImportNode = importNodes.LastOrDefault(); + var abbreviation = GetAbbreviationForWellKnownModules(analysis, fullyQualifiedName); + // first check whether module name is dotted or not var dotIndex = fullyQualifiedName.LastIndexOf('.'); if (dotIndex < 0) { // there can't be existing import since we have the error - WellKnownAbbreviationMap.TryGetValue(fullyQualifiedName, out var abbreviation); - return new InsertionInfo(addBlankLine: lastImportNode == null, - GetInsertionText(fullyQualifiedName, abbreviation), + GetInsertionText($"import {fullyQualifiedName}", abbreviation), GetRange(analysis.Ast, body, lastImportNode), indentation, abbreviation); @@ -341,31 +347,35 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, foreach (var current in importNodes.Reverse().OfType()) { if (current.Root.MakeString() == fromPart) { return new InsertionInfo(addBlankLine: false, - GetInsertionText(current, fromPart, nameToAdd), + GetInsertionText(current, fromPart, nameToAdd, abbreviation), current.GetSpan(analysis.Ast), - indentation); + indentation, + abbreviation); } } // add new from * import * statement return new InsertionInfo(addBlankLine: lastImportNode == null, - $"from {fromPart} import {nameToAdd}", + GetInsertionText($"from {fromPart} import {nameToAdd}", abbreviation), GetRange(analysis.Ast, body, lastImportNode), - indentation); + indentation, + abbreviation); } - private static string GetInsertionText(string module, string abbreviation) { - if (abbreviation == null) { - return $"import {module}"; - } - - return $"import {module} as {abbreviation}"; + private static string GetAbbreviationForWellKnownModules(IDocumentAnalysis analysis, string fullyQualifiedName) { + // bind analysis to see whether abbreviation already exist + WellKnownAbbreviationMap.TryGetValue(fullyQualifiedName, out var abbreviation); + return abbreviation; } - private string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd) { + private static string GetInsertionText(string insertionText, string abbreviation) => + abbreviation == null ? insertionText : $"{insertionText} as {abbreviation}"; + + private string GetInsertionText(FromImportStatement fromImportStatement, string rootModuleName, string moduleNameToAdd, string abbreviation) { var imports = fromImportStatement.Names.Select(n => n.Name) - .Concat(new string[] { moduleNameToAdd }) + .Concat(new string[] { GetInsertionText(moduleNameToAdd, abbreviation) }) .OrderBy(n => n).ToList(); + return $"from {rootModuleName} import {string.Join(", ", imports)}"; } diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index d75d1230e..bb220500e 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -286,30 +286,36 @@ await TestCodeActionAsync( [TestMethod, Priority(0)] public async Task SuggestAbbreviationForKnownModule() { - MarkupUtils.GetNamedSpans(@"{|insertionSpan:|}{|diagnostic:pandas|}", out var code, out var spans); - - // get main analysis and add mock pandas module - var analysis = await GetAnalysisAsync(code); - await GetAnalysisAsync("", analysis.ExpressionEvaluator.Services, modulePath: TestData.GetTestSpecificPath("pandas.py")); - - // calculate actions - var diagnostics = GetDiagnostics(analysis, spans["diagnostic"].First().ToSourceSpan(analysis.Ast), MissingImportCodeActionProvider.Instance.FixableDiagnostics); - var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); - - var docTable = analysis.ExpressionEvaluator.Services.GetService(); - - // verify results - var codeAction = codeActions.Single(c => c.title == "import pandas as pd"); - codeAction.edit.changes.Should().HaveCount(1); + await TestCodeActionAsync( + @"{|insertionSpan:|}{|diagnostic:pandas|}", + title: "import pandas as pd", + newText: "import pandas as pd" + Environment.NewLine + Environment.NewLine, + abbreviation: "pd", + relativePaths: "pandas"); + } - var edits = codeAction.edit.changes[analysis.Document.Uri]; - edits.Should().HaveCount(2); + [TestMethod, Priority(0)] + public async Task SuggestAbbreviationForKnownModule2() { + await TestCodeActionAsync( + @"{|insertionSpan:|}{|diagnostic:pyplot|}", + title: "from matplotlib import pyplot as plt", + newText: "from matplotlib import pyplot as plt" + Environment.NewLine + Environment.NewLine, + abbreviation: "plt", + relativePaths: @"matplotlib\pyplot"); + } - var invocationEdit = edits.Single(e => e.newText == "pd"); - invocationEdit.range.Should().Be(spans["diagnostic"].First().ToSourceSpan(analysis.Ast)); + [TestMethod, Priority(0)] + public async Task SuggestAbbreviationForKnownModule3() { + var markup = @" +{|insertionSpan:from matplotlib import test|} +{|diagnostic:pyplot|}"; - var insertEdit = edits.Single(e => e.newText == "import pandas as pd" + Environment.NewLine + Environment.NewLine); - insertEdit.range.Should().Be(spans["insertionSpan"].First().ToSourceSpan(analysis.Ast)); + await TestCodeActionAsync( + markup, + title: "from matplotlib import pyplot as plt, test", + newText: "from matplotlib import pyplot as plt, test", + abbreviation: "plt", + relativePaths: new string[] { @"matplotlib\pyplot", @"matplotlib\test" }); } [TestMethod, Priority(0)] @@ -394,5 +400,36 @@ private static Diagnostic[] GetDiagnostics(IDocumentAnalysis analysis, SourceSpa .Select(d => d.ToDiagnostic()) .ToArray(); } + + private async Task TestCodeActionAsync(string markup, string title, string newText, string abbreviation, params string[] relativePaths) { + MarkupUtils.GetNamedSpans(markup, out var code, out var spans); + + // get main analysis and add mock pandas module + var analysis = await GetAnalysisAsync(code); + + foreach (var relativePath in relativePaths) { + await GetAnalysisAsync("", analysis.ExpressionEvaluator.Services, modulePath: TestData.GetTestSpecificPath($"{relativePath}.py")); + } + + // calculate actions + var diagnosticSpan = spans["diagnostic"].First().ToSourceSpan(analysis.Ast); + var diagnostics = GetDiagnostics(analysis, diagnosticSpan, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); + + var docTable = analysis.ExpressionEvaluator.Services.GetService(); + + // verify results + var codeAction = codeActions.Single(c => c.title == title); + codeAction.edit.changes.Should().HaveCount(1); + + var edits = codeAction.edit.changes[analysis.Document.Uri]; + edits.Should().HaveCount(2); + + var invocationEdit = edits.Single(e => e.newText == abbreviation); + invocationEdit.range.Should().Be(diagnosticSpan); + + var insertEdit = edits.Single(e => e.newText == newText); + insertEdit.range.Should().Be(spans["insertionSpan"].First().ToSourceSpan(analysis.Ast)); + } } } From 52a2e5a2f5d363044e5b71ac8beb559859d4540a Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Wed, 23 Oct 2019 23:28:44 -0700 Subject: [PATCH 37/43] removed unnecessary time out --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 876c8fc4a..89defc16b 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -42,8 +42,6 @@ namespace Microsoft.Python.LanguageServer.CodeActions { internal sealed class MissingImportCodeActionProvider : ICodeActionProvider { - private const int ModuleLoadTimeout = 10 * 1000; // 10 seconds - public static readonly ICodeActionProvider Instance = new MissingImportCodeActionProvider(); // right now, it is a static. in future, we might consider giving an option to users to customize this list From c0abc41d507c0ea678a8f4a09d7f607c4aa9e1b5 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 24 Oct 2019 00:07:22 -0700 Subject: [PATCH 38/43] support reverse abbreviation case where user types "np" and we suggest numpy --- .../MissingImportCodeActionProvider.cs | 79 +++++++++++++++---- .../Impl/Definitions/ICodeActionProvider.cs | 2 +- .../Impl/Sources/CodeActionSource.cs | 2 +- .../Test/MissingImportCodeActionTests.cs | 34 ++++++++ 4 files changed, 100 insertions(+), 17 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 89defc16b..f5aad73ea 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -64,28 +64,59 @@ private MissingImportCodeActionProvider() { public ImmutableArray FixableDiagnostics => ImmutableArray.Create( ErrorCodes.UndefinedVariable, ErrorCodes.VariableNotDefinedGlobally, ErrorCodes.VariableNotDefinedNonLocal); - public async Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { + public async Task> GetCodeActionsAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); var node = finder.GetExpression(diagnostic.SourceSpan); if (!(node is NameExpression nex)) { return Enumerable.Empty(); } - var interpreter = analysis.Document.Interpreter; - var pathResolver = interpreter.ModuleResolution.CurrentPathResolver; - - var name = nex.Name; - if (string.IsNullOrEmpty(name)) { + var identifier = nex.Name; + if (string.IsNullOrEmpty(identifier)) { return Enumerable.Empty(); } + var codeActions = new List(); + var diagnostics = new[] { diagnostic.ToDiagnostic() }; + + // add given name as it is + await GetCodeActionsAsync(analysis, diagnostics, new Input(node, identifier), codeActions, cancellationToken); + + // see whether it is one of abbreviation we specialize + foreach (var moduleFullName in WellKnownAbbreviationMap.Where(kv => kv.Value == identifier).Select(kv => kv.Key)) { + var moduleName = GetModuleName(moduleFullName); + + await GetCodeActionsAsync(analysis, diagnostics, new Input(node, moduleName, moduleFullName), codeActions, cancellationToken); + } + + return codeActions; + + string GetModuleName(string moduleFullName) { + var index = moduleFullName.LastIndexOf("."); + if (index < 0) { + return moduleFullName; + } + + return moduleFullName.Substring(index + 1); + } + } + + private async Task GetCodeActionsAsync(IDocumentAnalysis analysis, + Diagnostic[] diagnostics, + Input input, + List codeActions, + CancellationToken cancellationToken) { var importFullNameMap = new Dictionary(); - await AddCandidatesFromIndexAsync(analysis, name, importFullNameMap, cancellationToken); + await AddCandidatesFromIndexAsync(analysis, input.Identifier, importFullNameMap, cancellationToken); + + var interpreter = analysis.Document.Interpreter; + var pathResolver = interpreter.ModuleResolution.CurrentPathResolver; // find installed modules matching the given name. this will include submodules var languageVersion = Parsing.PythonLanguageVersionExtensions.ToVersion(interpreter.LanguageVersion); var includeImplicit = !ModulePath.PythonVersionRequiresInitPyFiles(languageVersion); - foreach (var moduleFullName in pathResolver.GetAllImportableModulesByName(name, includeImplicit)) { + + foreach (var moduleFullName in pathResolver.GetAllImportableModulesByName(input.Identifier, includeImplicit)) { cancellationToken.ThrowIfCancellationRequested(); importFullNameMap[moduleFullName] = new ImportInfo(moduleImported: false, memberImported: false, isModule: true); } @@ -98,21 +129,27 @@ public async Task> GetCodeActionAsync(IDocumentAnalysis } // module name is full module name that you can use in import xxxx directly - CollectCandidates(moduleInfo.Reset(module), name, importFullNameMap, cancellationToken); + CollectCandidates(moduleInfo.Reset(module), input.Identifier, importFullNameMap, cancellationToken); Debug.Assert(moduleInfo.NameParts.Count == 1 && moduleInfo.NameParts[0] == module.Name); } - FilterCandidatesBasedOnContext(analysis, node, importFullNameMap, cancellationToken); + // check quick bail out case where we know what module we are looking for + if (input.ModuleFullNameOpt != null) { + if (importFullNameMap.ContainsKey(input.ModuleFullNameOpt)) { + // add code action if the module exist, otherwise, bail out empty + codeActions.AddIfNotNull(CreateCodeAction(analysis, input.Context, input.ModuleFullNameOpt, diagnostics, locallyInserted: false, cancellationToken)); + } + return; + } + + // regular case + FilterCandidatesBasedOnContext(analysis, input.Context, importFullNameMap, cancellationToken); // this will create actual code fix with certain orders - var diagnostics = new[] { diagnostic.ToDiagnostic() }; - var codeActions = new List(); foreach (var fullName in OrderFullNames(importFullNameMap)) { cancellationToken.ThrowIfCancellationRequested(); - codeActions.AddIfNotNull(CreateCodeAction(analysis, node, fullName, diagnostics, locallyInserted: false, cancellationToken)); + codeActions.AddIfNotNull(CreateCodeAction(analysis, input.Context, fullName, diagnostics, locallyInserted: false, cancellationToken)); } - - return codeActions; } private void FilterCandidatesBasedOnContext(IDocumentAnalysis analysis, Node node, Dictionary importFullNameMap, CancellationToken cancellationToken) { @@ -605,6 +642,18 @@ public InsertionInfo(bool addBlankLine, string insertionText, Range range, strin } } + private struct Input { + public readonly Node Context; + public readonly string Identifier; + public readonly string ModuleFullNameOpt; + + public Input(Node context, string identifier, string moduleFullNameOpt = null) { + Context = context; + Identifier = identifier; + ModuleFullNameOpt = moduleFullNameOpt; + } + } + private struct ModuleInfo { public readonly IDocumentAnalysis CurrentFileAnalysis; public readonly IPythonModule Module; diff --git a/src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs b/src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs index 95cb4ba0b..fc500bb54 100644 --- a/src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs +++ b/src/LanguageServer/Impl/Definitions/ICodeActionProvider.cs @@ -37,6 +37,6 @@ public interface ICodeActionProvider { /// that code action is supposed to fix /// /// that can fix the given - Task> GetCodeActionAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellation); + Task> GetCodeActionsAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellation); } } diff --git a/src/LanguageServer/Impl/Sources/CodeActionSource.cs b/src/LanguageServer/Impl/Sources/CodeActionSource.cs index 11f02f29a..21d787c6e 100644 --- a/src/LanguageServer/Impl/Sources/CodeActionSource.cs +++ b/src/LanguageServer/Impl/Sources/CodeActionSource.cs @@ -45,7 +45,7 @@ public async Task GetCodeActionsAsync(IDocumentAnalysis analysis, foreach (var diagnostic in GetMatchingDiagnostics(analysis, diagnostics, cancellationToken)) { foreach (var codeActionProvider in _codeActionProviders) { if (codeActionProvider.FixableDiagnostics.Any(code => code == diagnostic.ErrorCode)) { - results.AddRange(await codeActionProvider.GetCodeActionAsync(analysis, diagnostic, cancellationToken)); + results.AddRange(await codeActionProvider.GetCodeActionsAsync(analysis, diagnostic, cancellationToken)); } } } diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index bb220500e..b56d1602a 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -318,6 +318,40 @@ await TestCodeActionAsync( relativePaths: new string[] { @"matplotlib\pyplot", @"matplotlib\test" }); } + [TestMethod, Priority(0)] + public async Task SuggestReverseAbbreviationForKnownModule() { + await TestCodeActionAsync( + @"{|insertionSpan:|}{|diagnostic:pd|}", + title: "import pandas as pd", + newText: "import pandas as pd" + Environment.NewLine + Environment.NewLine, + abbreviation: "pd", + relativePaths: "pandas"); + } + + [TestMethod, Priority(0)] + public async Task SuggestReverseAbbreviationForKnownModule2() { + await TestCodeActionAsync( + @"{|insertionSpan:|}{|diagnostic:plt|}", + title: "from matplotlib import pyplot as plt", + newText: "from matplotlib import pyplot as plt" + Environment.NewLine + Environment.NewLine, + abbreviation: "plt", + relativePaths: @"matplotlib\pyplot"); + } + + [TestMethod, Priority(0)] + public async Task SuggestReverseAbbreviationForKnownModule3() { + var markup = @" +{|insertionSpan:from matplotlib import test|} +{|diagnostic:plt|}"; + + await TestCodeActionAsync( + markup, + title: "from matplotlib import pyplot as plt, test", + newText: "from matplotlib import pyplot as plt, test", + abbreviation: "plt", + relativePaths: new string[] { @"matplotlib\pyplot", @"matplotlib\test" }); + } + [TestMethod, Priority(0)] public async Task ContextBasedSuggestion() { var markup = From a5da0d3754f5d17578edcd7b82340488fb2a53d6 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 24 Oct 2019 02:08:00 -0700 Subject: [PATCH 39/43] made abbreviation to check existing symbol names to use unique name for abbreviation --- .../MissingImportCodeActionProvider.cs | 14 +- .../Impl/Sources/HoverSource.cs | 44 ++--- .../Impl/Utilities/UniqueNameGenerator.cs | 114 +++++++++++++ .../Test/MissingImportCodeActionTests.cs | 43 ++++- .../Test/UniqueNameGeneratorTests.cs | 151 ++++++++++++++++++ 5 files changed, 332 insertions(+), 34 deletions(-) create mode 100644 src/LanguageServer/Impl/Utilities/UniqueNameGenerator.cs create mode 100644 src/LanguageServer/Test/UniqueNameGeneratorTests.cs diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index f5aad73ea..815d01fa7 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -13,7 +13,6 @@ // See the Apache Version 2.0 License for specific language governing // permissions and limitations under the License. - using System; using System.Collections.Generic; using System.Data; @@ -37,6 +36,7 @@ using Microsoft.Python.LanguageServer.Diagnostics; using Microsoft.Python.LanguageServer.Indexing; using Microsoft.Python.LanguageServer.Protocol; +using Microsoft.Python.LanguageServer.Utilities; using Microsoft.Python.Parsing.Ast; using Range = Microsoft.Python.Core.Text.Range; @@ -65,7 +65,7 @@ private MissingImportCodeActionProvider() { ErrorCodes.UndefinedVariable, ErrorCodes.VariableNotDefinedGlobally, ErrorCodes.VariableNotDefinedNonLocal); public async Task> GetCodeActionsAsync(IDocumentAnalysis analysis, DiagnosticsEntry diagnostic, CancellationToken cancellationToken) { - var finder = new ExpressionFinder(analysis.Ast, FindExpressionOptions.Complete); + var finder = new ExpressionFinder(analysis.Ast, new FindExpressionOptions() { Names = true }); var node = finder.GetExpression(diagnostic.SourceSpan); if (!(node is NameExpression nex)) { return Enumerable.Empty(); @@ -398,9 +398,13 @@ private CodeAction CreateCodeAction(IDocumentAnalysis analysis, } private static string GetAbbreviationForWellKnownModules(IDocumentAnalysis analysis, string fullyQualifiedName) { - // bind analysis to see whether abbreviation already exist - WellKnownAbbreviationMap.TryGetValue(fullyQualifiedName, out var abbreviation); - return abbreviation; + if (WellKnownAbbreviationMap.TryGetValue(fullyQualifiedName, out var abbreviation)) { + // for now, use module wide unique name for abbreviation. even though technically we could use + // context based unique name since variable declared in lower scope will hide it and there is no conflict + return UniqueNameGenerator.Generate(analysis, abbreviation); + } + + return null; } private static string GetInsertionText(string insertionText, string abbreviation) => diff --git a/src/LanguageServer/Impl/Sources/HoverSource.cs b/src/LanguageServer/Impl/Sources/HoverSource.cs index 22ed0aeb5..4f843714e 100644 --- a/src/LanguageServer/Impl/Sources/HoverSource.cs +++ b/src/LanguageServer/Impl/Sources/HoverSource.cs @@ -54,27 +54,27 @@ public Hover GetHover(IDocumentAnalysis analysis, SourceLocation location) { var eval = analysis.ExpressionEvaluator; switch (statement) { case FromImportStatement fi when node is NameExpression nex: { - var contents = HandleFromImport(fi, location, hoverScopeStatement, analysis); - if (contents != null) { - return new Hover { - contents = contents, - range = range - }; - } - - break; + var contents = HandleFromImport(fi, location, hoverScopeStatement, analysis); + if (contents != null) { + return new Hover { + contents = contents, + range = range + }; } - case ImportStatement imp: { - var contents = HandleImport(imp, location, hoverScopeStatement, analysis); - if (contents != null) { - return new Hover { - contents = contents, - range = range - }; - } - break; + break; + } + case ImportStatement imp: { + var contents = HandleImport(imp, location, hoverScopeStatement, analysis); + if (contents != null) { + return new Hover { + contents = contents, + range = range + }; } + + break; + } } IMember value; @@ -96,13 +96,13 @@ public Hover GetHover(IDocumentAnalysis analysis, SourceLocation location) { IVariable variable = null; if (expr is NameExpression nex) { - analysis.ExpressionEvaluator.LookupNameInScopes(nex.Name, out _, out variable, LookupOptions.All); + eval.LookupNameInScopes(nex.Name, out _, out variable, LookupOptions.All); if (IsInvalidClassMember(variable, hoverScopeStatement, location.ToIndex(analysis.Ast))) { return null; } } - value = variable?.Value ?? analysis.ExpressionEvaluator.GetValueFromExpression(expr, LookupOptions.All); + value = variable?.Value ?? eval.GetValueFromExpression(expr, LookupOptions.All); type = value?.GetPythonType(); if (type == null) { return null; @@ -122,7 +122,7 @@ public Hover GetHover(IDocumentAnalysis analysis, SourceLocation location) { // In case of a member expression get the target since if we end up with method // of a generic class, the function will need specific type to determine its return // value correctly. I.e. in x.func() we need to determine type of x (self for func). - var v = analysis.ExpressionEvaluator.GetValueFromExpression(mex.Target); + var v = eval.GetValueFromExpression(mex.Target); self = v?.GetPythonType(); } @@ -165,7 +165,7 @@ private bool IsInvalidClassMember(IVariable v, ScopeStatement scope, int hoverPo } switch (v.Value) { case IPythonClassType cls when cls.ClassDefinition == scope: - return hoverPosition > cls.ClassDefinition.HeaderIndex; + return hoverPosition > cls.ClassDefinition.HeaderIndex; case IPythonFunctionType ft when ft.FunctionDefinition == scope: return hoverPosition > ft.FunctionDefinition.HeaderIndex; case IPythonPropertyType prop when prop.FunctionDefinition == scope: diff --git a/src/LanguageServer/Impl/Utilities/UniqueNameGenerator.cs b/src/LanguageServer/Impl/Utilities/UniqueNameGenerator.cs new file mode 100644 index 000000000..752c9f569 --- /dev/null +++ b/src/LanguageServer/Impl/Utilities/UniqueNameGenerator.cs @@ -0,0 +1,114 @@ +// 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, +// MERCHANTABILITY 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 System.Linq; +using Microsoft.Python.Analysis; +using Microsoft.Python.Analysis.Analyzer.Expressions; +using Microsoft.Python.Parsing.Ast; +using Microsoft.Python.Parsing.Extensions; + +namespace Microsoft.Python.LanguageServer.Utilities { + /// + /// Generate unique identifier based on given context + /// + internal class UniqueNameGenerator { + private readonly IDocumentAnalysis _analysis; + private readonly ScopeStatement _scope; + private readonly bool _uniqueInModule; + + public static string Generate(IDocumentAnalysis analysis, int position, string name) { + var generator = new UniqueNameGenerator(analysis, position); + return generator.Generate(name); + } + + public static string Generate(IDocumentAnalysis analysis, string name) { + var generator = new UniqueNameGenerator(analysis, position: -1); + return generator.Generate(name); + } + + public UniqueNameGenerator(IDocumentAnalysis analysis, int position) { + _analysis = analysis; + _uniqueInModule = position < 0; + + if (!_uniqueInModule) { + var finder = new ExpressionFinder(analysis.Ast, new FindExpressionOptions() { Names = true }); + finder.Get(position, position, out _, out _, out _scope); + } + } + + public string Generate(string name) { + // for now, there is only 1 new name rule which is just incrementing count at the end. + int count = 0; + Func getNextName = () => { + return $"{name}{++count}"; + }; + + // for now, everything is fixed. and there is no knob to control what to consider when + // creating unique name and how to create new name if there is a conflict + if (_uniqueInModule) { + return GenerateModuleWideUniqueName(name, getNextName); + } else { + return GenerateContextBasedUniqueName(name, getNextName); + } + } + + private string GenerateModuleWideUniqueName(string name, Func getNextName) { + // think of a better way to do this. + var leafScopes = GetLeafScopes(_analysis.Ast.ChildNodesDepthFirst().OfType()); + + while (true) { + if (!leafScopes.Any(s => NameExist(name, s))) { + return name; + } + + name = getNextName(); + } + } + + private HashSet GetLeafScopes(IEnumerable scopes) { + var set = scopes.ToHashSet(); + foreach (var scope in set.ToList()) { + if (scope.Parent != null) { + set.Remove(scope.Parent); + } + } + + return set; + } + + private bool NameExist(string name, ScopeStatement scope) { + var eval = _analysis.ExpressionEvaluator; + using (eval.OpenScope(_analysis.Document, scope)) { + return eval.LookupNameInScopes(name, Analysis.Analyzer.LookupOptions.All) != null; + } + } + + private string GenerateContextBasedUniqueName(string name, Func getNextName) { + var eval = _analysis.ExpressionEvaluator; + using (eval.OpenScope(_analysis.Document, _scope)) { + while (true) { + var member = eval.LookupNameInScopes(name, Analysis.Analyzer.LookupOptions.All); + if (member == null) { + return name; + } + + name = getNextName(); + } + } + } + } +} diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index b56d1602a..80899360e 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -291,7 +291,7 @@ await TestCodeActionAsync( title: "import pandas as pd", newText: "import pandas as pd" + Environment.NewLine + Environment.NewLine, abbreviation: "pd", - relativePaths: "pandas"); + relativePaths: "pandas.py"); } [TestMethod, Priority(0)] @@ -301,7 +301,7 @@ await TestCodeActionAsync( title: "from matplotlib import pyplot as plt", newText: "from matplotlib import pyplot as plt" + Environment.NewLine + Environment.NewLine, abbreviation: "plt", - relativePaths: @"matplotlib\pyplot"); + relativePaths: @"matplotlib\pyplot.py"); } [TestMethod, Priority(0)] @@ -315,7 +315,7 @@ await TestCodeActionAsync( title: "from matplotlib import pyplot as plt, test", newText: "from matplotlib import pyplot as plt, test", abbreviation: "plt", - relativePaths: new string[] { @"matplotlib\pyplot", @"matplotlib\test" }); + relativePaths: new string[] { @"matplotlib\pyplot.py", @"matplotlib\test.py" }); } [TestMethod, Priority(0)] @@ -325,7 +325,7 @@ await TestCodeActionAsync( title: "import pandas as pd", newText: "import pandas as pd" + Environment.NewLine + Environment.NewLine, abbreviation: "pd", - relativePaths: "pandas"); + relativePaths: "pandas.py"); } [TestMethod, Priority(0)] @@ -335,7 +335,7 @@ await TestCodeActionAsync( title: "from matplotlib import pyplot as plt", newText: "from matplotlib import pyplot as plt" + Environment.NewLine + Environment.NewLine, abbreviation: "plt", - relativePaths: @"matplotlib\pyplot"); + relativePaths: @"matplotlib\pyplot.py"); } [TestMethod, Priority(0)] @@ -349,7 +349,36 @@ await TestCodeActionAsync( title: "from matplotlib import pyplot as plt, test", newText: "from matplotlib import pyplot as plt, test", abbreviation: "plt", - relativePaths: new string[] { @"matplotlib\pyplot", @"matplotlib\test" }); + relativePaths: new string[] { @"matplotlib\pyplot.py", @"matplotlib\test.py" }); + } + + [TestMethod, Priority(0)] + public async Task AbbreviationConflict() { + var markup = @"{|insertionSpan:|}pd = 1 + +{|diagnostic:pandas|}"; + + await TestCodeActionAsync( + markup, + title: "import pandas as pd1", + newText: "import pandas as pd1" + Environment.NewLine + Environment.NewLine, + abbreviation: "pd1", + relativePaths: "pandas.py"); + } + + [TestMethod, Priority(0)] + public async Task AbbreviationConflict2() { + var markup = @"{|insertionSpan:|}{|diagnostic:pandas|} + +def Method(): + pd = 1"; + + await TestCodeActionAsync( + markup, + title: "import pandas as pd1", + newText: "import pandas as pd1" + Environment.NewLine + Environment.NewLine, + abbreviation: "pd1", + relativePaths: "pandas.py"); } [TestMethod, Priority(0)] @@ -442,7 +471,7 @@ private async Task TestCodeActionAsync(string markup, string title, string newTe var analysis = await GetAnalysisAsync(code); foreach (var relativePath in relativePaths) { - await GetAnalysisAsync("", analysis.ExpressionEvaluator.Services, modulePath: TestData.GetTestSpecificPath($"{relativePath}.py")); + await GetAnalysisAsync("", analysis.ExpressionEvaluator.Services, modulePath: TestData.GetTestSpecificPath(relativePath)); } // calculate actions diff --git a/src/LanguageServer/Test/UniqueNameGeneratorTests.cs b/src/LanguageServer/Test/UniqueNameGeneratorTests.cs new file mode 100644 index 000000000..4233ae81f --- /dev/null +++ b/src/LanguageServer/Test/UniqueNameGeneratorTests.cs @@ -0,0 +1,151 @@ +// 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, +// MERCHANTABILITY OR NON-INFRINGEMENT. +// +// See the Apache Version 2.0 License for specific language governing +// permissions and limitations under the License. + +using System.Reflection.PortableExecutable; +using System.Threading.Tasks; +using FluentAssertions; +using Microsoft.Python.Analysis; +using Microsoft.Python.LanguageServer.Tests.FluentAssertions; +using Microsoft.Python.LanguageServer.Utilities; +using Microsoft.Python.Parsing.Tests; +using Microsoft.Python.UnitTests.Core; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using TestUtilities; + +namespace Microsoft.Python.LanguageServer.Tests { + [TestClass] + public class UniqueNameGeneratorTests : LanguageServerTestBase { + public TestContext TestContext { get; set; } + + [TestInitialize] + public void TestInitialize() + => TestEnvironmentImpl.TestInitialize($"{TestContext.FullyQualifiedTestClassName}.{TestContext.TestName}"); + + [TestCleanup] + public void Cleanup() => TestEnvironmentImpl.TestCleanup(); + + [TestMethod, Priority(0)] + public async Task NoConflict() { + MarkupUtils.GetPosition(@"$$", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name"); + Test(analysis, "name", "name"); + } + + [TestMethod, Priority(0)] + public async Task Conflict_TopLevel() { + MarkupUtils.GetPosition(@"$$ + +name = 1", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name1"); + Test(analysis, "name", "name1"); + } + + [TestMethod, Priority(0)] + public async Task Conflict_TopLevel2() { + MarkupUtils.GetPosition(@"$$ + +class name: + pass", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name1"); + Test(analysis, "name", "name1"); + } + + [TestMethod, Priority(0)] + public async Task Conflict_Function() { + MarkupUtils.GetPosition(@"def Test(): + $$ + +name = 1", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name1"); + Test(analysis, "name", "name1"); + } + + [TestMethod, Priority(0)] + public async Task Conflict_Function2() { + MarkupUtils.GetPosition(@"def Test(): + name = 1 + $$ + pass", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name1"); + Test(analysis, "name", "name1"); + } + + [TestMethod, Priority(0)] + public async Task Conflict_Function3() { + MarkupUtils.GetPosition(@"def Test(): + name = 1 + +def Test2(): + $$ + pass", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name"); + Test(analysis, "name", "name1"); + } + + [TestMethod, Priority(0)] + public async Task Conflict_TopLevel3() { + MarkupUtils.GetPosition(@"def Test(): + name = 1 + +$$", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name"); + Test(analysis, "name", "name1"); + } + + [TestMethod, Priority(0)] + public async Task MultipleConflicts() { + MarkupUtils.GetPosition(@" +name1 = 1 + +class name3: + name2 = 1 + +def Test(): + name = 1 + + def name4(): + pass + +$$", out var code, out int position); + + var analysis = await GetAnalysisAsync(code); + Test(analysis, position, "name", "name"); + Test(analysis, "name", "name5"); + } + + private static void Test(IDocumentAnalysis analysis, int position, string name, string expected) { + var actual = UniqueNameGenerator.Generate(analysis, position, name); + actual.Should().Be(expected); + } + + private static void Test(IDocumentAnalysis analysis, string name, string expected) { + Test(analysis, position: -1, name, expected); + } + } +} From e71c803353fce4a2e83d2da107c275340165bad2 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 24 Oct 2019 11:01:23 -0700 Subject: [PATCH 40/43] change order of where we show special one (abbreviation one). now we show them at the top if there is any. --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 815d01fa7..90f88874c 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -79,9 +79,6 @@ public async Task> GetCodeActionsAsync(IDocumentAnalysis var codeActions = new List(); var diagnostics = new[] { diagnostic.ToDiagnostic() }; - // add given name as it is - await GetCodeActionsAsync(analysis, diagnostics, new Input(node, identifier), codeActions, cancellationToken); - // see whether it is one of abbreviation we specialize foreach (var moduleFullName in WellKnownAbbreviationMap.Where(kv => kv.Value == identifier).Select(kv => kv.Key)) { var moduleName = GetModuleName(moduleFullName); @@ -89,6 +86,9 @@ public async Task> GetCodeActionsAsync(IDocumentAnalysis await GetCodeActionsAsync(analysis, diagnostics, new Input(node, moduleName, moduleFullName), codeActions, cancellationToken); } + // add then search given name as it is + await GetCodeActionsAsync(analysis, diagnostics, new Input(node, identifier), codeActions, cancellationToken); + return codeActions; string GetModuleName(string moduleFullName) { From 4a44e7647183a44aa68ad61357e5400ed2585ae2 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Thu, 24 Oct 2019 11:24:52 -0700 Subject: [PATCH 41/43] add unit test for new ordering --- .../Test/MissingImportCodeActionTests.cs | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/src/LanguageServer/Test/MissingImportCodeActionTests.cs b/src/LanguageServer/Test/MissingImportCodeActionTests.cs index 80899360e..afaa77669 100644 --- a/src/LanguageServer/Test/MissingImportCodeActionTests.cs +++ b/src/LanguageServer/Test/MissingImportCodeActionTests.cs @@ -275,6 +275,32 @@ public async Task SymbolOrdering2() { importedMemberIndex.Should().BeLessThan(restIndex); } + [TestMethod, Priority(0)] + public async Task SymbolOrdering3() { + var markup = @"{|insertionSpan:|}{|diagnostic:pd|}"; + + MarkupUtils.GetNamedSpans(markup, out var code, out var spans); + + // get main analysis and add mock modules + var analysis = await GetAnalysisAsync(code); + + await GetAnalysisAsync("", analysis.ExpressionEvaluator.Services, modulePath: TestData.GetTestSpecificPath("pandas.py")); + await GetAnalysisAsync("", analysis.ExpressionEvaluator.Services, modulePath: TestData.GetTestSpecificPath("pd.py")); + + // calculate actions + var diagnosticSpan = spans["diagnostic"].First().ToSourceSpan(analysis.Ast); + var diagnostics = GetDiagnostics(analysis, diagnosticSpan, MissingImportCodeActionProvider.Instance.FixableDiagnostics); + var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); + + var list = codeActions.Select(c => c.title).ToList(); + var zipList = Enumerable.Range(0, list.Count).Zip(list); + + var pandasIndex = zipList.First(t => t.Second == "import pandas as pd").First; + var pdIndex = zipList.First(t => t.Second == "import pd").First; + + pandasIndex.Should().BeLessThan(pdIndex); + } + [TestMethod, Priority(0)] public async Task ModuleNotReachableFromUserDocument() { await TestCodeActionAsync( @@ -467,7 +493,7 @@ private static Diagnostic[] GetDiagnostics(IDocumentAnalysis analysis, SourceSpa private async Task TestCodeActionAsync(string markup, string title, string newText, string abbreviation, params string[] relativePaths) { MarkupUtils.GetNamedSpans(markup, out var code, out var spans); - // get main analysis and add mock pandas module + // get main analysis and add mock modules var analysis = await GetAnalysisAsync(code); foreach (var relativePath in relativePaths) { @@ -479,8 +505,6 @@ private async Task TestCodeActionAsync(string markup, string title, string newTe var diagnostics = GetDiagnostics(analysis, diagnosticSpan, MissingImportCodeActionProvider.Instance.FixableDiagnostics); var codeActions = await new CodeActionSource(analysis.ExpressionEvaluator.Services).GetCodeActionsAsync(analysis, diagnostics, CancellationToken.None); - var docTable = analysis.ExpressionEvaluator.Services.GetService(); - // verify results var codeAction = codeActions.Single(c => c.title == title); codeAction.edit.changes.Should().HaveCount(1); From 4fdbba7c089b2ad9050fed26500f46ee5f8dc69e Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 25 Oct 2019 11:07:20 -0700 Subject: [PATCH 42/43] addressed PR feedbacks --- .../Impl/Modules/Resolution/MainModuleResolution.cs | 10 +++++----- .../Impl/DependencyResolution/PathResolverSnapshot.cs | 6 +----- src/Core/Impl/Extensions/ArrayExtensions.cs | 9 +++++++-- .../CodeActions/MissingImportCodeActionProvider.cs | 2 +- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs b/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs index 137576469..d057b32e2 100644 --- a/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs +++ b/src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs @@ -56,17 +56,17 @@ public MainModuleResolution(string root, IServiceContainer services, ImmutableAr public IBuiltinsPythonModule BuiltinsModule { get; private set; } public IEnumerable GetImportedModules(CancellationToken cancellationToken) { + foreach (var module in _specialized.Values) { + cancellationToken.ThrowIfCancellationRequested(); + yield return module; + } + foreach (var moduleRef in Modules.Values) { cancellationToken.ThrowIfCancellationRequested(); if (moduleRef.Value != null) { yield return moduleRef.Value; } } - - foreach (var module in _specialized.Values) { - cancellationToken.ThrowIfCancellationRequested(); - yield return module; - } } protected override IPythonModule CreateModule(string name) { diff --git a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs index 09fa12fbb..f97e90212 100644 --- a/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs +++ b/src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs @@ -119,11 +119,7 @@ private ImmutableArray GetAllImportableModuleInfo(Func predica } public string GetModuleNameByPath(string modulePath) { - if (TryFindModule(modulePath, out var edge, out _)) { - return edge.End.FullModuleName; - } - - return null; + return TryFindModule(modulePath, out var edge, out _) ? edge.End.FullModuleName : null; } public ModuleImport GetModuleImportFromModuleName(in string fullModuleName) { diff --git a/src/Core/Impl/Extensions/ArrayExtensions.cs b/src/Core/Impl/Extensions/ArrayExtensions.cs index 4bc152b93..ce9185487 100644 --- a/src/Core/Impl/Extensions/ArrayExtensions.cs +++ b/src/Core/Impl/Extensions/ArrayExtensions.cs @@ -38,7 +38,9 @@ public static int IndexOf(this T[] array, TValue value, Func AddIfNotNull(this List list, T item) where T : class { + public static TCollection AddIfNotNull(this TCollection list, TItem item) + where TCollection : ICollection + where TItem : class { if (item == null) { return list; } @@ -46,7 +48,10 @@ public static List AddIfNotNull(this List list, T item) where T : class list.Add(item); return list; } - public static List AddIfNotNull(this List list, params T[] items) where T : class { + + public static TCollection AddIfNotNull(this TCollection list, params TItem[] items) + where TCollection : ICollection + where TItem : class { foreach (var item in items) { list.AddIfNotNull(item); } diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 90f88874c..39637ff27 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -184,7 +184,7 @@ private IEnumerable OrderFullNames(Dictionary import yield return fullName; } - // heuristic is we put entries with decl without any exports at the top + // heuristic is we put entries with decl without any exports (imported member with __all__) at the top // such as array. another example will be chararray. // this will make numpy chararray at the top and numpy defchararray at the bottom. // if we want, we can add more info to hide intermediate ones. From 4ce017a9bc07b7f3eedd236aa848326cda5889f5 Mon Sep 17 00:00:00 2001 From: HeeJae Chang Date: Fri, 25 Oct 2019 11:10:55 -0700 Subject: [PATCH 43/43] more refactoring --- .../Impl/CodeActions/MissingImportCodeActionProvider.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs index 39637ff27..9ae6c9e64 100644 --- a/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs +++ b/src/LanguageServer/Impl/CodeActions/MissingImportCodeActionProvider.cs @@ -93,11 +93,7 @@ public async Task> GetCodeActionsAsync(IDocumentAnalysis string GetModuleName(string moduleFullName) { var index = moduleFullName.LastIndexOf("."); - if (index < 0) { - return moduleFullName; - } - - return moduleFullName.Substring(index + 1); + return index < 0 ? moduleFullName : moduleFullName.Substring(index + 1); } }