From a2d2bc8f2c8e4480493f93951dba473e804a7284 Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Wed, 20 Sep 2023 14:56:40 -0400 Subject: [PATCH 01/10] add test case --- conjure-test/test-ir.json | 9 +++++++++ conjure-test/test.yml | 1 + 2 files changed, 10 insertions(+) diff --git a/conjure-test/test-ir.json b/conjure-test/test-ir.json index bd456df7..162edce2 100644 --- a/conjure-test/test-ir.json +++ b/conjure-test/test-ir.json @@ -19,6 +19,15 @@ "type" : "primitive", "primitive" : "INTEGER" } + }, { + "fieldName" : "ref", + "type" : { + "type" : "reference", + "reference" : { + "name": "SafeStringAlias", + "package": "com.palantir.conjure" + } + } } ], "unsafeArgs" : [ { "fieldName" : "unsafeFoo", diff --git a/conjure-test/test.yml b/conjure-test/test.yml index eb0fbba9..cc0236ff 100644 --- a/conjure-test/test.yml +++ b/conjure-test/test.yml @@ -156,6 +156,7 @@ types: safe-args: foo: string bar: integer + ref: SafeStringAlias unsafe-args: unsafeFoo: boolean From a89466bb21cbcf18000946260338a77c1ca29a83 Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Wed, 20 Sep 2023 16:36:09 -0400 Subject: [PATCH 02/10] fix --- conjure-test/src/test/errors.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/conjure-test/src/test/errors.rs b/conjure-test/src/test/errors.rs index 72b2a262..c2eb5fae 100644 --- a/conjure-test/src/test/errors.rs +++ b/conjure-test/src/test/errors.rs @@ -19,11 +19,16 @@ use crate::types::*; #[test] fn error_serialization() { - let error = SimpleError::new("hello", 15, false); + let error = SimpleError::builder() + .foo("hello") + .bar(15) + .ref_(SafeStringAlias("safe".to_string())) + .unsafe_foo(false) + .build(); assert_eq!(error.code(), ErrorCode::Internal); assert_eq!(error.name(), "Test:SimpleError"); - assert_eq!(error.safe_args(), &["bar", "foo"]); + assert_eq!(error.safe_args(), &["bar", "foo", "ref"]); let encoded = conjure_error::encode(&error); @@ -33,6 +38,7 @@ fn error_serialization() { let mut params = BTreeMap::new(); params.insert("foo".to_string(), "hello".to_string()); params.insert("bar".to_string(), "15".to_string()); + params.insert("ref".to_string(), "safe".to_string()); params.insert("unsafeFoo".to_string(), "false".to_string()); assert_eq!(*encoded.parameters(), params); } From 193374df4e4a0ba16a5c292b1ff6a25249469d90 Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Wed, 20 Sep 2023 16:46:12 -0400 Subject: [PATCH 03/10] introduce error --- conjure-test/src/test/errors.rs | 2 +- conjure-test/test-ir.json | 4 ++-- conjure-test/test.yml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/conjure-test/src/test/errors.rs b/conjure-test/src/test/errors.rs index c2eb5fae..9c09661e 100644 --- a/conjure-test/src/test/errors.rs +++ b/conjure-test/src/test/errors.rs @@ -22,7 +22,7 @@ fn error_serialization() { let error = SimpleError::builder() .foo("hello") .bar(15) - .ref_(SafeStringAlias("safe".to_string())) + .baz(TestObject::builder().foo(1).build()) .unsafe_foo(false) .build(); diff --git a/conjure-test/test-ir.json b/conjure-test/test-ir.json index 162edce2..98155433 100644 --- a/conjure-test/test-ir.json +++ b/conjure-test/test-ir.json @@ -20,11 +20,11 @@ "primitive" : "INTEGER" } }, { - "fieldName" : "ref", + "fieldName" : "baz", "type" : { "type" : "reference", "reference" : { - "name": "SafeStringAlias", + "name": "TestObject", "package": "com.palantir.conjure" } } diff --git a/conjure-test/test.yml b/conjure-test/test.yml index cc0236ff..20bbbec9 100644 --- a/conjure-test/test.yml +++ b/conjure-test/test.yml @@ -156,7 +156,7 @@ types: safe-args: foo: string bar: integer - ref: SafeStringAlias + baz: TestObject unsafe-args: unsafeFoo: boolean From d70eb4f3b71b7fa3c9495720faff05d1894a9e72 Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Wed, 20 Sep 2023 17:05:20 -0400 Subject: [PATCH 04/10] "fix" --- conjure-codegen/src/context.rs | 10 +++++++--- conjure-test/src/test/errors.rs | 3 +-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/conjure-codegen/src/context.rs b/conjure-codegen/src/context.rs index 5719cb38..610d7021 100644 --- a/conjure-codegen/src/context.rs +++ b/conjure-codegen/src/context.rs @@ -329,9 +329,13 @@ impl Context { let needs_box = match &ctx.def { TypeDefinition::Alias(def) => self.needs_box(def.alias()), TypeDefinition::Enum(_) => false, - TypeDefinition::Object(_) => match &self.types[this_type].def { - TypeDefinition::Union(_) => false, - _ => true, + TypeDefinition::Object(_) => match self.types.get(this_type) { + // this is not a type, e.g., it is an error + None => true, + Some(ctx) => match &ctx.def { + TypeDefinition::Union(_) => false, + _ => true, + }, }, TypeDefinition::Union(_) => true, }; diff --git a/conjure-test/src/test/errors.rs b/conjure-test/src/test/errors.rs index 9c09661e..b72db64c 100644 --- a/conjure-test/src/test/errors.rs +++ b/conjure-test/src/test/errors.rs @@ -28,7 +28,7 @@ fn error_serialization() { assert_eq!(error.code(), ErrorCode::Internal); assert_eq!(error.name(), "Test:SimpleError"); - assert_eq!(error.safe_args(), &["bar", "foo", "ref"]); + assert_eq!(error.safe_args(), &["bar", "baz", "foo"]); let encoded = conjure_error::encode(&error); @@ -38,7 +38,6 @@ fn error_serialization() { let mut params = BTreeMap::new(); params.insert("foo".to_string(), "hello".to_string()); params.insert("bar".to_string(), "15".to_string()); - params.insert("ref".to_string(), "safe".to_string()); params.insert("unsafeFoo".to_string(), "false".to_string()); assert_eq!(*encoded.parameters(), params); } From 4df11248d7813a646e9bf1a6f027a91ef1280815 Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Wed, 20 Sep 2023 17:19:35 -0400 Subject: [PATCH 05/10] ignore deprecated --- conjure-test/src/test/errors.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/conjure-test/src/test/errors.rs b/conjure-test/src/test/errors.rs index b72db64c..91eee6ae 100644 --- a/conjure-test/src/test/errors.rs +++ b/conjure-test/src/test/errors.rs @@ -19,6 +19,7 @@ use crate::types::*; #[test] fn error_serialization() { + #[allow(deprecated)] let error = SimpleError::builder() .foo("hello") .bar(15) From 212329128de9fe153f153cd28799fffe6c33c057 Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Thu, 21 Sep 2023 11:07:39 -0400 Subject: [PATCH 06/10] better --- conjure-codegen/src/context.rs | 26 +++++++++++++++++--------- conjure-codegen/src/errors.rs | 16 +++++++++++----- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/conjure-codegen/src/context.rs b/conjure-codegen/src/context.rs index 610d7021..168f0f0b 100644 --- a/conjure-codegen/src/context.rs +++ b/conjure-codegen/src/context.rs @@ -20,8 +20,8 @@ use std::cell::{Cell, RefCell}; use std::collections::HashMap; use crate::types::{ - ArgumentDefinition, ConjureDefinition, Documentation, LogSafety, PrimitiveType, Type, - TypeDefinition, TypeName, + ArgumentDefinition, ConjureDefinition, Documentation, LogSafety, ObjectDefinition, + PrimitiveType, Type, TypeDefinition, TypeName, }; enum CachedLogSafety { @@ -83,6 +83,18 @@ impl Context { ); } + for def in defs.errors() { + context.types.insert( + def.error_name().clone(), + TypeContext { + def: TypeDefinition::Object(ObjectDefinition::from(def.clone())), + has_double: Cell::new(None), + is_copy: Cell::new(None), + log_safety: RefCell::new(CachedLogSafety::Uncomputed), + }, + ); + } + context } @@ -329,13 +341,9 @@ impl Context { let needs_box = match &ctx.def { TypeDefinition::Alias(def) => self.needs_box(def.alias()), TypeDefinition::Enum(_) => false, - TypeDefinition::Object(_) => match self.types.get(this_type) { - // this is not a type, e.g., it is an error - None => true, - Some(ctx) => match &ctx.def { - TypeDefinition::Union(_) => false, - _ => true, - }, + TypeDefinition::Object(_) => match &self.types[this_type].def { + TypeDefinition::Union(_) => false, + _ => true, }, TypeDefinition::Union(_) => true, }; diff --git a/conjure-codegen/src/errors.rs b/conjure-codegen/src/errors.rs index 0ad0c36f..96a7a124 100644 --- a/conjure-codegen/src/errors.rs +++ b/conjure-codegen/src/errors.rs @@ -18,12 +18,18 @@ use crate::context::Context; use crate::objects; use crate::types::{ErrorDefinition, ObjectDefinition}; +impl From 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() + } +} + pub fn generate(ctx: &Context, def: &ErrorDefinition) -> TokenStream { - let object = ObjectDefinition::builder() - .type_name(def.error_name().clone()) - .fields(def.safe_args().iter().chain(def.unsafe_args()).cloned()) - .docs(def.docs().cloned()) - .build(); + let object = ObjectDefinition::from(def.clone()); let object_def = objects::generate(ctx, &object); let error_type = generate_error_type(ctx, def); From f180aed95b17cc2922cbb5dda6b4c8e72dab9814 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Thu, 21 Sep 2023 15:42:45 +0000 Subject: [PATCH 07/10] Add generated changelog entries --- changelog/@unreleased/pr-264.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-264.v2.yml diff --git a/changelog/@unreleased/pr-264.v2.yml b/changelog/@unreleased/pr-264.v2.yml new file mode 100644 index 00000000..1cd9194b --- /dev/null +++ b/changelog/@unreleased/pr-264.v2.yml @@ -0,0 +1,5 @@ +type: fix +fix: + description: Codegen succeeds for error type args that reference objects + links: + - https://github.com/palantir/conjure-rust/pull/264 From 7d673fb83f924c5c41d137235bd5705249abd25e Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Tue, 26 Sep 2023 10:55:03 -0400 Subject: [PATCH 08/10] better --- conjure-codegen/src/context.rs | 6 +++--- conjure-codegen/src/errors.rs | 12 ++++++------ conjure-test/src/test/errors.rs | 2 +- conjure-test/test-ir.json | 2 +- conjure-test/test.yml | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/conjure-codegen/src/context.rs b/conjure-codegen/src/context.rs index 168f0f0b..d9d6e3d7 100644 --- a/conjure-codegen/src/context.rs +++ b/conjure-codegen/src/context.rs @@ -20,8 +20,8 @@ use std::cell::{Cell, RefCell}; use std::collections::HashMap; use crate::types::{ - ArgumentDefinition, ConjureDefinition, Documentation, LogSafety, ObjectDefinition, - PrimitiveType, Type, TypeDefinition, TypeName, + ArgumentDefinition, ConjureDefinition, Documentation, LogSafety, PrimitiveType, Type, + TypeDefinition, TypeName, }; enum CachedLogSafety { @@ -87,7 +87,7 @@ impl Context { context.types.insert( def.error_name().clone(), TypeContext { - def: TypeDefinition::Object(ObjectDefinition::from(def.clone())), + def: TypeDefinition::Object(def.object_definition()), has_double: Cell::new(None), is_copy: Cell::new(None), log_safety: RefCell::new(CachedLogSafety::Uncomputed), diff --git a/conjure-codegen/src/errors.rs b/conjure-codegen/src/errors.rs index 96a7a124..effd1f1f 100644 --- a/conjure-codegen/src/errors.rs +++ b/conjure-codegen/src/errors.rs @@ -18,18 +18,18 @@ use crate::context::Context; use crate::objects; use crate::types::{ErrorDefinition, ObjectDefinition}; -impl From for ObjectDefinition { - fn from(error: ErrorDefinition) -> Self { +impl ErrorDefinition { + pub fn object_definition(&self) -> ObjectDefinition { ObjectDefinition::builder() - .type_name(error.error_name().clone()) - .fields(error.safe_args().iter().chain(error.unsafe_args()).cloned()) - .docs(error.docs().cloned()) + .type_name(self.error_name().clone()) + .fields(self.safe_args().iter().chain(self.unsafe_args()).cloned()) + .docs(self.docs().cloned()) .build() } } pub fn generate(ctx: &Context, def: &ErrorDefinition) -> TokenStream { - let object = ObjectDefinition::from(def.clone()); + let object = def.object_definition(); let object_def = objects::generate(ctx, &object); let error_type = generate_error_type(ctx, def); diff --git a/conjure-test/src/test/errors.rs b/conjure-test/src/test/errors.rs index 91eee6ae..ac52a22f 100644 --- a/conjure-test/src/test/errors.rs +++ b/conjure-test/src/test/errors.rs @@ -23,7 +23,7 @@ fn error_serialization() { let error = SimpleError::builder() .foo("hello") .bar(15) - .baz(TestObject::builder().foo(1).build()) + .baz(EmptyObject::new()) .unsafe_foo(false) .build(); diff --git a/conjure-test/test-ir.json b/conjure-test/test-ir.json index 98155433..9859237e 100644 --- a/conjure-test/test-ir.json +++ b/conjure-test/test-ir.json @@ -24,7 +24,7 @@ "type" : { "type" : "reference", "reference" : { - "name": "TestObject", + "name": "EmptyObject", "package": "com.palantir.conjure" } } diff --git a/conjure-test/test.yml b/conjure-test/test.yml index 20bbbec9..1961853d 100644 --- a/conjure-test/test.yml +++ b/conjure-test/test.yml @@ -156,7 +156,7 @@ types: safe-args: foo: string bar: integer - baz: TestObject + baz: EmptyObject unsafe-args: unsafeFoo: boolean From a54b30413947c66a606632aebc04bd49f8212c1a Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Tue, 26 Sep 2023 10:58:08 -0400 Subject: [PATCH 09/10] no longer deprecated --- conjure-test/src/test/errors.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/conjure-test/src/test/errors.rs b/conjure-test/src/test/errors.rs index ac52a22f..f1bab301 100644 --- a/conjure-test/src/test/errors.rs +++ b/conjure-test/src/test/errors.rs @@ -19,7 +19,6 @@ use crate::types::*; #[test] fn error_serialization() { - #[allow(deprecated)] let error = SimpleError::builder() .foo("hello") .bar(15) From 2f19a03abdaf22454d4a826887ab5b9d745ebf15 Mon Sep 17 00:00:00 2001 From: Andrew Floren Date: Thu, 28 Sep 2023 14:20:42 -0400 Subject: [PATCH 10/10] free fn --- conjure-codegen/src/context.rs | 3 ++- conjure-codegen/src/errors.rs | 16 +++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/conjure-codegen/src/context.rs b/conjure-codegen/src/context.rs index d9d6e3d7..df78befe 100644 --- a/conjure-codegen/src/context.rs +++ b/conjure-codegen/src/context.rs @@ -19,6 +19,7 @@ use quote::quote; use std::cell::{Cell, RefCell}; use std::collections::HashMap; +use crate::errors::error_object_definition; use crate::types::{ ArgumentDefinition, ConjureDefinition, Documentation, LogSafety, PrimitiveType, Type, TypeDefinition, TypeName, @@ -87,7 +88,7 @@ impl Context { context.types.insert( def.error_name().clone(), TypeContext { - def: TypeDefinition::Object(def.object_definition()), + def: TypeDefinition::Object(error_object_definition(def)), has_double: Cell::new(None), is_copy: Cell::new(None), log_safety: RefCell::new(CachedLogSafety::Uncomputed), diff --git a/conjure-codegen/src/errors.rs b/conjure-codegen/src/errors.rs index effd1f1f..05317bee 100644 --- a/conjure-codegen/src/errors.rs +++ b/conjure-codegen/src/errors.rs @@ -18,18 +18,16 @@ use crate::context::Context; use crate::objects; use crate::types::{ErrorDefinition, ObjectDefinition}; -impl ErrorDefinition { - pub fn object_definition(&self) -> ObjectDefinition { - ObjectDefinition::builder() - .type_name(self.error_name().clone()) - .fields(self.safe_args().iter().chain(self.unsafe_args()).cloned()) - .docs(self.docs().cloned()) - .build() - } +pub fn error_object_definition(def: &ErrorDefinition) -> ObjectDefinition { + ObjectDefinition::builder() + .type_name(def.error_name().clone()) + .fields(def.safe_args().iter().chain(def.unsafe_args()).cloned()) + .docs(def.docs().cloned()) + .build() } pub fn generate(ctx: &Context, def: &ErrorDefinition) -> TokenStream { - let object = def.object_definition(); + let object = error_object_definition(def); let object_def = objects::generate(ctx, &object); let error_type = generate_error_type(ctx, def);