diff --git a/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs b/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs index 847bbc000..ba11bc09d 100644 --- a/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs +++ b/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}")] @@ -117,6 +119,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 diff --git a/src/Analysis/Ast/Impl/Diagnostics/ErrorCodes.cs b/src/Analysis/Ast/Impl/Diagnostics/ErrorCodes.cs index 45c6a1407..fdf6f03f8 100644 --- a/src/Analysis/Ast/Impl/Diagnostics/ErrorCodes.cs +++ b/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"; } } diff --git a/src/Analysis/Ast/Impl/Resources.Designer.cs b/src/Analysis/Ast/Impl/Resources.Designer.cs index 785cbf9a8..365bba623 100644 --- a/src/Analysis/Ast/Impl/Resources.Designer.cs +++ b/src/Analysis/Ast/Impl/Resources.Designer.cs @@ -249,6 +249,15 @@ internal static string PropertyOfUnknownType { } } + /// + /// Looks up a localized string similar to Explicit return in __init__ . + /// + internal static string ReturnInInit { + get { + return ResourceManager.GetString("ReturnInInit", resourceCulture); + } + } + /// /// Looks up a localized string similar to Unable to determine analysis cache path. Using default '{0}'.. /// diff --git a/src/Analysis/Ast/Impl/Resources.resx b/src/Analysis/Ast/Impl/Resources.resx index 95cd89002..b882f93f7 100644 --- a/src/Analysis/Ast/Impl/Resources.resx +++ b/src/Analysis/Ast/Impl/Resources.resx @@ -189,4 +189,7 @@ Unable to determine analysis cache path. Exception: {0}. Using default '{1}'. + + Explicit return in __init__ + \ No newline at end of file diff --git a/src/Analysis/Ast/Test/LintReturnInInitTests.cs b/src/Analysis/Ast/Test/LintReturnInInitTests.cs new file mode 100644 index 000000000..4ae7626a2 --- /dev/null +++ b/src/Analysis/Ast/Test/LintReturnInInitTests.cs @@ -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(); + } + } +}