Use PathResolverSnapshot to find modules. #361

AlexanderSher · 2018-11-08T16:45:08Z

No description provided.

src/Analysis/Engine/Impl/DependencyResolution/AvailablePackageImports.cs

src/Analysis/Engine/Impl/DependencyResolution/IAvailableImports.cs

src/Analysis/Engine/Impl/Analyzer/DDG.cs

src/Analysis/Engine/Impl/DependencyResolution/PathResolverSnapshot.Node.cs

src/Analysis/Engine/Impl/DependencyResolution/PathResolverSnapshot.cs

MikhailArkhipov · 2018-11-08T23:38:11Z

src/Analysis/Engine/Impl/DependencyResolution/PathResolverSnapshot.cs

-            var rootNodes = searchPaths.Where(Path.IsPathRooted).Select(Node.CreateRoot).Prepend(_roots[0]).ToArray();
-            return new PathResolverSnapshot(_pythonLanguageVersion, rootNodes, Version + 1);
+        public PathResolverSnapshot SetUserSearchPaths(in IEnumerable<string> searchPaths) {
+            var userSearchPaths = searchPaths.ToArray();


User search paths can contain anything, like . for example. VSC passed . as one of the paths normally. Also, since we pass them down verbatim from user settings, I'd try/catch IOException since they can contain random characters and may be malformed.

PathResolver never accesses file system, so there can't be any IOExceptions. Search paths are preserved in their form cause when _rootDirectory is changed, we can try to convert relative path to absolute.

I think Path.Combine(rootDirectory, p) etc also throw when path parts are invalid, but that is ArgumentException

src/Analysis/Engine/Impl/DependencyResolution/PathResolverSnapshot.cs

src/Analysis/Engine/Impl/Infrastructure/PathUtils.cs

MikhailArkhipov · 2018-11-09T00:23:59Z

BTW, how does it work with AstAnalysisWalker? It currently goes to AstNestedPythonModule which calls _interpreter.ImportModule bypassing analyzer. Should we have a service for the name resolution rather than stick it to the analyzer? The Ast* one is actually sees more complicated cases since it handles library modules imports.

MikhailArkhipov · 2018-11-09T00:30:20Z

src/Analysis/Engine/Impl/Infrastructure/PathUtils.cs

@@ -382,6 +408,46 @@ static class PathUtils {
            return null;
        }

+        public static bool IsNormalizedPath(string path) {


I wonder if we can just PathUtil.NormalizePath(path) == path?

PathUtil.NormalizePath(path) produces enormous amount of garbage. During initial load, some of that garbage even ends up in Gen2. So the goal was the opposite - don't create new path if the original one is normalized already.

AlexanderSher · 2018-11-12T23:36:47Z

Unfortunately, AstAnalysisWalker doesn't have a lot to share with DDG, cause they work differently:

DDG immediately tries to load module while walking through ImportStatement and FromImportStatement and assign all imported values to the variables. If module can't be loaded immediately because of circular dependency, it will use a sentinel, but because of that dependency module will be reanalyzed, and sentinel will be (most likely) replaced with the real module.
AstAnalysisWalker doesn't load anything (with exception for from module import * case). It creates AstNestedPythonModule for imported modules and AstNestedPythonModuleMember instance for imported members, which later resolve dependencies through lazy loading when variable is accessed. If module isn't imported at the moment of resolution, AstNestedPythonModule preserves sentinel module, and it will never be replaced with a real one. It is probably done intentionally to avoid complexity.

So the only thing I can share is PathResolver itself. I'll submit changes to AstAnalysisWalker in the next commit to this [WIP].

- Fixed namespace package resolution in completions (microsoft#12) - Fixed support for sys.modules (microsoft#363) - Added several tests (microsoft#365) - Fixed support for implicit relative module names (microsoft#366) - Added support for multiple matching rules in PathResolver (microsoft#367) Not yet implemented: - Fix imports of binary modules (microsoft#362) - Fix stubs handling - AstAnalysisWalker - More tests - Code cleanup

MikhailArkhipov · 2018-11-20T19:47:08Z

src/Analysis/Engine/Impl/DependencyResolution/PathResolverSnapshot.cs

@@ -14,132 +14,344 @@
 // See the Apache Version 2.0 License for specific language governing
 // permissions and limitations under the License.

+using Microsoft.PythonTools.Analysis.Infrastructure;
+using Microsoft.PythonTools.Parsing;


src/LanguageServer/Impl/Program.cs

MikhailArkhipov · 2018-11-20T19:50:47Z

This needs tons of tests :-)

MikhailArkhipov · 2018-11-23T22:56:17Z

I assume we merge it post Nov release?

- Use PathResolverSnapshot in AstAnalysisWalker - More cleanup Bug to fix: PathResolverSnapshot.AddModulePath should add path to all roots where it matches, not just the first one.

… is created - Added support for native module file names with versions

AlexanderSher · 2018-12-03T18:32:47Z

Ok, I've removed the WIP prefix. There are some known bugs, but they can be fixed separately.

# Conflicts: # src/Analysis/Engine/Impl/PythonAnalyzer.cs

…-server into NamespacePackages # Conflicts: # src/Analysis/Engine/Impl/Interpreter/Ast/AstBuiltinsPythonModule.cs

jakebailey · 2018-12-03T21:46:43Z

Given that this dependency resolver is independent from the analyzer and really only makes use of the AST, would it make more sense to move this into one of the new assemblies Mikhail is planning on creating in a future PR?

The only using from Analysis is Microsoft.PythonTools.Analysis.Infrastructure, which I would expect to be moved/reworked to a more common location anyway since it contains a lot of useful extensions.

MikhailArkhipov · 2018-12-04T05:59:00Z

@jakebailey - I will move the resolver to the proper assembly with my PR so new code can access it. Currently there is no separate assembly yet.

Use PathResolverSnapshot to find modules.

WIP: Use PathResolverSnapshot to find modules.

fa5479f