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

cxx-qt: don't use unwrap and convert to a compile error instead #455

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

ahayzen-kdab
Copy link
Collaborator

This improve error messages when our cxx-qt-gen code breaks.

Eg I have added a duplicate attribute key-value in the code, before we used to get this error when compiling.

error: custom attribute panicked
  --> examples/cargo_without_cmake/src/cxxqt_object.rs:11:1
   |
11 | #[cxx_qt::bridge]
   | ^^^^^^^^^^^^^^^^^
   |
   = help: message: called `Result::unwrap()` on an `Err` value: Error("Duplicate keys in the attributes")

Now we have a much better error that points to the code block that is at fault.

error: Duplicate keys in the attributes
  --> examples/cargo_without_cmake/src/cxxqt_object.rs:23:5
   |
23 |     #[cxx_qt::qobject(qml_uri = "com.kdab.cxx_qt.demo", qml_version = "1.0", qml_version = "1.0")]
   |     ^

Related to #38

This improve error messages when our cxx-qt-gen code breaks.

Eg I have added a duplicate attribute key-value in the code,
before we used to get this error when compiling.

```
error: custom attribute panicked
  --> examples/cargo_without_cmake/src/cxxqt_object.rs:11:1
   |
11 | #[cxx_qt::bridge]
   | ^^^^^^^^^^^^^^^^^
   |
   = help: message: called `Result::unwrap()` on an `Err` value: Error("Duplicate keys in the attributes")
```

Now we have a much better error that points to the code block that is at fault.

```
error: Duplicate keys in the attributes
  --> examples/cargo_without_cmake/src/cxxqt_object.rs:23:5
   |
23 |     #[cxx_qt::qobject(qml_uri = "com.kdab.cxx_qt.demo", qml_version = "1.0", qml_version = "1.0")]
   |     ^
```

Related to KDAB#38
@ahayzen-kdab
Copy link
Collaborator Author

This doesn't seem to make the IDE pick up the error yet, but is already an improvement, I'll read more about the IDE side of things.

@Be-ing Be-ing merged commit 1fed0bb into KDAB:main Feb 28, 2023
@LeonMatthesKDAB
Copy link
Collaborator

Regarding IDE support:
I'm pretty sure this is actually not caused by issues in the macro expansion, but by how our build script works.
Currently, if the build script fails to parse the macro code, it panics.
This seems right at first glance, but this is actually what's causing the issue (at least in my setup with rust_analyzer + nvim).
If the build script panics, rust_analyzer won't even try to expand our proc_macro, as the appropriate build setup isn't working.

So the right way to fix this would be to silently ignore any parsing errors in the build script, and maybe emit a warning.
Then the same parsing would still fail in the expansion of the procedural macro, which rust_analyzer can deal with and nicely highlight in your IDE of choice 🥳
We do need to be careful though to invoke the same code paths from the build script then to make sure it's not possible to have a parsing error in the build script, but not in the proc_macro itself.

You can already see this working when there's an error in the Rust generation.
In this case the right span will be highlighted in the editor.
In such cases the build script won't panic as it only generates the C++ code.

@ahayzen-kdab
Copy link
Collaborator Author

Regarding IDE support: I'm pretty sure this is actually not caused by issues in the macro expansion, but by how our build script works. Currently, if the build script fails to parse the macro code, it panics. This seems right at first glance, but this is actually what's causing the issue (at least in my setup with rust_analyzer + nvim). If the build script panics, rust_analyzer won't even try to expand our proc_macro, as the appropriate build setup isn't working.

So the right way to fix this would be to silently ignore any parsing errors in the build script, and maybe emit a warning. Then the same parsing would still fail in the expansion of the procedural macro, which rust_analyzer can deal with and nicely highlight in your IDE of choice partying_face We do need to be careful though to invoke the same code paths from the build script then to make sure it's not possible to have a parsing error in the build script, but not in the proc_macro itself.

You can already see this working when there's an error in the Rust generation. In this case the right span will be highlighted in the editor. In such cases the build script won't panic as it only generates the C++ code.

Yup that's the next phase of #38 is to try changing the build script not to explode when cxx_gen::generate_header_and_cc fails, however i tried switching this to simply produce an empty GeneratedCode block and then it instead explodes trying to build the C++ side. So it needs some careful investigation.

But this is i think definitely the issue with the CXX macros, as the build script fails first it never reaches the macro expansion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants