"AttributeError: attribute 'arguments' of 'FuncDef' undefined" when running mypy twice

[fperrin]

Removed references to #11899 since Ariel no longer sees the issue with mypy master. Presumably I hit a different issue with similar symptoms.

Description

Let's have classes a.Foo, and b.Bar inheriting from Foo. When Bar.frobnicate is incompatible with super method Foo.frobnicate, and that method Foo.frobnicate was defined in another module, and mypy knows of Foo.frobnicate method from deserializing the cache, then the arguments field of the FuncDef object is not defined.

If that's not clear enough, cherry-pick the first commit and run pytest -k testIncompatibleOverrideFromCachedModuleIncremental. The second iteration hits the issue I'm describing above.

Looking at FuncItem / FuncDef in mypy/nodes.py, we can see:

class FuncItem(FuncBase):
    """Base class for nodes usable as overloaded function items."""

    __slots__ = ('arguments',  # Note that can be None if deserialized (type is a lie!)
[ ... ]
    def __init__(self,
                 arguments: List[Argument],
[ ... ]
class FuncDef(FuncItem, SymbolNode, Statement):
[ ... ]
    def serialize(self) -> JsonDict:
        # We're deliberating omitting arguments and storing only arg_names and
        # arg_kinds for space-saving reasons (arguments is not used in later
        # stages of mypy).
        # TODO: After a FuncDef is deserialized, the only time we use `arg_names`
        # and `arg_kinds` is when `type` is None and we need to infer a type. Can
        # we store the inferred type ahead of time?
[ ... ]
    def deserialize(cls, data: JsonDict) -> 'FuncDef':
[ ... ]
        # Leave these uninitialized so that future uses will trigger an error
        del ret.arguments

The comment stating "type is a lie!" is itself a lie; arguments is not None (or the empty list), it is in fact not defined at all.

My approach is to rebuild FuncDef.arguments from arg_names/arg_kinds, defaulting argument names to "".

Test Plan

Added a test case triggering the issue.

Cheers,
Fred.

[fperrin]

Hi @arielf ; when you opened the issue you commented "Unfortunately, updating from git is not an option in my env.". Has that changed, and are you able to test my change in your environment?

[arielf]

Hi Fred,

Thank you for your work on this!

It seems like the diffs above are vs an older version than the one I'm using.

I'm now using mypy 0.931
When I opened the issue I was on mypy 0.920

0.931 has solved multiple INTERNAL_ERROR instances (on existing .mypy cache) but not all.
I recently hit another instance (with mypy 0.931) that forces me to rm -rf .mypy_cache to avoid the mypy INTERNAL_ERROR

Anyway, sorry, it seems that I can't cleanly apply your changes cleanly/simply to the latest mypy 0.931 source.
But I'll be happy to test/verify any diff vs 0.931.
Thanks again.

Later edit:

I followed the mypy.readthedocs.io instructions to install the latest dev mypy from git:

% git clone https://github.com/python/mypy.git
...
% cd mypy
% python3.8 -m pip install --user --upgrade .
...
      Successfully uninstalled mypy-0.931
Successfully installed mypy-0.950+dev.504779b8511b36880df13476b4fbb35a6625743f

And I can't make it crash on a second run (with a pre-existing .mypy_cache).
I suppose this includes your recent changes.
I guess this is good news!

[fperrin]

Hi @arielf ,

It seems like the diffs above are vs an older version than the one I'm using.

I'm working directly on top of git master. It's entirely possible the diffs don't apply to either 0.930 or 0.950.

I followed the mypy.readthedocs.io instructions to install the latest dev mypy from git ... And I can't make it crash on a second run (with a pre-existing .mypy_cache).

Well I can (using git master), so there is still an issue somewhere :)

In any case, as the bot points out, I've broken something, so ignore this PR for now. I think I understand the issue a bit better, and if you leave me until tonight I should have a better fix. I was a bit too quick in running only some of the self-tests.

[fperrin]

Running this properly in private branch seems to work better: fperrin#2

[jhance]

It seems dangerous, potentially leading to incorrect behavior instead of crash.

[JukkaL]

Yeah, a crash is better than incorrect behavior. This can also slow down incremental runs, since we deserialization will construct many additional objects that are almost never used for anything.

I think that a better fix would be to change the type of arguments to an optional type, and change all code that accesses it to use assert is not None (if it's known to exist in that particular context) or use the function type instead.

[fperrin]

Hi @JukkaL ,

Based on yours and @jhance 's comments, I've made FuncItem.arguments: Optional[List[Argument]]; set ret.arguments=None here in deserialize; and updated all usages to do a assert X.arguments is not None (or replace an hasattr check, etc.).

Is that the right approach?

[fperrin]

Hi @JukkaL & @jhance ;

Can I prode you to have a second look at the latest version of that PR?

[fperrin]

Hi @JukkaL & @jhance,
Can I prode you to have a second look at the latest version of that PR?

I just rebased on top of master. On master as of today, the testcase testIncompatibleOverrideFromCachedModuleIncremental still fails. The code changes applied with no conflict or even fuzzing, so the issue hasn't been silently fixed in the meantime.

Hi @msullivan , you have worked in the area of code I'm changing here (most recently in d180456), do you have an opinion regarding those changes?

[fperrin]

Rebased again. After 7bd6fdd326, which inits FuncDef.arguments to the empty list, the changes are much simpler than before: instead of making arguments an optional type, and many checks for is not None, only a couple of places need to check whether arguments is the empty list.

[emmatyping]

I think this looks pretty good, nice that Jukka's changes made the diff cleaner :)

[hauntsaninja]

Thanks for fixing this and keeping it up to date with master, just one nit!

[PeterJCLaw]

Thanks for putting this together @fperrin 🎉 I'd hit this myself, so grateful for the fix. (I've confirmed this fixes the issue on my largeish codebase).

@hauntsaninja is there anything that needs to happen before this can merged? (Anything I can help with?)

[fperrin]

Hi @jhance , @hauntsaninja , @JukkaL ,

Can I prod you again regarding this issue? It seems that I'm not the only one hitting this issue, and at least Peter confirmed this PR fixes his issue. I just check that applying this PR to lastest master still passes CI: fperrin#6.

If the current approach (which boils down to do: 1/ set FuncDef.arguments to the empty list on unserialize and 2/ check whether arguments is the empty list before using it) is not the best one, I'm happy to rework the PR.

[hauntsaninja]

Thanks for the PR and for the reminder! In the meantime, Alex also confirmed that this fixed an issue typeshed was running into python/typeshed#7977 (comment)

I'm a little concerned here about the potential for silent incorrect behaviour, since an empty argument list would now both be a valid value and a sentinel used for the weird deserialisation logic (unless I'm misunderstanding). That is, we're going against the defensive intention of # Leave these uninitialized so that future uses will trigger an error

