_ast: Change ast.ImportFrom.level to int | None

[iyume]

See: python/cpython#57645

[github-actions]

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

[hauntsaninja]

Thanks for the PR!

Note that level is None can only happen on artificially constructed nodes, so if it comes to it, I'd prefer errors for people doing that over false positives for everyone else (speaking as an author of recent CPython PRs in this space). But mypy_primer seems okay, so maybe it's fine.

I'm curious what your interest in this change is? Does it affect a project of yours? If so, is that project open source?

[iyume]

@hauntsaninja The idea is followed the ASDL and docs.

A similar issue python/cpython#90447 changed ASDL to disallow optional.

But ast.ImportFrom.level still allow optional...

https://github.com/python/cpython/blob/f177f6f29b069f522a0b3ba44eaae19852b6d2b0/Parser/Python.asdl#L47

https://github.com/python/cpython/blob/f177f6f29b069f522a0b3ba44eaae19852b6d2b0/Parser/Python.asdl#L78

[hauntsaninja]

Yes, I'm aware, see for instance python/cpython#92987

I was just wondering if this affected code you're using or if this was just a theoretical thing :-)

[iyume]

Yes, I'm aware, see for instance python/cpython#92987

I was just wondering if this affected code you're using or if this was just a theoretical thing :-)

Here is my code: https://github.com/nonebot/nb-autodoc/blob/3786ea2e9a39e69551264b86c2ad1b755389d181/nb_autodoc/utils.py#L64-L69

But I will change level to level or 0 to adapt the ASDL behaviour even though it has no affect...

[hauntsaninja]

Okay, thank you!

I'm -0.5 on this change, so I don't plan on hitting merge myself. I'll leave this PR to other maintainers :-)

This is basically a bug in CPython's ASDL, like python/cpython#30467 which got fixed in 3.11. Unfortunately, this particular bug got half fixed in the wrong direction (making compile more permissive) and CPython is now hesitant to remove it for backward compatibility reasons.

It is only possible to have None here with artificially constructed nodes. When opening python/cpython#92987 I grepped through a lot of real world code and couldn't find any cases where people relied on the behaviour with None when constructing nodes. And I don't like making changes that cause extra checks for things that never happen. Note that if we type AST constructors (#8378) correspondingly, in fully typed programs we can guarantee there won't be any issue.

But shouldn't we type things close to the implementation? Yes, but also the implementation never produces a None ;-) We've repeatedly chosen not to change this, e.g. in #7877. Implementations can be very flexible, especially for "plain old data" classes that can store basically anything. Plenty of functions allow basically anything when we type them as taking bools. Anyway, I recognise that this is an opinionated opinion (as opinions tend to be) and that mypy_primer currently claims no coverage of this, so 🤷

[AlexWaygood]

I'm also not a massive fan of this change. There's nothing on mypy_primer that would currently be affected by this change, but:

1. I could quite easily see myself doing something with flake8-pyi at some point that might be affected by this change.
2. We should probably add flake8-pyi to mypy_primer!

[AlexWaygood]

We have two maintainers who are both -1 on this, so I'm going to go ahead and close this for now. But thank you for the PR; sorry that we disagree on this one :)

[Akuli]

I agree that type checking should prevent doing potentially bad things that CPython allows for historical reasons.