Fixes #3437 Typeshed integration #4264

zooba · 2018-05-25T23:42:13Z

Fixes #3437 Typeshed integration
Enables value merging from typeshed
Improves some handling of type annotations in AST imported files

Enables value merging from typeshed Improves some handling of type annotations in AST imported files

zooba · 2018-05-25T23:42:47Z

@MikhailArkhipov In case you want to watch my work in progress on this. The basics are in there, just need to beef up the tests and the handling of the typing module when doing AST imports.

MikhailArkhipov · 2018-05-29T19:17:42Z

Python/Product/Analysis/Infrastructure/PathUtils.cs

+        /// This should be used in place of:
+        /// <c>Path.GetDirectoryName(CommonUtils.TrimEndSeparator(path)) + Path.DirectorySeparatorChar</c>
+        /// </remarks>
+        public static string GetParent(string path) {


You can technically do Path.GetFullPath(Path.Combine(path, ".."))

That's much slower and far more pedantic about paths (it will actually validate the paths...)

MikhailArkhipov · 2018-05-29T20:05:12Z

Python/Product/Analysis/Interpreter/Ast/AstPythonInterpreterFactory.cs

+            if (typeShedPaths != null) {
+                return typeShedPaths;
+            }
+            lock (_searchPathsLock) {


Looks like duplicate?

It's a double check after taking the lock, which we then release immediately to do the asynchronous collection. But I'll move the re-lock earlier so we are less likely to search for the paths multiple times.

Fixes PythonCompiledRegex Removes USE_TYPESHED variable

zooba · 2018-06-01T21:16:58Z

@MikhailArkhipov @huguesv This is ready for review

huguesv · 2018-06-01T21:21:22Z

Python/Product/Analysis/Interpreter/Ast/ImportScanner.cs

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


missing header

Huh, this whole file is unused and not meant to be checked in yet... whoops

AlexanderSher · 2018-06-01T21:25:46Z

Python/Product/Analysis/Analyzer/InterpreterScope.cs

@@ -289,15 +289,15 @@ internal bool RemoveVariable(string name, out VariableDef value)
        }

        internal virtual void ClearNodeValues() {
-            _nodeValues.Clear();
+            _nodeValues = new AnalysisDictionary<Node, NodeValue>();


References to these dictionaries are publicly available through properties. Is it safe to change the instance instead of calling Clear()?

Yes. We only change them on the thread that accesses it, and we replace it specifically so that references obtained through the properties continue to point at the old dictionary.

AlexanderSher · 2018-06-01T21:55:18Z

Python/Product/Analysis/Interpreter/Ast/AstPythonFunction.cs

+            DeclaringModule = original.DeclaringModule;
+            DeclaringType = original.DeclaringType;
+            Name = original.Name;
+            _doc = (original as AstPythonFunction)?._doc ?? original.Documentation;


AstPythonFunction.Documentation first checks for _doc as well, no need to check for it separately.

I meant to copy the null from _doc, but this doesn't do that either :) So a slightly different change coming

AlexanderSher · 2018-06-01T21:56:24Z

Python/Product/Analysis/Interpreter/Ast/AstPythonFunction.cs

+            IsClassMethod = original.IsClassMethod;
+            IsStatic = original.IsStatic;
+            _overloads = original.Overloads.ToList();
+            Locations = (original as AstPythonFunction)?.Locations ?? (original as ILocatedMember)?.Locations ?? Array.Empty<LocationInfo>();


AstPythonFunction implements ILocatedMember, so no need to check AstPythonFunction by itself

AlexanderSher · 2018-06-01T22:04:20Z

Python/Product/Analysis/Interpreter/Ast/AstPythonInterpreterFactory.cs

+
+        private IReadOnlyList<string> GetTypeShedPaths(int timeout) {
+            // First check
+            var typeShedPaths = _typeShedPaths;


_typeShedPaths is accessed outside of the lock

Deliberately, so we can avoid locking for the search if the search has already been done (it'll only ever be done once - we never change _typeShedPaths after initialization - but it depends on context too much to make it Lazy<T>).

Since _searchPathsLock is used only for locking, it will be a thin lock and the overhead of using it is close to nothing:
https://blogs.msdn.microsoft.com/seteplia/2017/09/06/managed-object-internals-part-2-object-header-layout-and-the-cost-of-locking/
(removed second article cause it is using custom testing technique instead of reliable benchmark)

Not using it has no overhead, and it's only for initialization.

It's also read outside the async to avoid any overhead there.

huguesv · 2018-06-01T22:07:53Z

Python/Tests/Analysis/AstAnalysisTests.cs

+            PythonPaths.Versions.LastOrDefault(v => Directory.Exists(Path.Combine(v.PrefixPath, "Lib", "site-packages", "typeshed")));
+
+        [TestMethod, Priority(0)]
+        public void TypeShedElementTree() {


I hacked this test to run under a version of python that doesn't have typeshed installed, and it still passes. I'm not sure what to make of that.

:( It means our analysis is so good that typeshed doesn't actually help here. Which version did you run it on? The one I tried seemed to need it

Also, an easy way to disable type shed is to go into AstPythonInterpreter, search for IncludeTypeShed and set it to false (which I'm currently turning into one of our secret registry key settings)

In the latest code, you can disable it by setting: HKEY_CURRENT_USER\Software\Microsoft\PythonTools\15.0\Analysis\Project@UseTypeStubPackages to 0 (defaults to 1). You can also set UseTypeStubPackagesExclusively to 1 to not merge with the original results.

AlexanderSher · 2018-06-01T22:08:20Z

Python/Product/Analysis/Interpreter/Ast/AstPythonMultipleModules.cs

+                return mmy;
+            } else if (mmx != null && mmy != null) {
+                mmx.AddModules(mmy._modules);
+                return mmx;


Does it matter anywhere that we return mmx instead of mmy?

No. This should only be being used while we're constructing objects and before the analyzer ever gets a hold of it (great question though)

huguesv · 2018-06-01T22:23:56Z

Python/Product/Analysis/Interpreter/Ast/AstPythonMultipleMembers.cs

@@ -39,6 +39,13 @@ class AstPythonMultipleMembers : IPythonMultipleMembers, ILocatedMember {
            _checkForLazy = true;
        }

+        public IMember Trim() {


What if _checkForLazy is true? Will the caller know/expect to see a ILazyMember? If not you can return Members[0] which will do the lazy get.

Oh, calling Members might resize _members potentially going from size 1 to 0

Good point though, should really check that here.

Actually, decided not to change it. If we trim down to a single lazy member, we should probably leave it lazy even if it will eventually (depending on context) resolve down to nothing.

huguesv · 2018-06-01T22:57:30Z

With typeshed installed:

Without:

huguesv · 2018-06-01T23:08:26Z

I was expecting to see the types for the urlopen parameters. Error list is from running mypy.

Are they removed intentionally? For my own method with type hints, the type appears:

zooba · 2018-06-01T23:33:41Z

About to push a fix for the module members issue, and it seems we're currently not resolving parameter annotations into displayable types, so there's a bit more work to do there. The default values of ... need to be ignored properly as well.

Fixes module members not combining shared names. Fixes crash when parsing tuple assignment.

AlexanderSher · 2018-06-02T01:37:28Z

Python/Product/Analysis/Interpreter/Ast/AstPythonInterpreterFactory.cs

+            // Assume caller has already done the quick checks
+
+            IReadOnlyList<string> typeShedPaths;
+            var typeshed = await FindModuleInSearchPathAsync("typeshed");


Is it ok that FindModuleInSearchPathAsync can be called several times concurrently before _typeShedPaths is finally assigned?

Yep. We could call it every time if we wanted, it doesn't change any state. But since its results (rarely) change, better to cache it.

zooba · 2018-06-04T19:09:03Z

Still need to fix that test before merging, but that's all I'm doing

huguesv · 2018-06-04T20:10:56Z

Python/Product/Analysis/Interpreter/Ast/AstAnalysisFunctionWalker.cs

        }

-        public IList<IPythonType> ReturnTypes => _returnTypes;
+        public IList<IPythonType> ReturnTypes => _overload.ReturnTypes;


according to the constructor, _overload can be null.

it's used in a few other places without checks, so I suppose ctor should throw if null.

huguesv · 2018-06-04T20:20:48Z

Just that one comment.

Fixes mishandling of linked variables that was breaking tests

[WIP] Fixes microsoft#3437 Typeshed integration

af5ca63

Enables value merging from typeshed Improves some handling of type annotations in AST imported files

zooba added the status:In Progress label May 25, 2018

MikhailArkhipov reviewed May 29, 2018

View reviewed changes

zooba added 3 commits May 29, 2018 15:17

Merge from master

8b82112

Fix issues in scrape_module

211ce11

Fixes PythonCompiledRegex Removes USE_TYPESHED variable

Fixes a number of analyzer tests

b0a32a6

zooba removed the status:In Progress label May 31, 2018

zooba changed the title ~~[WIP] Fixes #3437 Typeshed integration~~ Fixes #3437 Typeshed integration May 31, 2018

huguesv reviewed Jun 1, 2018

View reviewed changes

AlexanderSher reviewed Jun 1, 2018

View reviewed changes

Remove file that was not supposed to be checked in.

2bbf6d8

AlexanderSher reviewed Jun 1, 2018

View reviewed changes

huguesv reviewed Jun 1, 2018

View reviewed changes

AlexanderSher reviewed Jun 1, 2018

View reviewed changes

huguesv reviewed Jun 1, 2018

View reviewed changes

Allow disabling type stub packages or making them exclusive.

dd6e50c

Fixes module members not combining shared names. Fixes crash when parsing tuple assignment.

AlexanderSher reviewed Jun 2, 2018

View reviewed changes

zooba added 3 commits June 4, 2018 09:08

Code review feedback

7576275

[WIP] Expand test

71f8e19

Merge from master

2857f32

huguesv approved these changes Jun 4, 2018

View reviewed changes

Fixes handling of parameter annotations and "..." default values.

1eeb0b3

huguesv reviewed Jun 4, 2018

View reviewed changes

zooba added 2 commits June 4, 2018 15:16

Fixes AstAnalysisFunctionWalker constructor

8d95edc

Fixes mishandling of linked variables that was breaking tests

Fixes 2.6 support

be5c25b

zooba merged commit 814dd9b into microsoft:master Jun 5, 2018

zooba deleted the issue-3437 branch June 5, 2018 16:36

Fixes #3437 Typeshed integration #4264

Fixes #3437 Typeshed integration #4264

Conversation

zooba commented May 25, 2018

zooba commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zooba commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderSher Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huguesv commented Jun 1, 2018

huguesv commented Jun 1, 2018

zooba commented Jun 1, 2018

Choose a reason for hiding this comment

zooba Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

zooba commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huguesv commented Jun 4, 2018

AlexanderSher Jun 2, 2018 •

edited

Loading

zooba Jun 4, 2018 •

edited

Loading