When returning in init method, report a diagnostic error (microsoft#1261)

* When returning in init method, report a diagnostic error

* Adding check and tests for returning None before adding error for a return statement in the init function

* Cleaning up tests

Author: CTrando

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

@@ -18,13 +18,15 @@
 using System.Diagnostics;
 using System.Linq;
 using Microsoft.Python.Analysis.Analyzer.Evaluation;
-using Microsoft.Python.Analysis.Extensions;
+using Microsoft.Python.Analysis.Diagnostics;
 using Microsoft.Python.Analysis.Modules;
 using Microsoft.Python.Analysis.Types;
 using Microsoft.Python.Analysis.Values;
 using Microsoft.Python.Analysis.Values.Collections;
 using Microsoft.Python.Core;
+using Microsoft.Python.Parsing;
 using Microsoft.Python.Parsing.Ast;
+using ErrorCodes = Microsoft.Python.Analysis.Diagnostics.ErrorCodes;
 
 namespace Microsoft.Python.Analysis.Analyzer.Symbols {
     [DebuggerDisplay("{FunctionDefinition.Name}")]
@@ -116,6 +118,18 @@ public override bool Walk(AssignmentStatement node) {
         public override bool Walk(ReturnStatement node) {
             var value = Eval.GetValueFromExpression(node.Expression);
             if (value != null) {
+                // although technically legal, __init__ in a constructor should not have a not-none return value
+                if (FunctionDefinition.Name.EqualsOrdinal("__init__") && _function.DeclaringType.MemberType == PythonMemberType.Class 
+                    && !value.IsOfType(BuiltinTypeId.NoneType)) { 
+
+                    Eval.ReportDiagnostics(Module.Uri, new Diagnostics.DiagnosticsEntry(
+                            Resources.ReturnInInit,
+                            node.GetLocation(Module).Span,
+                            ErrorCodes.ReturnInInit,
+                            Severity.Warning,
+                            DiagnosticSource.Analysis));
+                }
+
                 _overload.AddReturnValue(value);
             }
             return true; // We want to evaluate all code so all private variables in __new__ get defined

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

@@ -25,5 +25,6 @@ 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 ReturnInInit = "return-in-init";
     }
 }

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

@@ -189,4 +189,7 @@
   <data name="UnableToDetermineCachePathException" xml:space="preserve">
     <value>Unable to determine analysis cache path. Exception: {0}. Using default '{1}'.</value>
   </data>
+  <data name="ReturnInInit" xml:space="preserve">
+    <value>Explicit return in __init__ </value>
+  </data>
 </root>

src/Analysis/Ast/Impl/Resources.resx

@@ -0,0 +1,127 @@
+// Copyright(c) Microsoft Corporation
+// All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the License); you may not use
+// this file except in compliance with the License. You may obtain a copy of the
+// License at http://www.apache.org/licenses/LICENSE-2.0
+//
+// THIS CODE IS PROVIDED ON AN  *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS
+// OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION ANY
+// IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR PURPOSE,
+// MERCHANTABILITY OR NON-INFRINGEMENT.
+//
+// See the Apache Version 2.0 License for specific language governing
+// permissions and limitations under the License.
+
+using System.Linq;
+using System.Threading.Tasks;
+using FluentAssertions;
+using Microsoft.Python.Analysis.Tests.FluentAssertions;
+using Microsoft.Python.Parsing;
+using Microsoft.Python.Parsing.Tests;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using TestUtilities;
+
+namespace Microsoft.Python.Analysis.Tests {
+    [TestClass]
+    public class LintReturnInInitTests : AnalysisTestBase {
+        public TestContext TestContext { get; set; }
+
+        [TestInitialize]
+        public void TestInitialize()
+            => TestEnvironmentImpl.TestInitialize($"{TestContext.FullyQualifiedTestClassName}.{TestContext.TestName}");
+
+        [TestCleanup]
+        public void Cleanup() => TestEnvironmentImpl.TestCleanup();
+
+        [TestMethod, Priority(0)]
+        public async Task ReturnInInit() {
+            const string code = @"
+class Rectangle:
+    def __init__(self, width, height):
+        self.width = width
+        self.height = height
+        self.area = width * height
+        return self.area
+
+r = Rectangle(10, 10)
+";
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().HaveCount(1);
+
+            var diagnostic = analysis.Diagnostics.ElementAt(0);
+            diagnostic.SourceSpan.Should().Be(7, 9, 7, 25);
+            diagnostic.Severity.Should().Be(Severity.Warning);
+            diagnostic.ErrorCode.Should().Be(Diagnostics.ErrorCodes.ReturnInInit);
+            diagnostic.Message.Should().Be(Resources.ReturnInInit);
+        }
+
+        [TestMethod, Priority(0)]
+        public async Task ReturnInitBasic() {
+            const string code = @"
+class Rectangle:
+    def __init__(self, width, height):
+        return 2
+
+r = Rectangle(10, 10)
+";
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().HaveCount(1);
+
+            var diagnostic = analysis.Diagnostics.ElementAt(0);
+            diagnostic.SourceSpan.Should().Be(4, 9, 4, 17);
+            diagnostic.Severity.Should().Be(Severity.Warning);
+            diagnostic.ErrorCode.Should().Be(Diagnostics.ErrorCodes.ReturnInInit);
+            diagnostic.Message.Should().Be(Resources.ReturnInInit);
+        }
+
+        [TestMethod, Priority(0)]
+        public async Task ReturnInInitConditional() {
+            const string code = @"
+class A:
+    def __init__(self, x):
+        self.x = x
+        if x > 0:
+            return 10
+
+a = A(1)
+";
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().HaveCount(1);
+
+            var diagnostic = analysis.Diagnostics.ElementAt(0);
+            diagnostic.SourceSpan.Should().Be(6, 13, 6, 22);
+            diagnostic.Severity.Should().Be(Severity.Warning);
+            diagnostic.ErrorCode.Should().Be(Diagnostics.ErrorCodes.ReturnInInit);
+            diagnostic.Message.Should().Be(Resources.ReturnInInit);
+        }
+
+        [TestMethod, Priority(0)]
+        public async Task ReturnNoneInInit() {
+            const string code = @"
+class A:
+    def __init__(self, x):
+        self.x = x
+        self.x += 1
+        return None
+
+a = A(1)
+";
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().BeEmpty();
+        }
+
+        [TestMethod, Priority(0)]
+        public async Task EmptyReturnInInit() {
+            const string code = @"
+class A:
+    def __init__(self, x):
+        self.x = x
+        return 
+a = A(1)
+";
+            var analysis = await GetAnalysisAsync(code);
+            analysis.Diagnostics.Should().BeEmpty();
+        }
+    }
+}