Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Bug and test fixes #1322

Merged
Alexander Sher (AlexanderSher) merged 4 commits into from
Jul 12, 2019
Merged

Bug and test fixes #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
396ae07
Multiple analysis fixes.
AlexanderSher Jul 9, 2019
af06f7a
Build fix
AlexanderSher Jul 9, 2019
40f6bd7
- Fix #1314: NRE in HandleModuleImportStar bug
AlexanderSher Jul 12, 2019
9e1d86c
Use semantically correct check for the implicit package imports
AlexanderSher Jul 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/Analysis/Ast/Impl/Analyzer/Handlers/FromImportHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ private void AssignVariables(FromImportStatement node, IImportSearchResult impor
// TODO: warn this is not a good style per
// TODO: https://docs.python.org/3/faq/programming.html#what-are-the-best-practices-for-using-import-in-a-module
// TODO: warn this is invalid if not in the global scope.
HandleModuleImportStar(variableModule);
HandleModuleImportStar(variableModule, imports is ImplicitPackageImport);
return;
}

Expand All @@ -75,14 +75,16 @@ private void AssignVariables(FromImportStatement node, IImportSearchResult impor
}
}

private void HandleModuleImportStar(PythonVariableModule variableModule) {
private void HandleModuleImportStar(PythonVariableModule variableModule, bool isImplicitPackage) {
if (variableModule.Module == Module) {
// from self import * won't define any new members
return;
}

// If __all__ is present, take it, otherwise declare all members from the module that do not begin with an underscore.
var memberNames = variableModule.Analysis.StarImportMemberNames ?? variableModule.GetMemberNames().Where(s => !s.StartsWithOrdinal("_"));
var memberNames = isImplicitPackage
? variableModule.GetMemberNames()
: variableModule.Analysis.StarImportMemberNames ?? variableModule.GetMemberNames().Where(s => !s.StartsWithOrdinal("_"));

foreach (var memberName in memberNames) {
var member = variableModule.GetMember(memberName);
Expand Down
19 changes: 11 additions & 8 deletions src/Analysis/Ast/Impl/Analyzer/Handlers/ImportHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,25 +55,28 @@ private void HandleImport(ModuleName moduleImportExpression, NameExpression asNa
// import_module('fob.oar')
// import_module('fob.oar.baz')
var importNames = ImmutableArray<string>.Empty;
var variableModule = default(PythonVariableModule);
var importedNamesCount = 0;
var lastModule = default(PythonVariableModule);
var firstModule = default(PythonVariableModule);
foreach (var nameExpression in moduleImportExpression.Names) {
importNames = importNames.Add(nameExpression.Name);
var imports = ModuleResolution.CurrentPathResolver.GetImportsFromAbsoluteName(Module.FilePath, importNames, forceAbsolute);
if (!HandleImportSearchResult(imports, variableModule, asNameExpression, moduleImportExpression, out variableModule)) {
if (!HandleImportSearchResult(imports, lastModule, asNameExpression, moduleImportExpression, out lastModule)) {
lastModule = default;
break;
}

importedNamesCount++;
if (firstModule == default) {
firstModule = lastModule;
}
}

// "import fob.oar.baz as baz" is handled as baz = import_module('fob.oar.baz')
// "import fob.oar.baz" is handled as fob = import_module('fob')
if (!string.IsNullOrEmpty(asNameExpression?.Name) && importedNamesCount == moduleImportExpression.Names.Count) {
Eval.DeclareVariable(asNameExpression.Name, variableModule, VariableSource.Import, asNameExpression);
} else if (importedNamesCount > 0 && !string.IsNullOrEmpty(importNames[0]) && _variableModules.TryGetValue(importNames[0], out variableModule)) {
if (!string.IsNullOrEmpty(asNameExpression?.Name) && lastModule != default) {
Eval.DeclareVariable(asNameExpression.Name, lastModule, VariableSource.Import, asNameExpression);
} else if (firstModule != default && !string.IsNullOrEmpty(importNames[0])) {
var firstName = moduleImportExpression.Names[0];
Eval.DeclareVariable(importNames[0], variableModule, VariableSource.Import, firstName);
Eval.DeclareVariable(importNames[0], firstModule, VariableSource.Import, firstName);

@AlexanderSher Alexander Sher (AlexanderSher) Jul 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the fix for #1320. Name of the module may not match the name of the variable in Python 2x

}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Analysis/Ast/Impl/Analyzer/PythonAnalyzerSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ private void AnalyzeEntry(IDependencyChainNode<PythonAnalyzerEntry> node, Python

private void LogCompleted(IDependencyChainNode<PythonAnalyzerEntry> node, IPythonModule module, Stopwatch stopWatch, TimeSpan startTime) {
if (_log != null) {
var completed = node != null && module.Analysis is LibraryAnalysis ? "completed" : "completed for library";
var completed = node != null && module.Analysis is LibraryAnalysis ? "completed for library" : "completed ";

@AlexanderSher Alexander Sher (AlexanderSher) Jul 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct logging

var message = node != null
? $"Analysis of {module.Name}({module.ModuleType}) on depth {node.VertexDepth} {completed} in {(stopWatch.Elapsed - startTime).TotalMilliseconds} ms."
: $"Out of order analysis of {module.Name}({module.ModuleType}) completed in {(stopWatch.Elapsed - startTime).TotalMilliseconds} ms.";
Expand Down
6 changes: 0 additions & 6 deletions src/Analysis/Ast/Impl/Modules/Definitions/IModuleManagement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ public interface IModuleManagement: IModuleResolution {

bool TryAddModulePath(in string path, in bool allowNonRooted, out string fullName);

/// <summary>
/// Sets user search paths. This changes <see cref="IModuleResolution.CurrentPathResolver"/>.
/// </summary>
/// <returns>Added roots.</returns>
IEnumerable<string> SetUserSearchPaths(in IEnumerable<string> searchPaths);

/// <summary>
/// Provides ability to specialize module by replacing module import by
/// <see cref="IPythonModule"/> implementation in code. Real module
Expand Down
5 changes: 1 addition & 4 deletions src/Analysis/Ast/Impl/Modules/Resolution/MainModuleResolution.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,10 @@ public async Task ReloadAsync(CancellationToken cancellationToken = default) {
}

addedRoots.UnionWith(PathResolver.SetInterpreterSearchPaths(InterpreterPaths));
addedRoots.UnionWith(SetUserSearchPaths(userSearchPaths));
addedRoots.UnionWith(PathResolver.SetUserSearchPaths(userSearchPaths));
ReloadModulePaths(addedRoots);
}

public IEnumerable<string> SetUserSearchPaths(in IEnumerable<string> searchPaths)
=> PathResolver.SetUserSearchPaths(searchPaths);

// For tests
internal void AddUnimportableModule(string moduleName)
=> Modules[moduleName] = new ModuleRef(new SentinelModule(moduleName, _services));
Expand Down
23 changes: 10 additions & 13 deletions src/Analysis/Ast/Test/AnalysisTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// permissions and limitations under the License.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Threading;
Expand Down Expand Up @@ -42,7 +43,7 @@ public abstract class AnalysisTestBase {

protected virtual IDiagnosticsService GetDiagnosticsService(IServiceContainer s) => null;

protected virtual ServiceManager CreateServiceManager() {
protected ServiceManager CreateServiceManager() {
Services = new ServiceManager();

var platform = new OSPlatform();
Expand All @@ -55,26 +56,23 @@ protected virtual ServiceManager CreateServiceManager() {
return Services;
}

protected string GetAnalysisTestDataFilesPath() => TestData.GetPath(Path.Combine("TestData", "AstAnalysis"));
protected string GetAnalysisTestDataFilesPath() => TestData.GetPath("TestData", "AstAnalysis");

protected Task<IServiceManager> CreateServicesAsync(InterpreterConfiguration configuration, IServiceManager sm = null)
=> CreateServicesAsync(TestData.GetTestSpecificRootUri().AbsolutePath, configuration, null, sm);
protected Task<IServiceManager> CreateServicesAsync(InterpreterConfiguration configuration, string[] searchPaths = null)
=> CreateServicesAsync(TestData.GetTestSpecificRootPath(), configuration, null, null, searchPaths);

protected async Task<IServiceManager> CreateServicesAsync(string root, InterpreterConfiguration configuration, string stubCacheFolderPath = null, IServiceManager sm = null) {
protected async Task<IServiceManager> CreateServicesAsync(string root, InterpreterConfiguration configuration, string stubCacheFolderPath = null, IServiceManager sm = null, string[] searchPaths = null) {
configuration = configuration ?? PythonVersions.LatestAvailable;
configuration.AssertInstalled();
stubCacheFolderPath = stubCacheFolderPath ?? TestData.GetAstAnalysisCachePath(configuration.Version, true);
Trace.TraceInformation("Cache Path: " + stubCacheFolderPath);
configuration.SearchPaths = new[] { GetAnalysisTestDataFilesPath() };
configuration.SearchPaths = searchPaths ?? new[] { GetAnalysisTestDataFilesPath() };
configuration.TypeshedPath = TestData.GetDefaultTypeshedPath();

sm = sm ?? CreateServiceManager();

var clientApp = Substitute.For<IClientApplication>();
sm.AddService(clientApp);

var idle = Substitute.For<IIdleTimeService>();
sm.AddService(idle);
sm.AddService(Substitute.For<IClientApplication>())
.AddService(Substitute.For<IIdleTimeService>());

var ds = GetDiagnosticsService(Services);
if (ds != null) {
Expand All @@ -90,8 +88,7 @@ protected async Task<IServiceManager> CreateServicesAsync(string root, Interpret
sm.AddService(interpreter);

TestLogger.Log(TraceEventType.Information, "Create RunningDocumentTable");
var documentTable = new RunningDocumentTable(sm);
sm.AddService(documentTable);
sm.AddService(new RunningDocumentTable(sm));

return sm;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Analysis/Ast/Test/TypeshedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public async Task TypeStubConditionalDefine() {
}

Console.WriteLine(@"Testing with {0}", ver.InterpreterPath);
using (var s = await CreateServicesAsync(TestData.Root, ver)) {
using (var s = await CreateServicesAsync(ver)) {
var analysis = await GetAnalysisAsync(code, s, @"testmodule", TestData.GetTestSpecificPath(@"testmodule.pyi"));

var expected = new List<string>();
Expand Down
4 changes: 2 additions & 2 deletions src/Analysis/Core/Impl/DependencyResolution/PathResolverSnapshot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public IImportSearchResult GetImportsFromAbsoluteName(in string modulePath, in I
}

var rootEdges = _roots.Select((r, i) => new Edge(i, r));
if (!lastEdge.IsEmpty && !forceAbsolute) {
if (!lastEdge.IsEmpty && !forceAbsolute && !lastEdge.Previous.IsFirst) {
rootEdges = rootEdges.Prepend(lastEdge.Previous);
}

Expand Down Expand Up @@ -711,7 +711,7 @@ private bool TryFindModule(string modulePath, out Edge lastEdge, out StringSpan
var rootIndex = 0;
while (rootIndex < _roots.Count) {
var rootPath = _roots[rootIndex].Name;
if (normalizedPath.PathStartsWith(rootPath) && IsRootedPathEndsWithValidNames(normalizedPath, rootPath.Length)) {
if (PathUtils.PathStartsWith(normalizedPath, rootPath) && IsRootedPathEndsWithValidNames(normalizedPath, rootPath.Length)) {
break;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Analysis/Core/Impl/Interpreter/PythonLibraryPath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ public static async Task<List<PythonLibraryPath>> GetSearchPathsFromInterpreterA
try {
var output = await ps.ExecuteAndCaptureOutputAsync(startInfo, cancellationToken);
return output.Split(new[] { Environment.NewLine }, StringSplitOptions.RemoveEmptyEntries).Select(s => {
if (s.PathStartsWith(tempWorkingDir)) {
if (PathUtils.PathStartsWith(s, tempWorkingDir)) {
return null;
}
try {
Expand Down
3 changes: 0 additions & 3 deletions src/Core/Impl/Extensions/StringExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,6 @@ public static string QuoteArgument(this string arg) {
return "\"{0}\"".FormatInvariant(arg);
}

public static bool PathStartsWith(this string s, string prefix)

@MikhailArkhipov Mikhail Arkhipov (MikhailArkhipov) Jul 12, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have it instead calling PathUtils.PathStartsWith ?

@AlexanderSher Alexander Sher (AlexanderSher) Jul 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bit of a mess in our extensions. From my understanding, StringExtensions should have things that are applicable to any strings, PathUtils should have string operations that are related to paths only, and FileSystemExtensions should have things like FindFile.

=> s?.StartsWith(prefix, PathsStringComparison) ?? false;

public static bool StartsWithOrdinal(this string s, string prefix, bool ignoreCase = false)
=> s?.StartsWith(prefix, ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal) ?? false;

Expand Down
18 changes: 15 additions & 3 deletions src/Core/Impl/IO/PathUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,14 @@ public static bool IsValidFileNameCharacter(char character)
/// Returns true if the path has a directory separator character at the end.
/// </summary>
public static bool HasEndSeparator(string path)
=> !string.IsNullOrEmpty(path) && DirectorySeparators.Contains(path[path.Length - 1]);
=> !string.IsNullOrEmpty(path) && IsDirectorySeparator(path[path.Length - 1]);


public static bool IsDirectorySeparator(char c) => Array.IndexOf(DirectorySeparators, c) != -1;

public static bool PathStartsWith(string s, string prefix)
=> s != null && s.StartsWith(prefix, StringExtensions.PathsStringComparison) &&
(s.Length == prefix.Length || IsDirectorySeparator(s[prefix.Length]));

@AlexanderSher Alexander Sher (AlexanderSher) Jul 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fix for #1321, to ensure that part of folder name isn't considered as part of the path.


/// <summary>
/// Removes up to one directory separator character from the end of path.
Expand Down Expand Up @@ -400,7 +407,7 @@ public static bool IsNormalizedPath(string path) {
var altSeparator = Path.AltDirectorySeparatorChar;
int offset;
if (path.Length >= 1 && path[0] == separator) {
offset = 1;
offset = IsUncStart(path) ? 2 : 1;
} else if (IsWindows && path.Length >= 2 && IsValidWindowsDriveChar(path[0]) && path[1] == ':') {
offset = 2;
} else {
Expand Down Expand Up @@ -436,6 +443,11 @@ public static bool IsNormalizedPath(string path) {
return !(impossibleName || last == '.' || char.IsWhiteSpace(last));
}

private static bool IsUncStart(string path) {
var separator = Path.DirectorySeparatorChar;
return IsWindows && path.Length >= 2 && path[0] == separator && path[1] == separator;
}

/// <summary>
/// Normalizes and returns the provided path.
/// </summary>
Expand All @@ -451,7 +463,7 @@ public static string NormalizePath(string path) {
return path;
}

var root = EnsureEndSeparator(Path.GetPathRoot(path));
var root = IsUncStart(path) ? new string(Path.DirectorySeparatorChar, 2) : EnsureEndSeparator(Path.GetPathRoot(path));
var parts = path.Substring(root.Length).Split(DirectorySeparators);
var isDir = string.IsNullOrWhiteSpace(parts[parts.Length - 1]);

Expand Down
15 changes: 5 additions & 10 deletions src/LanguageServer/Test/CompletionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,8 +716,7 @@ public async Task ImportInPackage() {
var module2Path = TestData.GetTestSpecificUri("package", "module2.py");
var module3Path = TestData.GetTestSpecificUri("package", "sub_package", "module3.py");

var root = TestData.GetTestSpecificRootUri().AbsolutePath;
await CreateServicesAsync(root, PythonVersions.LatestAvailable3X);
await CreateServicesAsync(PythonVersions.LatestAvailable3X);
var rdt = Services.GetService<IRunningDocumentTable>();

var module1 = rdt.OpenDocument(module1Path, "import package.");
Expand Down Expand Up @@ -937,8 +936,7 @@ public async Task FromDotInRootWithInitPy() {
var initPyPath = TestData.GetTestSpecificUri("__init__.py");
var module1Path = TestData.GetTestSpecificUri("module1.py");

var root = TestData.GetTestSpecificRootUri().AbsolutePath;
await CreateServicesAsync(root, PythonVersions.LatestAvailable3X);
await CreateServicesAsync(PythonVersions.LatestAvailable3X);
var rdt = Services.GetService<IRunningDocumentTable>();

rdt.OpenDocument(initPyPath, string.Empty);
Expand All @@ -959,8 +957,7 @@ public async Task FromDotInExplicitPackage() {
var module2Path = TestData.GetTestSpecificUri("package", "module2.py");
var module3Path = TestData.GetTestSpecificUri("package", "sub_package", "module3.py");

var root = TestData.GetTestSpecificRootUri().AbsolutePath;
await CreateServicesAsync(root, PythonVersions.LatestAvailable3X);
await CreateServicesAsync(PythonVersions.LatestAvailable3X);
var rdt = Services.GetService<IRunningDocumentTable>();
var analyzer = Services.GetService<IPythonAnalyzer>();

Expand All @@ -983,8 +980,7 @@ public async Task FromPartialName() {
var module1Path = TestData.GetTestSpecificUri("package", "module1.py");
var module2Path = TestData.GetTestSpecificUri("package", "sub_package", "module2.py");

var root = TestData.GetTestSpecificRootUri().AbsolutePath;
await CreateServicesAsync(root, PythonVersions.LatestAvailable3X);
await CreateServicesAsync(PythonVersions.LatestAvailable3X);
var rdt = Services.GetService<IRunningDocumentTable>();

var module = rdt.OpenDocument(initPyPath, "answer = 42");
Expand All @@ -1009,8 +1005,7 @@ public async Task FromDotInImplicitPackage() {
var module2 = TestData.GetTestSpecificUri("package", "module2.py");
var module3 = TestData.GetTestSpecificUri("package", "sub_package", "module3.py");

var root = TestData.GetTestSpecificRootUri().AbsolutePath;
await CreateServicesAsync(root, PythonVersions.LatestAvailable3X);
await CreateServicesAsync(PythonVersions.LatestAvailable3X);
var rdt = Services.GetService<IRunningDocumentTable>();

var module = rdt.OpenDocument(module1, "from .");
Expand Down
4 changes: 2 additions & 2 deletions src/LanguageServer/Test/GoToDefinitionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,8 +328,8 @@ def __init__(self):
def foo(self):
return self.bar.get_name()
";
var root = TestData.GetTestSpecificRootUri().AbsolutePath;
await CreateServicesAsync(root, PythonVersions.LatestAvailable3X);

await CreateServicesAsync(PythonVersions.LatestAvailable3X);
var rdt = Services.GetService<IRunningDocumentTable>();

var mainPath = TestData.GetTestSpecificUri("main.py");
Expand Down
Loading