Wouldn't it be easier to just change the condition in mypy/messages.py to check hasattr(tp.definition, "arguments")?

I'd also accept a PR that actually does things the type safe way and makes arguments actually of type Optional[List[Argument]] where None represents the case where we don't know arguments for whatever deserialisation reason.

[fperrin]

Hi @hauntsaninjaMaking `arguments` an optional list requires adding `assert xxx.arguments is not None` in many locations. fperrin@370a035 On further reflection, doing that VS adding the hasattr check means we still have a runtime error, just a AssertError instead of AttributeError. After 7bd6fdd326 which does `self.arguments = arguments or []`, using the empty list as a guard value gave a much shorter diff. I didn't consider functions with empty argument list that could still get type-checked. From yoyr comment the simplest option of adding a hasattr check is your preferred? Ill do that on Monday, I'm offline over the weekend.Thank for your input!

[hauntsaninja]

Yeah, I think the simple hasattr option here is easiest for me to see correctness of and be confident that it won't result in silent incorrect behaviour somehow. No rush, and thanks again for working on this! :-)

[fperrin]

Update the PR as decided last Friday.

@AlexWaygood and @PeterJCLaw , could I ask you to test again if your issue is still fixed please?

[AlexWaygood]

Update the PR as decided last Friday.

@AlexWaygood and @PeterJCLaw , could I ask you to test again if your issue is still fixed please?

I can confirm that if I unskip testing Flask-SQLAlchemy in typeshed's mypy_test.py:

• the test still crashes using mypy master if I run python tests/mypy_test.py -p3.11 twice in succession
• but this PR still fixes it 😊

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[PeterJCLaw]

Yup, can confirm that this still works on my codebase with the updated fix (and master still errors).

[hauntsaninja]

Awesome, thank you so much!!

[fperrin]

Thank you @hauntsaninja for guiding me to commit the fix, and @PeterJCLaw & @AlexWaygood for the second check!