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

No errors/warnings are thrown when using wrong attributes. #965

Open
dphaldes opened this issue May 30, 2024 · 5 comments
Open

No errors/warnings are thrown when using wrong attributes. #965

dphaldes opened this issue May 30, 2024 · 5 comments
Labels
🪲 bug Something isn't working 📖 documentation Improvements or additions to documentation 🙋 good first issue Good for newcomers

Comments

@dphaldes
Copy link
Contributor

When using a wrong attribute macro inside an extern block, there are no errors or warning. It would help a lot to have some sort of compile time sanitation to avoid user error.

Context: I spent a while to figure out why my properties were not exposed to QML when the error was using #[qml_property] instead of #[qproperty]

@ahayzen-kdab
Copy link
Collaborator

This is likely linked to our qobject tagged type item, we likely aren't passing through attributes like we do for other places like methods ? But we should also check that the same is working for methods too etc.

@ahayzen-kdab ahayzen-kdab added 🪲 bug Something isn't working 📖 documentation Improvements or additions to documentation 🙋 good first issue Good for newcomers labels May 30, 2024
@LeonMatthesKDAB
Copy link
Collaborator

@ahayzen-kdab This is a problem I've not yet found a very clean way to handle in our parser/generator.

For most of the types we parse, there's a predefined list of attributes that we allow, like cxx_name, rust_name, namespace, base etc.
So we'll probably want to add checks that only the expected attributes are added, and nothing more.

Maybe we should just list all allowed properties every time we parse and check that no other exist?
It's somewhat of a cross-cutting concern though, as then that check would need to know every possible attribute that e.g. the naming later uses, which is not ideal...
Something for a "checks" phase maybe?

@ahayzen-kdab
Copy link
Collaborator

ahayzen-kdab commented Jun 5, 2024

@LeonMatthesKDAB right sounds like something for the checks phase. However for other things i thought was pass through unknown attributes?

eg #[my_attribute] here on a signal is passed through

unsafe extern "RustQt" {
#[my_attribute]
#[qsignal]
fn ready(self: Pin<&mut SecondObject>);

As we can see in the test outputs

unsafe extern "C++" {
#[my_attribute]
#[cxx_name = "ready"]
fn ready(self: Pin<&mut SecondObject>);
}

I assume we are doing this for all methods ? (invokables, signals, overrrides etc). But we should also do it for attributes on the type T which seems to be missing at the moment eg

    unsafe extern "C++Qt" {
        #[my_attribute]
        #[qobject]
        type QPushButton;
    }

    extern "RustQt" {
        #[my_attribute]
        #[qobject]
        type MyObject = super::MyObjectRust;
    }

should probably generate the following?

    unsafe extern "C++" {
        #[my_attribute]
        type QPushButton;
    }

    unsafe extern "C++" {
        #[my_attribute]
        type MyObject;
    }
    extern "Rust" {
        type MyObjectRust;
    }

If they did this then at least any unknown attributes would fail to compile in Rust rather than silently failing ?

As part of your refactoring we should have a map available of the original attributes for a token so then we should be able to tag the original attributes with some filters of known attributes?

@LeonMatthesKDAB
Copy link
Collaborator

Well that's really the question, whether we want to pass anything unknown through to CXX, or whether we throw an error...

Not so sure about that, but either way we need to figure out which attributes we've computed ourselves and which remain to be passed through/errored on.

@LeonMatthesKDAB
Copy link
Collaborator

@BenFordTytherington That should be done as of #1063 , right? Can you confirm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪲 bug Something isn't working 📖 documentation Improvements or additions to documentation 🙋 good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants