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
merged 3 commits into from
Sep 26, 2018

Conversation

lostmsu
Copy link
Contributor

@lostmsu lostmsu commented Sep 25, 2018

This adds a test for the analysis of return values of abstract properties (properties, marked with @abc.abstractproperty).

This also adds a partial fix for it not being analyzed properly. (related to #115)

Unfortunately, due to some regression (?) between PTVS/master and python-language-server/master, this does not seem to be enough: test variable b analysis results in two possible return values, int (correct) and A.virt (which is incorrect, and eventually resolves to be Unknown). The later seems to be happening because on the first pass ddg._eval.Evaluate(@abc.abstractproperty) results in empty set.

@lostmsu
Copy link
Contributor Author

lostmsu commented Sep 25, 2018

Speaking of the later: what is the reasoning behind module's members being analyzed before overall module is? I'd expect import statements in a module to be analyzed before any class definitions in it.

@@ -1,7 +1,7 @@
<Project>
<PropertyGroup>
<TargetFramework>netcoreapp2.1</TargetFramework>
<RootNamespace>Microsoft.PythonTools.Analysis</RootNamespace>
<RootNamespace>AnalysisTests</RootNamespace>
Copy link
Contributor

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

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

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.

@@ -217,6 +217,15 @@ abstract class SpecializedNamespace : AnalysisValue {
}
}

public override string Name {
get {
Copy link
Contributor

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;

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

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

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

Choose a reason for hiding this comment

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

Changed in 5fc8652

@MikhailArkhipov MikhailArkhipov merged commit 22944ac into microsoft:master Sep 26, 2018
jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
A test to ensure @abc.abstractproperty return value is analyzed correctly
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants