typing: Handling of arguments that can be None

[Phil Thompson]

On 09/07/2023 00:23, Florian Bruhin wrote:
> Hey,
> 
> In Qt, all arguments are pointers, yet only rarely it's intended usage
> to pass a nullptr to functions.

...although SIP assumes it is valid by default.

> PyQt currently handles this in the type annotations by never using
> Optional[...], i.e. always disallowing None.
> 
> While this is often the right thing to do, it means that e.g.
> 
>     from PyQt6.QtWidgets import QTableView, QApplication
>     app = QApplication([])
>     tv = QTableView()
>     tv.setModel(None)
> 
> despite being intended usage. Unfortunately (like here), the Qt
> documentation never really points that out:
> 
> https://doc.qt.io/qt-6/qabstractitemview.html#setModel
> 
> However, the PyQt6-stubs project has gathered a list of known methods
> where it's intended usage to pass None for an argument:
> 
> https://github.com/python-qt-tools/PyQt6-stubs/blob/main/fixes/annotation_fixes.py#L74
> 
> They are:
> 
> 	- QLineEdit.setText(None)

This is actually a bug in the type hints for QString.

> 	- QAbstractItemView.setModel(None) and everything inheriting it
>       (QColumnView, QHeaderView, QTableView, QTreeView)
> 	- QMessageBox.aboutQt(None, ...)  # parent
> 	- QMessageBox.about(None, ...)  # parent
> 	- QProgressDialog.setCancelButton(None)
> 
> Undoubtedly there will be more yet to be discovered.
> But maybe it'd make sense to have some kind of sip annotation (or the 
> existing
> /AllowNone/?) so that it generates Optional[...] in the type hints? 
> Then this
> could probably be fixed piece by piece when it pops up.

SIP wasn't generating 'Optional' when it should - now fixed. However, as 
I said above, the default behaviour is for SIP to allow None to be used 
and passed as NULL so while the behaviour of PyQt hasn't changed, the 
user might now assume that None is a valid value to pass (because the 
help and type hints say it is allowed) even though it's a bad idea.

The solution, using the current SIP, is to use the /DisallowNone/ 
annotation wherever None is a bad idea. However if, as you suggest, this 
is the majority of cases then it might be better to add a global option 
to switch the default to disallow None and only annotate the relatively 
few cases that it is allowed.

I'll leave it as it is for the moment.

Phil