Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more tests for simple augmented assignments #1612

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tristanlatr
Copy link
Contributor

Description

This PR adds a test for with two augmented assignments, I believe there is a flaw in the logic, so creating a failing test for it.

Type of Changes

Type
Adds a test

Related Issue

#192

@coveralls
Copy link

coveralls commented Jun 10, 2022

Pull Request Test Coverage Report for Build 2477304180

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 92.233%

Totals Coverage Status
Change from base Build 2468320632: 0.01%
Covered Lines: 9358
Relevant Lines: 10146

💛 - Coveralls

@tristanlatr
Copy link
Contributor Author

tristanlatr commented Jun 10, 2022

Weird, the test does not pass locally with astroid-2.11.5.

Did you fixed #192 without closing the issue recently ?

@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Jun 10, 2022
@Pierre-Sassoulas
Copy link
Member

Thank you for the regression test and the issue triaging 👍 It's entirely possible that some issues are not up to date and should be closed. Would you mind adding the example from the issue with lists maybe it was the problem and not integer ?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Jun 10, 2022
@tristanlatr
Copy link
Contributor Author

Would you mind adding the example from the issue with lists maybe it was the problem and not integer ?

The exact same test case fails locally. I'll investigate more.

@tristanlatr tristanlatr changed the title Add a second test for simple augmented assignments Add more tests for simple augmented assignments Jun 10, 2022
@tristanlatr
Copy link
Contributor Author

tristanlatr commented Jun 10, 2022

Sorry for all of this, I was just confused. I'm hacking hard and did not see an obvious issue in my test case.

Astroid is working properly ^^ :)

The good thing is that you can close #192 and you have more tests ;)

@tristanlatr
Copy link
Contributor Author

tristanlatr commented Jun 10, 2022

Nevertheless, I believe the getattr() method does not handle augmented assignments very well. I'll try to provide some more test cases for this.

@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as draft June 10, 2022 20:56
@tristanlatr
Copy link
Contributor Author

So here's the weird behaviour:

    def test_except_assign_after_block_overwritten_getattr(self) -> None:
        """When a variable is assigned in an except clause, it is not returned
        when it is reassigned and used after the except block.
        """
        code = """
            try:
                1 / 0
            except ZeroDivisionError:
                x = 10
            except NameError:
                x = 100
            x = 1000
            print(x)
        """
        astroid = builder.parse(code)
        stmts = astroid.getattr('x')
>       self.assertEqual(len(stmts), 1)
E       AssertionError: 3 != 1

Shouldn't it return only the last assignment?

@tristanlatr tristanlatr marked this pull request as ready for review June 11, 2022 19:08
@tristanlatr
Copy link
Contributor Author

Or is it me that is missing something ?

‘getattr’ seems to return statements that should get flittered-out, because they’re overridden in the main flow.

Tell me what you think.

@DanielNoord
Copy link
Collaborator

I believe this works like intended:

cat test.py
from astroid import builder, extract_node

code = """
    try:
        1 / 0
    except ZeroDivisionError:
        x = 10
    except NameError:
        x = 100
    x = 1000
    x
"""
astroid = builder.parse(code)
stmts = astroid.getattr("x")

print(stmts)
node = extract_node(code)
print(list(node.infer()))python test.py
[<AssignName.x l.5 at 0x1021e8090>, <AssignName.x l.7 at 0x102289650>, <AssignName.x l.8 at 0x1022b6d10>]
[<Const.int l.8 at 0x1022b5d90>]

getattr returns all assignments in a module, whereas inferring the value at the end of the flow correctly infers what that value should be.

@tristanlatr
Copy link
Contributor Author

Hi @DanielNoord,

Thanks for the explanation, I have a follow-up question:

getattr returns all assignments in a module

If I'm not mistaken, getattr is used to infer Attribute instances.
So in the context of the following code:

class C:
    try:
        1 / 0
    except ZeroDivisionError:
        x = 10
    except NameError:
        x = 100
    x = 1000

C.x

C.x wouldn't resolve to a single node, which seems odd (I've added a test for that).

Tell me what you think,

@DanielNoord
Copy link
Collaborator

Yeah, that does seem like a bug.

This is somewhat related to what I describe in pylint-dev/pylint#5264 (comment). Basically attribute and method resolvement on ClassDefs are not able to consider control-flows. Perhaps a solution can fix both at the same time?

@tristanlatr
Copy link
Contributor Author

I believe it's indeed the same root cause.

I've initially discovered this issue because I'm building work based on astroid that keeps the compatibility with standard library nodes: https://github.com/tristanlatr/astuce, and by replicating the logic, I also replicated the bug.

And I fixed it by adding a sentinel node at the end of every frame nodes, and using it to do the lookups in the context of the frame.

Adding sentinel: https://github.com/tristanlatr/astuce/blob/7704c0be5b36993e64d0981d0c9634d74a6f84c0/astuce/parser.py#L112
Using it in getattr: https://github.com/tristanlatr/astuce/blob/7704c0be5b36993e64d0981d0c9634d74a6f84c0/astuce/inference.py#L204

It's probably not the best way to go though!

Tell me what you think.

@DanielNoord
Copy link
Collaborator

I think the issue should probably be resolved by taking all/the last definition into consideration. For __get__ on ClassDef's the last definition should probably be enough while getattr should probably consider all assignments. That would require some control flow knowledge in some cases, but would avoid returning incorrect values.

@DanielNoord DanielNoord removed this from the 2.12.0 milestone Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining astroid or the dev workflow Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants