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

Improve handling of circular and imported class bases #1497

Merged
merged 13 commits into from
Sep 3, 2019
Merged

Conversation

MikhailArkhipov
Copy link

Fixes #1495

  • Since we don't handle reassignments, improve handling of imported bases like
from Base import A, B

class A(A, B):
    def methodA(self, x):
        return x

class C(B): ...

class B(A):
    def methodB(self, x):
        return x

by declaring imported variables in regular collection as well as in special imported collection so they can be found when we need to resolve class base.

  • Decide on declaration or import depending on location
  • Implement deep filtering of circular base chains
  • Tests

Copy link
Contributor

@CTrando CTrando left a comment

Choose a reason for hiding this comment

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

if all the tests pass then looks fine, tests should handle majority of cases. Make sure to start VS Code as well just in case the multithreading issues come back

@@ -47,14 +47,23 @@ public void DeclareVariable(string name, IMember value, VariableSource source, N
=> DeclareVariable(name, value, source, GetLocationOfName(location), overwrite);

public void DeclareVariable(string name, IMember value, VariableSource source, Location location, bool overwrite = true) {
if (source == VariableSource.Import) {
// Duplicate declaration so if variable gets overwritten it can still be retrieved. Consider:
// from X import A
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe update the comment now that it loses its old context:
Store declaration source so in case if variable gets overwritten...

@@ -60,23 +61,19 @@ internal partial class PythonClassType {
/// </summary>
public IPythonType CreateSpecificType(IArgumentSet args) {
lock (_genericParameterLock) {
var genericTypeParameters = GetTypeParameters();
var newGenericTypeParameters = GetTypeParameters();
Copy link
Contributor

Choose a reason for hiding this comment

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

newGenericTypeParameters is not entirely accurate because GetTypeParameters just gets the parameters that need to be filled in for this class

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

LGTM, passes and VS Code works on my test projects, Cameron's command/variable naming feedback notwithstanding.

@MikhailArkhipov MikhailArkhipov merged commit 51ccb26 into master Sep 3, 2019
@MikhailArkhipov MikhailArkhipov deleted the 1495 branch September 3, 2019 17:55
jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
* Add test for GPy (fails)

* Disambiguate bases

* Handle base loops better

* GPy test (on ignore)

* PT feedback + test

* PR feedback

* Pass template class

* Add declaration position extension

* Pass template class

* Change eval to scope

* Add test for scoped import

* Adjust comparison
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.

Stack overflow when importing GPy
4 participants