Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile-time detection for calling unsupported virtual method override #137

Open
arnetheduck opened this issue Jan 16, 2025 · 6 comments
Open
Labels

Comments

@arnetheduck
Copy link
Contributor

  • QAbstractListModel inherits from QAbstractItemModel
  • creating a QAbstractListModel instantiates a MiqtVirtualQAbstractListModel
  • QAbstractItemModel contains https://doc.qt.io/qt-6/qabstractitemmodel.html#columnCount as a pure virtual function
  • the generated bindings for QAbstractListModel do not contain an override function for columnCount
  • Instead, QAbstractListModel_override_virtual_columnCount is calledvia inheritance which fails its dynamic_cast<MiqtVirtualQAbstractItemModel> (because we created MiqtVirtualQAbstractListModel).
@mappu
Copy link
Owner

mappu commented Jan 17, 2025

There is an override function for QAbstractListModel::columnCount - https://github.com/mappu/miqt/blob/master/qt6/gen_qabstractitemmodel.cpp#L547

There is also a working example for this Qt class, in the examples folder. Can you repro this on miqt master?

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Jan 17, 2025

What you linked actually is part of MiqtVirtualQAbstractItemModel and as a consequence calls miqt_exec_callback_QAbstractItemModel_ColumnCount - notice Item vs List (basically QAbstractListModel_override_virtual_ColumnCount is nowhere to be found)

@arnetheduck
Copy link
Contributor Author

Ok, I think I understand this one now - columnCount has a private override in QAbstractListModel - so when you're using C++ it's "off limits" if you have a QAbstractListModel pointer.

But there is nothing in the exported API that prevents it from being called through the inheritance structure, and this is where the failure happens.

I think this happens in the go bindings as well (don't know go enough), but to fix and "block" onColumnCount for the QAbstractListModel, the IL would have to retain information about private method overrides.

Normally, users wouldn't try to override this way, it doesn't make much sense, but this happened in a generic override section of nimqml that ideally would have seen a compile-time error.

@mappu
Copy link
Owner

mappu commented Jan 18, 2025

Right, that makes sense - columnCount is virtual public/protected in the base QAbstractItemModel, and then the derived QAbstractListModel changes it to private.

The Miqt IL does track this, and only generates an OnColumnCount function for the base QAbstractItemModel, there is no generated QAbstractListModel.OnColumnCount function. You can see a comment about this in intermediate.go: https://github.com/mappu/miqt/blob/master/cmd/genbindings/intermediate.go#L477

In the Go projection, the base struct is embedded in the derived struct, allowing you to seamlessly call methods on the base type. However, only the directly constructed type has the isSubclass flag set. When calling QAbstractListModel.OnColumnCount, Go transparently calls (QAbstractListModel).(*QAbstractItemModel).OnColumnCount, but the interior QAbstractItemModel has isSubclass=false inside its Go struct, which prevents you from calling the method and segfaulting.

The change in #138 moves this defense into the C ABI, which can help for other language projections. This will detect the dynamic_cast failure and, instead of dereferencing a null pointer, it can return false to the language projection, which can then produce an error message at runtime. But this has no effect for the Go projection other than saving 1x bool.


Another way to fix this would be to prevent downcasting, but rather, expose language projection methods for all of the public methods on the base class. But, this causes problems if some other function wants to take a base class pointer.

Another way to fix this would be to distinguish QFoo and MiqtVirtualQFoo in the ABI, and only allow the override methods if you have a strictly typed subclass pointer. This would also allow compiling with -fno-rtti for smaller binary sizes. But that's a really massive change.

@mappu mappu removed the bug Something isn't working label Jan 18, 2025
@arnetheduck
Copy link
Contributor Author

there is no generated QAbstractListModel.OnColumnCount function.

What I'd be looking to generate is a "deleted" function that would catch the attempt at compile time as long as you make the attempt through a QAbstractListModel (ie it would still be possible to fail at runtime if via the base class, but as long as you use the derived List class, the function call would simply not be available to you.

One could almost achieve this with PrivateMethods, as long as there's no overloading (since there's only the name to go by).

@mappu
Copy link
Owner

mappu commented Jan 20, 2025

What I'd be looking to generate is a "deleted" function that would catch the attempt at compile time

That would be fantastic.

I can see how it would be possible in C++, but I think Go is not expressive enough to do this (maybe with some go:linkname tricks?).

@mappu mappu changed the title Missing overrides for virtual functions causes dynamic cast failure Compile-time detection for calling unsupported virtual method override Jan 20, 2025
@mappu mappu added the wishlist label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants