[lldb] Configure pyright to the documented minimum python version #162952
Conversation
Pyright is an MIT-licensed static type checker and can be found at
https://github.com/microsoft/pyright
there are also various integrations to use it as an LSP server in various
editors which is the main way I use it.
It's useful on our python scripts to detect issues such as where functions
are called with unexpected types or it's possible to access obj.attr on an
object that doesn't have that attribute. It can be used without any
configuration this config setting causes it to also report issues with
type hints that do not meet our python 3.8 minimum such as this one from
dap_server.py:
```
init_commands: list[str],
```
subscripting the builtin type like that requires python 3.9 while the 3.8
equivalent is:
```
from typing import List
...
init_commands: List[str],
```
In practice these scripts still work on 3.8 because type hints aren't
normally evaluated during normal execution but since we have a minimum, we
should fully comply with it.
Note: The error pyright reports for this particular issue isn't great:
```
error: Subscript for class "list" will generate runtime exception; enclose type expression in quotes
```
This is technically correct as it is possible to evaluate type hints at
runtime but I believe anything that would do so would also evaluate the
string form as well and still hit the runtime exception. A better suggestion
in this case would have been the 3.8 compatible `List[str]`. However, it is
better than silently passing code that doesn't confirm to the minimum.
|
So as I understand this, it configures pyright if I am already using it. Rather than saying you must use it. I don't think llvm has made any recommendations of tools for this. We don't necessarily need to, given they all consume the same hints, but this minimum version setting is interesting for pyright specifically. It's probably fine to add this, but let me do a bit of research on type checking first. Last I worked on Python seriously, it wasn't a thing. In the meantime, a PR to change the 3.9 hints to the 3.8 compatible kind would be welcome. If you could get LLDB to build (verify?) cleanly with pyright set to 3.8 that'd be great. |
|
That's right, it just configures it if you want to use it
Yep, I'll try to fit that into some spare time |
|
The discussion about the minimum version will probably end up with some static analysis like pyright, so let's see what happens with that first (see https://discourse.llvm.org/t/rfc-upgrading-llvm-s-minimum-required-python-version/88605/32). Not because no upgrade == no extra checks, but because I don't want to distract from the upgrade discussion. I've been running vermin and fixing a bunch of issues I found that way. I don't know that it does much with type annotations though. So the combination of that and a type checker would be ideal. |
|
I tried out a few of these tools and I think we'll have an uphill battle trying to get it applied to the LLVM Project as a whole because:
Also what's emerging on the version update thread is different use cases for Python in the project. So I think what we could do is "suggest" tools via the settings that you're changing, but restricted to areas we know use type annotations. So if you can figure out how to apply the settings only to lldb-dap scripts and make it run without errors then we can have the lldb-dap maintainers review it. If they don't use pyright, they can add their own settings and now we've got a reference for what to use and it's a good small case study for later. And if pyright doesn't work that way, well, you'd know better than me. Hopefully you see my concerns anyway so feel free to do this another way. |
|
Sorry for the slow reply. I was going to suggest that we put a pyrightconfig.json in lldb/packages/Python but I didn't like the idea of adding lots of those files over the project and it turns out there's a better way. pyright has an 'Execution environments' feature (https://microsoft.github.io/pyright/#/configuration?id=execution-environment-options) which allows you configure minimum python versions along with search paths and other configuration options differently in different parts of the project. I've used this to limit the setting to lldb/packages/Python I've tried making a different minimum version apply to dap_server.py and that doesn't seem to be possible. pyright doesn't seem to like it when execution environments overlap (I consistently got the outer one) and it's not one of the settings that can be overridden with |
DavidSpickett
changed the title
|
I'm fine with this. It'll be unenforced, but useful for anyone who does use the tool. Since there's no enforcement upstream of this, people should note take it as an endorsement or commitment to zero errors when using it. Someone will but that's on them. Going to add a few others to review, including lldb-dap contributors. |
|
Hang on, this is still in the top level pyproject. Which I suppose is how it has to be. Hmm. Well, see what others think anyway. |
DavidSpickett
requested review from
JDevlieghere,
ashgti and
walter-erquinigo
|
I made an RFC for type checking the lldb python code in https://discourse.llvm.org/t/rfc-type-checking-to-python-code/86605 and ashgti@85739f3 was my prototype of this being integrated into the CI. Its a bit out of date, I've been side tracked by other work and haven't revisited that yet. There is a larger question of how this integrates with the rest of the python code in llvm. So you may want to check with @boomanaiden154 I think they work with the CI system more than I have. |
|
One other issue with type checking the python code is the swig generated code does not have any annotations. There is an open issue swig/swig#735 for swig generating python type annotations, but at the moment the |
I don't know if we need to think that far yet. Would people be ok accepting this as is, or prefer users of specific tools set their own settings until such time llvm endorses said tools? And like you said, making CI results useful is probably not possible yet for some code. |
This seems fine to me assuming LLDB maintainers are willing to bless this version as their official version. |
|
Do you mean version of Python or of the tool (pyright)? Presumably Python. |
|
We are aligned with LLVM, and state 3.8 on our build instructions - https://lldb.llvm.org/resources/build.html#optional-dependencies. |
Sorry. Meant pyright. I thought this was configuring a pyright version since I didn't look very closely at the patch. 😓 |
|
Understood. That would be another thing to consider usually but since we wouldn't be guaranteeing it won't have issues, implicitly being the latest version would be ok. @JDevlieghere what do you think? Ok with this as an improvement for people that use pyright, but not endorsing it as "the tool" or claiming zero issues with it? Eventually we'll do something around version/type checking where we might make such claims, but not in the short term. |
|
I'm supportive of this. This seems like a win for folks that want to use it and harmless to those who don't. I would even support going a little further and say that for LLDB, we are endorsing it as "the tool" if you opt in to type annotations. However, I'd feel more comfortable if we did that through a small RFC. Everyone who is using type annotations today is already on this thread so I don't think it would receive any pushback, but it would be the right process to make this broader decision and a prerequisite to document that policy on the website. It's not necessary for this change. |
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Go ahead and merge if you have commit access, or I can do it for you.
5 participants
Pyright is an MIT-licensed static type checker and can be found at
https://github.com/microsoft/pyright
there are also various integrations to use it as an LSP server in various editors which is the main way I use it.
It's useful on our python scripts to detect issues such as where functions are called with unexpected types or it's possible to access obj.attr on an object that doesn't have that attribute. It can be used without any configuration this config setting causes it to also report issues with type hints that do not meet our python 3.8 minimum such as this one from dap_server.py:
subscripting the builtin type like that requires python 3.9 while the 3.8 equivalent is:
In practice these scripts still work on 3.8 because type hints aren't normally evaluated during normal execution but since we have a minimum, we should fully comply with it.
Note: The error pyright reports for this particular issue isn't great:
This is technically correct as it is possible to evaluate type hints at runtime but I believe anything that would do so would also evaluate the string form as well and still hit the runtime exception. A better suggestion in this case would have been the 3.8 compatible
List[str]. However, it is better than silently passing code that doesn't confirm to the minimum.