Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

A test to ensure @abc.abstractproperty return value is analyzed correctly #131

Merged
MikhailArkhipov merged 3 commits into from
Sep 26, 2018
Merged

A test to ensure @abc.abstractproperty return value is analyzed correctly #131

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7832170
added an inheritance test for property type
lostmsu Sep 25, 2018
9eecb28
fixed processing of @abc's decorators
lostmsu Sep 25, 2018
5fc8652
use C# 7 syntax when possible
lostmsu Sep 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Analysis/Engine/Impl/Analyzer/FunctionAnalysisUnit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ private bool ProcessAbstractDecorators(IAnalysisSet decorator) {

// Only handle these if they are specialized
foreach (var d in decorator.OfType<SpecializedCallable>()) {
if (d.DeclaringModule?.ModuleName != "abc") {
if (d.DeclaringModule != null
&& d.DeclaringModule.ModuleName != "abc") {
continue;
}

Expand Down
9 changes: 9 additions & 0 deletions src/Analysis/Engine/Impl/Values/SpecializedNamespace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,15 @@ public override IEnumerable<ILocationInfo> Locations {
}
}

public override string Name {
get {
Copy link
Contributor

@AlexanderSher AlexanderSher Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be written with expression:

public override string Name => _original != null ? _original.Name : base.Name;

Copy link

@MikhailArkhipov MikhailArkhipov Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probable even

public override string Name => _original?.Name ?? base.Name;

Copy link
Contributor

@AlexanderSher AlexanderSher Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's a bit different semantics. In original code, if _original != null and _original.Name == null, it will return null. In case of _original?.Name ?? base.Name, it will return base.Name. Can't say what is the right behavior though.

Copy link
Contributor Author

@lostmsu lostmsu Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure if the project wants to maintain compatibility with older C# versions, so just followed the patterns around (you never know what reviewers would prefer). Will change to C# 7.

Copy link
Contributor Author

@lostmsu lostmsu Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in 5fc8652

if (_original == null)
return base.Name;

return _original.Name;
}
}

public override IEnumerable<OverloadResult> Overloads {
get {
if (_original == null) {
Expand Down
28 changes: 27 additions & 1 deletion src/Analysis/Engine/Test/InheritanceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
using System.Text;
using System.Threading.Tasks;
using Microsoft.Python.LanguageServer.Implementation;
using Microsoft.PythonTools.Analysis;
using Microsoft.PythonTools.Analysis.FluentAssertions;
using Microsoft.PythonTools.Interpreter;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using TestUtilities;

namespace Microsoft.PythonTools.Analysis {
namespace AnalysisTests {
[TestClass]
public class InheritanceTests {
public TestContext TestContext { get; set; }
Expand Down Expand Up @@ -38,5 +39,30 @@ def virt():
analysis.Should().HaveVariable("b").OfType(BuiltinTypeId.Int);
}
}

[TestMethod]
public async Task AbstractPropertyReturnTypeIgnored() {
var code = @"
import abc

class A:
@abc.abstractproperty
def virt():
pass

class B(A):
@property
def virt():
return 42

a = A()
b = a.virt";

using (var server = await new Server().InitializeAsync(PythonVersions.Required_Python36X)) {
var analysis = await server.OpenDefaultDocumentAndGetAnalysisAsync(code);

analysis.Should().HaveVariable("b").OfType(BuiltinTypeId.Int);
}
}
}
}
2 changes: 1 addition & 1 deletion src/Analysis/Engine/Test/Microsoft.Python.Analysis.Engine.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project>
<PropertyGroup>
<TargetFramework>netcoreapp2.1</TargetFramework>
<RootNamespace>Microsoft.PythonTools.Analysis</RootNamespace>
<RootNamespace>AnalysisTests</RootNamespace>
Copy link
Contributor

@AlexanderSher AlexanderSher Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not change namespaces for now.

Copy link
Contributor Author

@lostmsu lostmsu Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only affects what namespace VS uses when you create a new file. It was set to AnalysisTests in PTVS but, I guess, was overlooked when porting, which in turn accidentally caused my new file InheritanceTests to get wrong namespace.

Copy link
Contributor Author

@lostmsu lostmsu Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, I think this change should be kept until you guys decide to straighten this up.

<AssemblyName>Microsoft.Python.Analysis.Engine.Tests</AssemblyName>
</PropertyGroup>
<PropertyGroup>
Expand Down