Use human friendly type format for type error display #708
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| //! Defines the type structure for typechecking and various utilities for | ||
| //! constructing and manipulating types. | ||
|
|
||
| use itertools::Itertools; | ||
| use serde::Serialize; | ||
| use smol_str::SmolStr; | ||
| use std::{ | ||
|
|
@@ -497,98 +498,6 @@ impl Type { | |
| ) | ||
| } | ||
|
|
||
| /// Is this validator type "consistent with" the given Core SchemaType. | ||
| /// Meaning, is there at least some value that could have this SchemaType and | ||
| /// this validator type simultaneously. | ||
|
|
@@ -877,11 +786,60 @@ impl Type { | |
|
|
||
| impl Display for Type { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Type::Never => write!(f, "__cedar::internal::Never"), | ||
| Type::True => write!(f, "__cedar::internal::True"), | ||
| Type::False => write!(f, "__cedar::internal::False"), | ||
| Type::Primitive { | ||
| primitive_type: Primitive::Long, | ||
| } => write!(f, "Long"), | ||
| Type::Primitive { | ||
| primitive_type: Primitive::Bool, | ||
| } => write!(f, "Bool"), | ||
| Type::Primitive { | ||
| primitive_type: Primitive::String, | ||
| } => write!(f, "String"), | ||
| Type::Set { element_type } => match element_type { | ||
| Some(element_type) => write!(f, "Set<{element_type}>"), | ||
| None => write!(f, "Set<__cedar::internal::Any>"), | ||
| }, | ||
| Type::EntityOrRecord(EntityRecordKind::AnyEntity) => { | ||
| write!(f, "__cedar::internal::AnyEntity") | ||
| } | ||
| // Ignoring action attributes for display purposes. | ||
| Type::EntityOrRecord(EntityRecordKind::ActionEntity { | ||
| name, | ||
| attrs: _attrs, | ||
| }) => write!(f, "{name}"), | ||
| Type::EntityOrRecord(EntityRecordKind::Entity(elub)) => { | ||
| match elub.get_single_entity() { | ||
| Some(e) => write!(f, "{e}"), | ||
| None => write!( | ||
| f, | ||
| "__cedar::internal::Union<{}>", | ||
| elub.iter().map(ToString::to_string).join(", ") | ||
| ), | ||
| } | ||
| } | ||
| Type::EntityOrRecord(EntityRecordKind::Record { | ||
| attrs, | ||
| open_attributes, | ||
| }) => { | ||
| if open_attributes.is_open() { | ||
| write!(f, "__cedar::internal::OpenRecord")?; | ||
| } | ||
| write!(f, "{{")?; | ||
| for (name, ty) in attrs.iter() { | ||
| write!(f, "{name}")?; | ||
| if !ty.is_required { | ||
| write!(f, "?")?; | ||
| } | ||
| write!(f, ": {},", ty.attr_type)?; | ||
| } | ||
| write!(f, "}}") | ||
| } | ||
| Type::ExtensionType { name } => write!(f, "{name}"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1077,35 +1035,6 @@ impl EntityLUB { | |
| pub(crate) fn contains_entity_type(&self, ety: &Name) -> bool { | ||
| self.lub_elements.contains(ety) | ||
| } | ||
| } | ||
|
|
||
| /// Represents the attributes of a record or entity type. Each attribute has an | ||
|
|
@@ -1655,7 +1584,7 @@ impl<'a> Effect<'a> { | |
| #[cfg(test)] | ||
| mod test { | ||
| use super::*; | ||
| use crate::{human_schema::parser::parse_type, ActionBehavior, ValidatorNamespaceDef}; | ||
| use cool_asserts::assert_matches; | ||
| use std::collections::HashMap; | ||
|
|
||
|
|
@@ -2342,11 +2271,11 @@ mod test { | |
| } | ||
|
|
||
| #[track_caller] // report the caller's location as the location of the panic, not the location in this function | ||
| fn assert_type_display_roundtrip(ty: Type) { | ||
| let type_str = ty.to_string(); | ||
| println!("{type_str}"); | ||
| let parsed_schema_type = parse_type(&type_str) | ||
| .expect("String representation should have parsed into a schema type"); | ||
| let type_from_schema_type = | ||
| ValidatorNamespaceDef::try_schema_type_into_validator_type(None, parsed_schema_type) | ||
| .expect("Schema type should have converted to type.") | ||
|
|
@@ -2356,22 +2285,19 @@ mod test { | |
| } | ||
|
|
||
| #[test] | ||
| fn type_display_roundtrip() { | ||
| assert_type_display_roundtrip(Type::primitive_boolean()); | ||
| assert_type_display_roundtrip(Type::primitive_long()); | ||
| assert_type_display_roundtrip(Type::primitive_string()); | ||
| assert_type_display_roundtrip(Type::set(Type::primitive_boolean())); | ||
| assert_type_display_roundtrip(Type::set(Type::primitive_string())); | ||
| assert_type_display_roundtrip(Type::set(Type::primitive_long())); | ||
| assert_type_display_roundtrip(Type::closed_record_with_attributes(None)); | ||
| assert_type_display_roundtrip(Type::closed_record_with_attributes([( | ||
| "a".into(), | ||
| AttributeType::required_attribute(Type::primitive_boolean()), | ||
| )])); | ||
| assert_type_display_roundtrip(Type::closed_record_with_attributes([ | ||
| ( | ||
| "a".into(), | ||
| AttributeType::required_attribute(Type::primitive_boolean()), | ||
|
|
@@ -2393,13 +2319,29 @@ mod test { | |
| ); | ||
| } | ||
|
|
||
| // Test display for types that don't roundtrip. | ||
| #[test] | ||
| fn test_type_display() { | ||
| // Entity types don't roundtrip because the human format type parser | ||
| // checks that they are defined already, so we'd need to provide a | ||
| // complete schema. TODO: the final stage of schema parsing already does | ||
| // this. Can we remove duplicated checks from human schema parsing? | ||
|
Comment on lines
+2327
to
+2328
There was a problem hiding this comment. Would be good to address this TODO in the PR, or file an issue for later & add the issue number to the comment There was a problem hiding this comment. I think it's not necessarily an easy change. #711 |
||
| assert_displays_as(Type::named_entity_reference_from_str("Foo"), "Foo"); | ||
| assert_displays_as( | ||
| Type::named_entity_reference_from_str("Foo::Bar"), | ||
| "Foo::Bar", | ||
| ); | ||
| assert_displays_as( | ||
| Type::named_entity_reference_from_str("Foo::Bar::Baz"), | ||
| "Foo::Bar::Baz", | ||
| ); | ||
|
|
||
| // These type aren't representable in a schema. | ||
| assert_displays_as(Type::Never, "__cedar::internal::Never"); | ||
| assert_displays_as(Type::True, "__cedar::internal::True"); | ||
| assert_displays_as(Type::False, "__cedar::internal::False"); | ||
| assert_displays_as(Type::any_set(), "Set<__cedar::internal::Any>"); | ||
| assert_displays_as(Type::any_entity_reference(), "__cedar::internal::AnyEntity"); | ||
| assert_displays_as( | ||
| Type::least_upper_bound( | ||
| &ValidatorSchema::empty(), | ||
|
|
@@ -2408,14 +2350,14 @@ mod test { | |
| ValidationMode::Permissive, | ||
| ) | ||
| .expect("Expected a least upper bound to exist."), | ||
| "__cedar::internal::Union<Bar, Foo>", | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(feature = "ipaddr")] | ||
| fn text_extension_type_dislay() { | ||
| let ipaddr = Name::parse_unqualified_name("ipaddr").expect("should be a valid identifier"); | ||
| assert_type_display_roundtrip(Type::extension(ipaddr)); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "custom" type? Isn't it just the internal Cedar type?
(Context: I was confused by the name at first because I was trying to figure out how it related to
commonTypedefinitions in the JSON /typedeclarations in the new schema syntax)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just copied the phrasing from another function in this file. We could perhaps do a consistency pass so that we call the new schema format one name everywhere.