fix: codegen succeeds for error type args that reference objects #264
Conversation
afloren-palantir
changed the title
conjure-codegen/src/errors.rs
Outdated
| impl From<ErrorDefinition> for ObjectDefinition { | ||
| fn from(error: ErrorDefinition) -> Self { | ||
| ObjectDefinition::builder() | ||
| .type_name(error.error_name().clone()) | ||
| .fields(error.safe_args().iter().chain(error.unsafe_args()).cloned()) | ||
| .docs(error.docs().cloned()) | ||
| .build() | ||
| } | ||
| } |
There was a problem hiding this comment.
could be a regular helper fn if implementing from seems incorrect, I just wanted to ensure that the generated object definition was consistent between the code below and during context creation
There was a problem hiding this comment.
Yeah I think I'd just make this a function personally.
conjure-test/src/test/errors.rs
Outdated
| @@ -19,11 +19,17 @@ use crate::types::*; | |||
|
|
|||
| #[test] | |||
| fn error_serialization() { | |||
| let error = SimpleError::new("hello", 15, false); | |||
| #[allow(deprecated)] | |||
There was a problem hiding this comment.
the TestObject field foo is marked deprecated in the conjure but is also not optional, I'll just use a different test object. I only used it originally because it was the simplest test object.
There was a problem hiding this comment.
Ah gotcha. Yeah probably best to use another object.
conjure-codegen/src/errors.rs
Outdated
| @@ -18,12 +18,18 @@ use crate::context::Context; | |||
| use crate::objects; | |||
| use crate::types::{ErrorDefinition, ObjectDefinition}; | |||
|
|
|||
| impl ErrorDefinition { | |||
| pub fn object_definition(&self) -> ObjectDefinition { | |||
There was a problem hiding this comment.
Can we instead make this a free function? I try to avoid adding new impls on generated types.
There was a problem hiding this comment.
done, though lemme know if you have a preferred naming convention
codegen succeeds for error type args that reference objects
Before this PR
When an error is define with an argument that references an object type the generator panics: https://app.circleci.com/pipelines/github/palantir/conjure-rust/1028/workflows/fc887808-5e16-4d64-8576-d457a7719813/jobs/2726
This is because on this line we check the type context of
this_typebut it doesn't exist becausethis_typeactually references the error. I think this is in part because we are re-using the object generation codepath with a made up "error type" here.I also noticed while testing that the object arguments are not included in the serialized parameters of the generated error type. This appears to be intentional so leaving as is for now.
After this PR
We inject the error types as virtual object definitions in the context object so that everything resolves correctly.
==COMMIT_MSG==
codegen succeeds for error type args that reference objects
==COMMIT_MSG==
Possible downsides?
Technically you could reference an error type as in object in other fields/args but we expect this should be handled already when compiling from yml to the IR.