Fix is_name_used_as_error returning false for unused external errors
#2641
Conversation
| @@ -1021,6 +1021,10 @@ impl ComponentInterface { | |||
|
|
|||
| pub fn is_name_used_as_error(&self, name: &str) -> bool { | |||
| self.errors.contains(name) | |||
There was a problem hiding this comment.
Thanks for the review! That part is still needed (as all_component_interfaces is set only in library mode) and removing it breaks some tests; would setting all_component_interfaces in non-library mode be better?
There was a problem hiding this comment.
right 😅 "library mode" has always seemed strange to me. What does "non library mode" mean with external types? Non library mode having only a clone of itself in all_component_interfaces seems fine too, but not worth it here i guess?
(part of the confusion is actually around udl generating rust scaffolding - that's "not library mode" - but also doesn't need all_component_interfaces. In terms of bindings, "not library mode" shouldn't be a thing)
There was a problem hiding this comment.
I haven't had a deep look at the UDL parsing part (as I'm primarily using library mode), but if you find it awkward that the library mode cloning CIs to populate all_component_interfaces, how about removing the grouping logic in library_mode::find_components? In UDL files, external types to be used are all declared again, and it seems the UDL parsing logic doesn't re-group the metadata at all...
There was a problem hiding this comment.
UDL parsing is done for both scaffolding and bindings - ie, is used by library mode too. Further, individual crates can use both udl and macros - so I don't think we can just remove the grouping in that way.
| #[derive(uniffi::Error)] | ||
| pub enum ContainsExternalError { | ||
| ExternalError(NotToThrowError), | ||
| } | ||
|
|
There was a problem hiding this comment.
@mhammond This is the case that @dani-garcia mentioned; should I add cases like where NotToThrowError is used as parameters or record properties for better test coverage?
There was a problem hiding this comment.
@mhammond This is the case that @dani-garcia mentioned;
sorry, I've lost some context - mentioned where?
should I add cases like where
NotToThrowErroris used as parameters or record properties for better test coverage?
I guess that can't hurt, but probably isn't strictly necessary - just a test which fails without your patch but works with it seems fine to me.
Fixes #2636 by making
ComponentInterface::is_name_used_as_errorreturntruefor unused external errors usingall_component_interfacesintroduced in #2612.self.errors.contains(name) ||is still required for cases where library mode is not used, which is the reason why 4224423 has failed.Why #2636 occurs
self.error.inserthappens when the error enum itself is defined in the crate, or it is thrown from a function or a constructor. Since external errors are not defined in the crate, they are not registered throughself.error.insertwhen it is not used. The check to determine whether an enum is an error requires anEnumMetadata, butEnumMetadatafor external errors used in a variant of an enum is not being provided to CI.