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

Errors from retrieving model arguments are swallowed #1568

Closed
hannobraun opened this issue Feb 7, 2023 · 3 comments
Closed

Errors from retrieving model arguments are swallowed #1568

hannobraun opened this issue Feb 7, 2023 · 3 comments
Labels
good first issue Good for newcomers type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

The Fornjot application communicates with the models it loads over an FFI boundary (with Rust code on both sides). This is necessary, because models are compiled to dynamic libraries, and Rust doesn't have a stable ABI. Panics in model code must not cross this FFI boundary, as that would be undefined behavior. Therefore, panics in model code are caught and turned into errors, which are then displayed in the Fornjot application.

There is one hole in the code that is handling this. If a panic occurs in a model's argument handling code (which is generated by fj-proc), it is converted into an error, but then later silently ignored:
https://github.com/hannobraun/Fornjot/blob/ed08c4c8d99d3e146a9a815406e02e276a040d77/crates/fj/src/abi/context.rs#L73

This is not ideal, as that could hide bugs in a highly confusing manner. Instead, the method this is happening in (Context::get_argument) should return a Result instead of an Option and pass that error on. Any downstream code should be adapted as necessary.

Labeling as https://github.com/hannobraun/Fornjot/labels/good%20first%20issue, since this is a pretty localized problem.

@hannobraun hannobraun added type: bug Something isn't working good first issue Good for newcomers topic: host labels Feb 7, 2023
@hannobraun hannobraun changed the title Errors from retrieving model arguements are swallowed Errors from retrieving model arguments are swallowed Feb 7, 2023
@emcloughlin
Copy link

I think I will try to tackle this as a first issue. Wish me luck lol

@hannobraun
Copy link
Owner Author

Sounds great! Looking forward to what you come up with!

@hannobraun
Copy link
Owner Author

This issue is no longer applicable. The affected code has been removed. See A New Direction for context.

@hannobraun hannobraun closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants