From f1b234fe66085c59675c180213a42b9329de527e Mon Sep 17 00:00:00 2001 From: Cameron Trando Date: Thu, 27 Jun 2019 16:17:09 -0700 Subject: [PATCH 1/4] When returning in init method, report a diagnostic error --- .../Analyzer/Symbols/FunctionEvaluator.cs | 14 +++- .../Ast/Impl/Diagnostics/ErrorCodes.cs | 1 + src/Analysis/Ast/Impl/Resources.Designer.cs | 9 +++ src/Analysis/Ast/Impl/Resources.resx | 3 + .../Ast/Test/LintReturnInInitTests.cs | 76 +++++++++++++++++++ 5 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 src/Analysis/Ast/Test/LintReturnInInitTests.cs diff --git a/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs b/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs index 847bbc000..a9e384697 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,16 @@ 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__ should not have a return value + if (FunctionDefinition.Name.EqualsOrdinal("__init__") && _function.DeclaringType.MemberType == PythonMemberType.Class) { + 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..ac13b83ad --- /dev/null +++ b/src/Analysis/Ast/Test/LintReturnInInitTests.cs @@ -0,0 +1,76 @@ +// 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.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.Severity.Should().Be(Severity.Warning); + diagnostic.ErrorCode.Should().Be(Diagnostics.ErrorCodes.ReturnInInit); + diagnostic.Message.Should().Be(Resources.ReturnInInit); + } + } +} From 5a3c55d09630af7f176ddad65e80fa66167f5a7f Mon Sep 17 00:00:00 2001 From: Cameron Trando Date: Mon, 1 Jul 2019 10:15:24 -0700 Subject: [PATCH 2/4] Adding check and tests for returning None before adding error for a return statement in the init function --- .../Analyzer/Symbols/FunctionEvaluator.cs | 6 ++- .../Ast/Test/LintReturnInInitTests.cs | 48 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs b/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs index a9e384697..ba11bc09d 100644 --- a/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs +++ b/src/Analysis/Ast/Impl/Analyzer/Symbols/FunctionEvaluator.cs @@ -119,8 +119,10 @@ 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__ should not have a return value - if (FunctionDefinition.Name.EqualsOrdinal("__init__") && _function.DeclaringType.MemberType == PythonMemberType.Class) { + // 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, diff --git a/src/Analysis/Ast/Test/LintReturnInInitTests.cs b/src/Analysis/Ast/Test/LintReturnInInitTests.cs index ac13b83ad..de0af1bb0 100644 --- a/src/Analysis/Ast/Test/LintReturnInInitTests.cs +++ b/src/Analysis/Ast/Test/LintReturnInInitTests.cs @@ -72,5 +72,53 @@ def __init__(self, width, height): 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.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().HaveCount(0); + } + + [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().HaveCount(0); + } } } From fd6480549c442573d895f804d8823145240a6d0e Mon Sep 17 00:00:00 2001 From: Cameron Trando Date: Mon, 1 Jul 2019 10:37:19 -0700 Subject: [PATCH 3/4] Using BeEmpty() in tests --- src/Analysis/Ast/Test/LintReturnInInitTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Analysis/Ast/Test/LintReturnInInitTests.cs b/src/Analysis/Ast/Test/LintReturnInInitTests.cs index de0af1bb0..9fe0321ec 100644 --- a/src/Analysis/Ast/Test/LintReturnInInitTests.cs +++ b/src/Analysis/Ast/Test/LintReturnInInitTests.cs @@ -105,7 +105,7 @@ def __init__(self, x): a = A(1) "; var analysis = await GetAnalysisAsync(code); - analysis.Diagnostics.Should().HaveCount(0); + analysis.Diagnostics.Should().BeEmpty(); } [TestMethod, Priority(0)] @@ -118,7 +118,7 @@ def __init__(self, x): a = A(1) "; var analysis = await GetAnalysisAsync(code); - analysis.Diagnostics.Should().HaveCount(0); + analysis.Diagnostics.Should().BeEmpty(); } } } From 0bd5e6b668f6002359c3c169f457bd37e4ec6f13 Mon Sep 17 00:00:00 2001 From: Cameron Trando Date: Mon, 1 Jul 2019 15:04:04 -0700 Subject: [PATCH 4/4] Adding source span tests when adding diagnostics --- src/Analysis/Ast/Test/LintReturnInInitTests.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Analysis/Ast/Test/LintReturnInInitTests.cs b/src/Analysis/Ast/Test/LintReturnInInitTests.cs index 9fe0321ec..4ae7626a2 100644 --- a/src/Analysis/Ast/Test/LintReturnInInitTests.cs +++ b/src/Analysis/Ast/Test/LintReturnInInitTests.cs @@ -50,6 +50,7 @@ def __init__(self, width, height): 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); @@ -68,6 +69,7 @@ def __init__(self, width, height): 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); @@ -88,6 +90,7 @@ def __init__(self, x): 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);