analysis of optional attributes in >= v0.0.30

[mlin]

In this example,

from typing import Optional

class A:
    x: Optional[int]

    def __init__(self) -> None:
        self.x = 42

a = A()
if a.x is not None:
    assert isinstance(a.x, int)
    print(a.x + 1)

pyre v0.0.30 newly errors,

pyre_optional.py:12:10 Undefined attribute [16]: `Optional` has no attribute `__add__`.

In previous versions of pyre, either the if branch, or the assertion would satisfy pyre to disregard the attribute's optionality at that particular point. If I replace the references to a.x with an auxiliary variable a_x = a.x then it works as expected; but that seems pretty ugly/onerous. Is there an intentional change here I'm not grokking? Thanks!

[SamChou19815]

In between the assert and the use of a.x, there can be some code that sets a.x to be None again. Precisely deciding whether to invalidate the refinement will make the analysis non-local. We found that we didn't consistently enforce this design decision in previous releases, and this was corrected in the latest release.

[mlin]

Should one now use an intermediate variable every time for this pattern? Seems like inconveniencing a really common case to catch a rare one..

The following two variations seem to produce no error:

from typing import Optional

class A:
    x: Optional[int]

    def __init__(self) -> None:
        self.x = 42

a = A()
a_x: Optional[int] = a.x
if a_x is not None:
    assert isinstance(a_x, int)
    print(a_x + 1)

from typing import Optional

class A:
    x: Optional[int]

    def __init__(self) -> None:
        self.x = 42

def main():
    a = A()
    if a.x is not None:
        assert isinstance(a.x, int)
        print(a.x + 1)

if __name__ == "__main__":
    main()

[mlin]

Hi Pyre team! I've been sticking on v0.0.28 for the last few months because it didn't seem reasonable to me to have to go through my codebase and replace many dozens of references to objects' optional attributes with an intermediate variable. I just tried v0.0.38 and it seems like we're in the same place. Is there any relief in sight for this really common case, or some advice/pattern besides assigning & checking an intermediate variable?

Also, I could see you've been busy in the repo continuously, but it's been hard to tell where the project is going as there have been few release versions and without release notes. I'm sure the community would love to know a little more about the roadmap to feel confident about using the tool.

Thanks and OCaml ftw :vulcan_salute:

[grievejia]

Hi @mlin,

Sorry for the long turnaround time!

First to address the point you raised in the previous comment: we've fixed Pyre in the latest version the two variations you provided now would both error, provided that you annotate the main() function with def main() -> None -- By default, Pyre wouldn't check the body of a function that's not annotated because we assume that such functions are untyped. Adding a # pyre-strict on top of your file would make Pyre warn on functions without annotations.

Seems like inconveniencing a really common case to catch a rare one..

I'm not sure about the rareness claim there -- it really depends on how your codebase is developed and evolved.

But I do agree that we should have probably rolled out the optional refinement enforcement in a smoother and less rushed manner. We received lots of pushbacks internally within Facebook on this as well -- Like you said, there are tons of code out there that uses the pattern and it's not ideal to suddenly force people to rewrite their code.

Currently we are working on relaxing the enforcement by not warning if both the object and the attributes are annotated as Final. The relaxation is expected to be released in one or two months. As for the current workaround, we suggest # pyre-ignore[16] -- there's a helper script we released called pyre-upgrade that can help you add error ignore comments in batch.

At this point I don't think we are going to retract this behavior: it's the right thing to do from type safety perspective after all. Though we did learn our lessons here from the bumpy release process of it and it's unlikely that we'll have another rushy feature release like this in the future.

Also, I could see you've been busy in the repo continuously, but it's been hard to tell where the project is going as there have been few release versions and without release notes.

Yes there's no doubt that we should do better job at it. We've recently added some documents on our roadmap of H1 2020 -- hopefully this would be a reasonable starting point?

Historically the Pyre team don't have too much bandwidth to support our open-source users. As the project gets more and more mature and stable, this is likely going to change in the future 😄

[mlin]

Thanks @grievejia! The Final[] annotation looks like just the right solution. I also noted @shannonzhu mentioning the upcoming walrus operator on the related https://github.com/facebook/pyre-check/issues/221#issuecomment-566176602 -- this would make the intermediate variable approach more concise. Between the two, this seems very promising indeed.

Maybe it's useful to keep this issue visible to others coming by for the time being, but I'll leave this up to you.

Thanks very much also for sharing the roadmap, really terrific to see!! Noting,

Onboard more than one Facebook open source project

In case you end up needing to move the goalpost just a little bit, the CZI project I work on has certainly benefited hugely from (even the old version) of Pyre, with the bonus of several funny "meta" situations type-checking a static type-checker =)

[aspin]

As for the current workaround, we suggest # pyre-ignore[16] -- there's a helper script we released called pyre-upgrade that can help you add error ignore comments in batch.

Does any documentation exist on this? I'm just another open-source user stuck on 0.28 since fixing this is a pretty huge dig in our codebase. Running pyre-upgrade just seems to hang for me /shrug

EDIT: nevermind, found the section in guided tour: https://pyre-check.org/docs/guided-tour.html#using-pyre-upgrade

[lucasvazq]

If someone is having trouble with some type, can use getattr for now. E.g.

def get_file(path: str) -> Optional[pathlib.Path]:
    ...

file = get_file('file.txt')

# This gives an error: Undefined attribute [X]: `Optional` has no attribute `read_bytes`
# file_bytes: bytes = file.read_bytes()

# This doesn't
file_bytes: bytes = getattr(file, 'read_bytes', b'')