On redefinition of function, give a diagnostic message (microsoft#1273)

* Doing preliminary function redefinition check, need to add testS

* Fixing up error message and cleaning up code

* Adding some light tests

* Reordering

* Adding some more tests on function redefinition

* Switching to nicer syntax in SymbolCollector

* Renaming test files

* Adding redefined tests when importing as well

* handling properties too

* Switching to switch statement

* Minor syntax change

* Unused import

* Switching type out for interface

Author: CTrando

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

@@ -17,6 +17,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using Microsoft.Python.Analysis.Analyzer.Evaluation;
+using Microsoft.Python.Analysis.Diagnostics;
 using Microsoft.Python.Analysis.Types;
 using Microsoft.Python.Analysis.Values;
 using Microsoft.Python.Core;
@@ -98,19 +99,29 @@ private PythonClassType CreateClass(ClassDefinition cd) {
 
         private void AddFunctionOrProperty(FunctionDefinition fd) {
             var declaringType = fd.Parent != null && _typeMap.TryGetValue(fd.Parent, out var t) ? t : null;
-            if (!TryAddProperty(fd, declaringType)) {
-                AddFunction(fd, declaringType);
+            var existing = _eval.LookupNameInScopes(fd.Name, LookupOptions.Local);
+
+            switch (existing?.MemberType) {
+                case PythonMemberType.Method:
+                case PythonMemberType.Function:
+                case PythonMemberType.Property:
+                    ReportRedefinedFunction(fd, existing as PythonType);
+                    break;
+            }
+
+            if (!TryAddProperty(fd, existing, declaringType)) {
+                AddFunction(fd, existing, declaringType);
             }
         }
 
-        private void AddFunction(FunctionDefinition fd, IPythonType declaringType) {
-            if (!(_eval.LookupNameInScopes(fd.Name, LookupOptions.Local) is PythonFunctionType existing)) {
-                existing = new PythonFunctionType(fd, declaringType, _eval.GetLocationOfName(fd));
+        private void AddFunction(FunctionDefinition fd, IMember existing, IPythonType declaringType) {
+            if (!(existing is PythonFunctionType f)) {
+                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.
-                _eval.DeclareVariable(fd.Name, existing, VariableSource.Declaration);
+                _eval.DeclareVariable(fd.Name, f, VariableSource.Declaration);
             }
-            AddOverload(fd, existing, o => existing.AddOverload(o));
+            AddOverload(fd, f, o => f.AddOverload(o));
         }
 
         private void AddOverload(FunctionDefinition fd, IPythonClassMember function, Action<IPythonFunctionOverload> addOverload) {
@@ -148,31 +159,31 @@ private PythonFunctionOverload GetOverloadFromStub(FunctionDefinition node) {
             return null;
         }
 
-        private bool TryAddProperty(FunctionDefinition node, IPythonType declaringType) {
+        private bool TryAddProperty(FunctionDefinition node, IMember existing, IPythonType declaringType) {
             var dec = node.Decorators?.Decorators;
             var decorators = dec != null ? dec.ExcludeDefault().ToArray() : Array.Empty<Expression>();
 
             foreach (var d in decorators.OfType<NameExpression>()) {
                 switch (d.Name) {
                     case @"property":
-                        AddProperty(node, declaringType, false);
+                        AddProperty(node, existing, declaringType, false);
                         return true;
                     case @"abstractproperty":
-                        AddProperty(node, declaringType, true);
+                        AddProperty(node, existing, declaringType, true);
                         return true;
                 }
             }
             return false;
         }
 
-        private void AddProperty(FunctionDefinition fd, IPythonType declaringType, bool isAbstract) {
-            if (!(_eval.LookupNameInScopes(fd.Name, LookupOptions.Local) is PythonPropertyType existing)) {
-                existing = new PythonPropertyType(fd, _eval.GetLocationOfName(fd), declaringType, isAbstract);
+        private void AddProperty(FunctionDefinition fd, IMember existing, IPythonType declaringType, bool isAbstract) {
+            if (!(existing is PythonPropertyType p)) {
+                p = new PythonPropertyType(fd, _eval.GetLocationOfName(fd), declaringType, isAbstract);
                 // The variable is transient (non-user declared) hence it does not have location.
                 // Property type is tracking locations for references and renaming.
-                _eval.DeclareVariable(fd.Name, existing, VariableSource.Declaration);
+                _eval.DeclareVariable(fd.Name, p, VariableSource.Declaration);
             }
-            AddOverload(fd, existing, o => existing.AddOverload(o));
+            AddOverload(fd, p, o => p.AddOverload(o));
         }
 
         private IMember GetMemberFromStub(string name) {
@@ -202,6 +213,22 @@ private IMember GetMemberFromStub(string name) {
             return member;
         }
 
+        private void ReportRedefinedFunction(FunctionDefinition redefined, ILocatedMember existing) {
+            // get line number of existing function for diagnostic message
+            var existingLoc = existing.Definition;
+            var existingLine = existingLoc.Span.Start.Line;
+
+            _eval.ReportDiagnostics(
+                _eval.Module.Uri,
+                new DiagnosticsEntry(
+                    Resources.FunctionRedefined.FormatInvariant(existingLine),
+                    // only highlight the redefined name
+                    _eval.GetLocationInfo(redefined.NameExpression).Span,
+                    ErrorCodes.FunctionRedefined,
+                    Parsing.Severity.Error,
+                    DiagnosticSource.Analysis));
+        }
+
         private static bool IsDeprecated(ClassDefinition cd)
             => cd.Decorators?.Decorators != null && IsDeprecated(cd.Decorators.Decorators);
 

src/Analysis/Ast/Impl/Diagnostics/ErrorCodes.cs

@@ -25,6 +25,8 @@ public static class ErrorCodes {
         public const string UndefinedVariable = "undefined-variable";
         public const string VariableNotDefinedGlobally= "variable-not-defined-globally";
         public const string VariableNotDefinedNonLocal = "variable-not-defined-nonlocal";
+        public const string FunctionRedefined = "function-redefined";
+        public const string UnsupportedOperandType = "unsupported-operand-type";
         public const string ReturnInInit = "return-in-init";
         public const string TypingNewTypeArguments = "typing-newtype-arguments";
         public const string TypingGenericArguments = "typing-generic-arguments";

src/Analysis/Ast/Impl/Resources.Designer.cs

@@ -189,6 +189,9 @@
   <data name="UnableToDetermineCachePathException" xml:space="preserve">
     <value>Unable to determine analysis cache path. Exception: {0}. Using default '{1}'.</value>
   </data>
+  <data name="FunctionRedefined" xml:space="preserve">
+    <value>Function already defined at line {0}.</value>
+  </data>
   <data name="NewTypeFirstArgNotString" xml:space="preserve">
     <value>The first argument to NewType must be a string, but it is of type '{0}'.</value>
   </data>