Improve handling of circular and imported class bases (microsoft#1497)

* Add test for GPy (fails)

* Disambiguate bases

* Handle base loops better

* GPy test (on ignore)

* PT feedback + test

* PR feedback

* Pass template class

* Add declaration position extension

* Pass template class

* Change eval to scope

* Add test for scoped import

* Adjust comparison

Author: Mikhail Arkhipov

src/Analysis/Ast/Impl/Analyzer/Evaluation/ExpressionEval.Scopes.cs

@@ -47,14 +47,23 @@ public void DeclareVariable(string name, IMember value, VariableSource source, N
             => DeclareVariable(name, value, source, GetLocationOfName(location), overwrite);
 
         public void DeclareVariable(string name, IMember value, VariableSource source, Location location, bool overwrite = true) {
+            if (source == VariableSource.Import) {
+                // Duplicate declaration so if variable gets overwritten it can still be retrieved. Consider:
+                //    from X import A
+                //    class A(A): ...
+                CurrentScope.DeclareImported(name, value, location);
+            }
+
             var member = GetInScope(name);
             if (member != null && !overwrite) {
                 return;
             }
+            
             if (source == VariableSource.Import && value is IVariable v) {
                 CurrentScope.LinkVariable(name, v, location);
                 return;
             }
+            
             if (member != null) {
                 if (!value.IsUnknown()) {
                     CurrentScope.DeclareVariable(name, value, source, location);

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

@@ -80,16 +80,7 @@ public void HandleAssignment(AssignmentStatement node) {
             TryHandleClassVariable(node, value);
         }
 
-        private bool IsValidAssignment(string name, Location loc) {
-            if (Eval.GetInScope(name) is ILocatedMember m) {
-                // Class and function definition are processed first, so only override
-                // if assignment happens after declaration
-                if (loc.IndexSpan.Start < m.Location.IndexSpan.Start) {
-                    return false;
-                }
-            }
-            return true;
-        }
+        private bool IsValidAssignment(string name, Location loc) => !Eval.GetInScope(name).IsDeclaredAfter(loc);
 
         public void HandleAnnotatedExpression(ExpressionWithAnnotation expr, IMember value) {
             if (expr?.Annotation == null) {

src/Analysis/Ast/Impl/Analyzer/PythonAnalyzerSession.cs

@@ -361,9 +361,9 @@ 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 for library" : "completed ";
+                var completed = node != null && module.Analysis is LibraryAnalysis ? "completed for library" : "completed";
                 var message = node != null
-                    ? $"Analysis of {module.Name}({module.ModuleType}) on depth {node.VertexDepth} {completed} in {(stopWatch.Elapsed - startTime).TotalMilliseconds} ms."
+                    ? $"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.";
                 _log.Log(TraceEventType.Verbose, message);
             }

src/Analysis/Ast/Impl/Analyzer/Symbols/ClassEvaluator.cs

@@ -53,7 +53,7 @@ public void EvaluateClass() {
                 _class = classInfo;
 
                 var bases = ProcessBases();
-                _class.SetBases(bases);
+                _class.SetBases(bases, Eval.CurrentScope);
                 _class.DecideGeneric();
                 // Declare __class__ variable in the scope.
                 Eval.DeclareVariable("__class__", _class, VariableSource.Declaration);

src/Analysis/Ast/Impl/Extensions/EvaluatorExtensions.cs

@@ -24,6 +24,7 @@ public static IMember LookupNameInScopes(this IExpressionEvaluator eval, string
             => eval.LookupNameInScopes(name, out scope, out _, options);
         public static IMember LookupNameInScopes(this IExpressionEvaluator eval, string name, LookupOptions options = LookupOptions.Normal)
             => eval.LookupNameInScopes(name, out _, out _, options);
+
         public static IDisposable OpenScope(this IExpressionEvaluator eval, IPythonClassType cls)
          => eval.OpenScope(cls.DeclaringModule, cls.ClassDefinition);
         public static IDisposable OpenScope(this IExpressionEvaluator eval, IPythonFunctionType ft)

src/Analysis/Ast/Impl/Extensions/MemberExtensions.cs

@@ -14,7 +14,6 @@
 // permissions and limitations under the License.
 
 using System.Diagnostics;
-using Microsoft.Python.Analysis.Specializations.Typing;
 using Microsoft.Python.Analysis.Types;
 using Microsoft.Python.Analysis.Values;
 
@@ -106,5 +105,16 @@ public static ILocatedMember GetRootDefinition(this ILocatedMember lm) {
             return parent;
         }
 
+        public static bool IsDeclaredAfter(this IMember m, Location loc)
+            => m is ILocatedMember lm && lm.IsDeclaredAfter(loc);
+
+        public static bool IsDeclaredAfter(this ILocatedMember lm, ILocatedMember other)
+            => lm.IsDeclaredAfter(other.Location);
+        public static bool IsDeclaredAfter(this ILocatedMember lm, Location loc)
+            => lm.Location.IndexSpan.Start > loc.IndexSpan.Start;
+        public static bool IsDeclaredAfterOrAt(this ILocatedMember lm, ILocatedMember other)
+            => lm.IsDeclaredAfterOrAt(other.Location);
+        public static bool IsDeclaredAfterOrAt(this ILocatedMember lm, Location loc)
+            => lm.Location.IndexSpan.Start >= loc.IndexSpan.Start;
     }
 }

src/Analysis/Ast/Impl/Extensions/ScopeExtensions.cs

@@ -15,12 +15,35 @@
 
 using System.Linq;
 using Microsoft.Python.Analysis.Documents;
+using Microsoft.Python.Analysis.Types;
 using Microsoft.Python.Analysis.Values;
 using Microsoft.Python.Core.Text;
 using Microsoft.Python.Parsing.Ast;
 
 namespace Microsoft.Python.Analysis.Analyzer {
     public static class ScopeExtensions {
+        public static IMember LookupNameInScopes(this IScope currentScope, string name, out IScope scope) {
+            scope = null;
+            foreach (var s in currentScope.EnumerateTowardsGlobal) {
+                if (s.Variables.TryGetVariable(name, out var v) && v != null) {
+                    scope = s;
+                    return v.Value;
+                }
+            }
+            return null;
+        }
+
+        public static IMember LookupImportedNameInScopes(this IScope currentScope, string name, out IScope scope) {
+            scope = null;
+            foreach (var s in currentScope.EnumerateTowardsGlobal) {
+                if (s.Imported.TryGetVariable(name, out var v) && v != null) {
+                    scope = s;
+                    return v.Value;
+                }
+            }
+            return null;
+        }
+
         public static int GetBodyStartIndex(this IScope scope) {
             switch (scope.Node) {
                 case ClassDefinition cd:
@@ -34,7 +57,7 @@ public static int GetBodyStartIndex(this IScope scope) {
             }
         }
 
-        public static bool IsNestedInScope(this IScope s, IScope outer) 
+        public static bool IsNestedInScope(this IScope s, IScope outer)
             => s.OuterScope != null && s.OuterScope.EnumerateTowardsGlobal.Any(x => x == outer);
 
         public static IScope FindScope(this IScope parent, IDocument document, SourceLocation location) {

src/Analysis/Ast/Impl/Types/PythonClassType.Generics.cs

@@ -23,13 +23,14 @@
 
 namespace Microsoft.Python.Analysis.Types {
     internal partial class PythonClassType {
+        private readonly ReentrancyGuard<IPythonClassType> _genericSpecializationGuard = new ReentrancyGuard<IPythonClassType>();
+        private readonly ReentrancyGuard<IPythonClassType> _genericResolutionGuard = new ReentrancyGuard<IPythonClassType>();
+        private readonly object _genericParameterLock = new object();
+
         private bool _isGeneric;
-        private object _genericParameterLock = new object();
         private Dictionary<string, PythonClassType> _specificTypeCache;
         private Dictionary<IGenericTypeParameter, IPythonType> _genericParameters;
         private IReadOnlyList<IGenericTypeParameter> _parameters = new List<IGenericTypeParameter>();
-        private ReentrancyGuard<IPythonClassType> _genericSpecializationGuard = new ReentrancyGuard<IPythonClassType>();
-        private ReentrancyGuard<IPythonClassType> _genericResolutionGuard = new ReentrancyGuard<IPythonClassType>();
 
         #region IGenericType
         /// <summary>
@@ -60,23 +61,19 @@ internal partial class PythonClassType {
         /// </summary>
         public IPythonType CreateSpecificType(IArgumentSet args) {
             lock (_genericParameterLock) {
-                var genericTypeParameters = GetTypeParameters();
+                var newGenericTypeParameters = GetTypeParameters();
                 var newBases = new List<IPythonType>();
 
                 // Get map of generic type parameter to specific type - fill in what type goes to what
                 // type parameter T -> int, U -> str, etc.
-                var genericTypeToSpecificType = GetSpecificTypes(args, genericTypeParameters, newBases);
+                var genericTypeToSpecificType = GetSpecificTypes(args, newGenericTypeParameters, newBases);
 
-                PythonClassType classType = new PythonClassType(BaseName, new Location(DeclaringModule));
+                var classType = new PythonClassType(BaseName, new Location(DeclaringModule));
                 // Storing generic parameters allows methods returning generic types 
                 // to know what type parameter returns what specific type
-                StoreGenericParameters(classType, genericTypeParameters, genericTypeToSpecificType);
+                classType.StoreGenericParameters(this, newGenericTypeParameters, genericTypeToSpecificType);
 
                 // Set generic name
-                if (!classType._genericParameters.IsNullOrEmpty()) {
-                    classType._genericName = CodeFormatter.FormatSequence(BaseName, '[', classType._genericParameters.Values);
-                }
-
                 // Locking so threads can only access class after it's been initialized
                 // Store generic parameters first so name updates correctly, then check if class type has been cached
                 _specificTypeCache = _specificTypeCache ?? new Dictionary<string, PythonClassType>();
@@ -105,8 +102,8 @@ public IPythonType CreateSpecificType(IArgumentSet args) {
                     // Get list of bases that are generic but not generic class parameters, e.g A[T], B[T] but not Generic[T1, T2]
                     var genericTypeBases = bases.Except(genericClassParameters).OfType<IGenericType>().Where(g => g.IsGeneric).ToArray();
 
-                    // Removing all generic bases, and will only specialize genericTypeBases, remove generic class paramters entirely
-                    // We remove generic class paramters entirely because the type information is now stored in GenericParameters field
+                    // Removing all generic bases, and will only specialize genericTypeBases, remove generic class parameters entirely
+                    // We remove generic class parameters entirely because the type information is now stored in GenericParameters field
                     // We still need generic bases so we can specialize them 
                     var specificBases = bases.Except(genericTypeBases).Except(genericClassParameters).ToList();
 
@@ -127,12 +124,12 @@ public IPythonType CreateSpecificType(IArgumentSet args) {
                     }
 
                     // Set specific class bases 
-                    classType.SetBases(specificBases.Concat(newBases));
+                    classType.SetBases(specificBases.Concat(newBases), args.Eval.CurrentScope);
                     // Now that parameters are set, check if class is generic
                     classType._parameters = classType._genericParameters.Values.Distinct().OfType<IGenericTypeParameter>().ToList();
                     classType.DecideGeneric();
                     // Transfer members from generic to specific type.
-                    SetClassMembers(classType, args);
+                    classType.SetClassMembers(this, args);
                 }
                 return classType;
             }
@@ -154,7 +151,7 @@ private IGenericTypeParameter[] GetTypeParameters() {
             var genericClassParameter = bases.OfType<IGenericClassParameter>().FirstOrDefault();
 
             // If Generic[...] is present, ordering of type variables is determined from that
-            if (genericClassParameter != null && genericClassParameter.TypeParameters != null) {
+            if (genericClassParameter?.TypeParameters != null) {
                 fromBases.UnionWith(genericClassParameter.TypeParameters);
             } else {
                 // otherwise look at the generic class bases
@@ -214,7 +211,7 @@ private IReadOnlyDictionary<IGenericTypeParameter, IPythonType> GetSpecificTypes
                     // for the copy constructor. Consider 'class A(Generic[K, V], Mapping[K, V])'
                     // constructed as 'd = {1:'a', 2:'b'}; A(d)'. Here we look through bases
                     // and see if any matches the builtin type id. For example, Mapping or Dict
-                    // will have BultinTypeId.Dict and we can figure out specific types from
+                    // will have BuiltinTypeId.Dict and we can figure out specific types from
                     // the content of the collection.
                     var b = _bases.OfType<IGenericType>().Where(g => g.IsGeneric).FirstOrDefault(x => x.TypeId == type.TypeId);
                     if (b != null && !b.Parameters.IsNullOrEmpty()) {
@@ -259,33 +256,40 @@ private IReadOnlyDictionary<IGenericTypeParameter, IPythonType> GetSpecificTypes
         /// Points the generic type parameter in class type to their corresponding specific type (or a generic
         /// type parameter if no specific type was provided)
         /// </summary>
-        private void StoreGenericParameters(PythonClassType classType, IGenericTypeParameter[] genericParameters, IReadOnlyDictionary<IGenericTypeParameter, IPythonType> genericToSpecificTypes) {
+        private void StoreGenericParameters(
+            IPythonClassType templateClass,
+            IEnumerable<IGenericTypeParameter> newGenericParameters,
+            IReadOnlyDictionary<IGenericTypeParameter, IPythonType> genericToSpecificTypes) {
+
             // copy original generic parameters over and try to fill them in
-            classType._genericParameters = new Dictionary<IGenericTypeParameter, IPythonType>(GenericParameters.ToDictionary(k => k.Key, k => k.Value));
+            _genericParameters = templateClass.GenericParameters.ToDictionary(k => k.Key, k => k.Value);
 
             // Case when creating a new specific class type
-            if (Parameters.Count == 0) {
+            if (templateClass.Parameters.Count == 0) {
                 // Assign class type generic type parameters to specific types 
-                for (var i = 0; i < genericParameters.Length; i++) {
-                    var gb = genericParameters[i];
-                    classType._genericParameters[gb] = genericToSpecificTypes.TryGetValue(gb, out var v) ? v : null;
+                foreach (var gb in newGenericParameters) {
+                    _genericParameters[gb] = genericToSpecificTypes.TryGetValue(gb, out var v) ? v : null;
                 }
             } else {
                 // When Parameters field is not empty then need to update generic parameters field
-                foreach (var gp in GenericParameters.Keys) {
-                    if (GenericParameters[gp] is IGenericTypeParameter specificType) {
+                foreach (var gp in templateClass.GenericParameters.Keys) {
+                    if (templateClass.GenericParameters[gp] is IGenericTypeParameter specificType) {
                         // Get unfilled type parameter or type parameter that was filled with another type parameter
                         // and try to fill it in
                         // e.g 
                         // class A(Generic[T]):
                         // class B(A[U])
                         // A has T => U
-                        classType._genericParameters[gp] = genericToSpecificTypes.TryGetValue(specificType, out var v) ? v : null;
+                        _genericParameters[gp] = genericToSpecificTypes.TryGetValue(specificType, out var v) ? v : null;
                     }
                 }
             }
+
+            if (!_genericParameters.IsNullOrEmpty()) {
+                _genericName = CodeFormatter.FormatSequence(BaseName, '[', _genericParameters.Values);
+            }
         }
- 
+
         /// <summary>
         /// Given generic type such as Generic[T1, T2, ...] attempts to extract specific types
         /// for its parameters from an argument value. Handles common cases such as dictionary,
@@ -331,24 +335,24 @@ private void GetSpecificTypeFromArgumentValue(IGenericType gt, object argumentVa
         /// Transfers members from generic class to the specific class type
         /// while instantiating specific types for the members.
         /// </summary>
-        private void SetClassMembers(PythonClassType classType, IArgumentSet args) {
-            // Add members from the template class (this one).
+        private void SetClassMembers(IPythonClassType templateClass, IArgumentSet args) {
+            // Add members from the template class.
             // Members must be clones rather than references since
             // we are going to set specific types on them.
-            classType.AddMembers(this, true);
+            AddMembers(templateClass, true);
 
             // Resolve return types of methods, if any were annotated as generics
-            var members = classType.GetMemberNames()
+            var members = GetMemberNames()
                 .Except(new[] { "__class__", "__bases__", "__base__" })
-                .ToDictionary(n => n, classType.GetMember);
+                .ToDictionary(n => n, GetMember);
 
             // Create specific types.
             // Functions handle generics internally upon the call to Call.
             foreach (var m in members) {
                 switch (m.Value) {
                     case IPythonTemplateType tt when tt.IsGeneric(): {
                             var specificType = tt.CreateSpecificType(args);
-                            classType.AddMember(m.Key, specificType, true);
+                            AddMember(m.Key, specificType, true);
                             break;
                         }
                     case IPythonInstance inst: {
@@ -359,12 +363,12 @@ private void SetClassMembers(PythonClassType classType, IArgumentSet args) {
                                     specificType = tt.CreateSpecificType(args);
                                     break;
                                 case IGenericTypeParameter gtd:
-                                    classType.GenericParameters.TryGetValue(gtd, out specificType);
+                                    GenericParameters.TryGetValue(gtd, out specificType);
                                     break;
                             }
 
                             if (specificType != null) {
-                                classType.AddMember(m.Key, new PythonInstance(specificType), true);
+                                AddMember(m.Key, new PythonInstance(specificType), true);
                             }
                             break;
                         }