Resolve types at fragment-combine time

cedar-policy-core/Cargo.toml

@@ -20,6 +20,7 @@ lalrpop-util = { version = "0.20.0", features = ["lexer"] }
 lazy_static = "1.4"
 either = "1.8"
 itertools = "0.13"
+ref-cast = "1.0"
 rustc_lexer = "0.1"
 thiserror = "1.0"
 smol_str = { version = "0.2", features = ["serde"] }

cedar-policy-core/src/ast/entity.rs

@@ -24,6 +24,7 @@ use crate::transitive_closure::TCNode;
 use crate::FromNormalizedStr;
 use itertools::Itertools;
 use miette::Diagnostic;
+use ref_cast::RefCast;
 use serde::{Deserialize, Serialize};
 use serde_with::{serde_as, TryFromInto};
 use smol_str::SmolStr;
@@ -35,9 +36,10 @@ use thiserror::Error;
 pub static ACTION_ENTITY_TYPE: &str = "Action";
 
 /// Entity type names are just [`Name`]s, but we have some operations on them specific to entity types.
-#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Hash, PartialOrd, Ord)]
+#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Hash, PartialOrd, Ord, RefCast)]
 #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 #[serde(transparent)]
+#[repr(transparent)]
 pub struct EntityType(Name);
 
 impl EntityType {
@@ -76,6 +78,12 @@ impl From<Name> for EntityType {
     }
 }
 
+impl From<EntityType> for Name {
+    fn from(ty: EntityType) -> Name {
+        ty.0
+    }
+}
+
 impl AsRef<Name> for EntityType {
     fn as_ref(&self) -> &Name {
         &self.0

cedar-policy-core/src/ast/name.rs

@@ -107,6 +107,12 @@ impl Name {
         }
     }
 
+    /// Get the `Name` representing the reserved `__cedar` namespace
+    pub fn __cedar() -> Self {
+        // using `Id::new_unchecked()` for performance reasons -- this function called many times by validator code
+        Self::unqualified_name(Id::new_unchecked("__cedar"))
+    }
+
     /// Create a `Name` with no path (no namespaces).
     /// Returns an error if `s` is not a valid identifier.
     pub fn parse_unqualified_name(s: &str) -> Result<Self, ParseErrors> {
@@ -152,20 +158,22 @@ impl Name {
 
     /// Qualify the name with a namespace
     ///
-    /// When the name already has an explicit namespace, it doesn't make sense
-    /// to prefix any namespace, and hence this method returns a copy of `self`.
+    /// If the name already has a non-empty namespace, this method does not
+    /// apply any prefix and instead returns a copy of `self`.
     ///
     /// If `namespace` is `None`, that represents the empty namespace, so no
     /// prefixing will be done.
     ///
-    /// When the name does not already have an explicit namespace, and
-    /// `namespace` is `Some`, prefix it with the namespace.
+    /// If the name does not already have an explicit namespace (i.e., it's
+    /// just a single `Id`), this applies `namespace` as a prefix (if it is
+    /// present).
     ///
     /// Examples:
-    /// `A::B`.qualify_with(Some(C)) is just A::B
-    /// `A`.qualify_with(Some(C)) is C::A
-    /// `A`.qualify_with(Some(B::C)) is B::C::A
-    /// `A`.qualify_with(None) is A
+    /// - `A::B`.qualify_with(None) is `A::B`
+    /// - `A::B`.qualify_with(Some(C)) is also `A::B`
+    /// - `A`.qualify_with(None) is `A`
+    /// - `A`.qualify_with(Some(C)) is `C::A`
+    /// - `A`.qualify_with(Some(B::C)) is `B::C::A`
     pub fn qualify_with(&self, namespace: Option<&Name>) -> Name {
         if self.is_unqualified() {
             // Ideally, we want to implement `IntoIterator` for `Name`

cedar-policy-validator/Cargo.toml

@@ -19,6 +19,7 @@ serde_with = "3.0"
 miette = "7.1.0"
 thiserror = "1.0"
 itertools = "0.13"
+ref-cast = "1.0"
 unicode-security = "0.1.0"
 smol_str = { version = "0.2", features = ["serde"] }
 stacker = "0.1.15"

cedar-policy-validator/src/coreschema.rs

@@ -197,10 +197,7 @@ impl ast::RequestSchema for ValidatorSchema {
                     euid: principal, ..
                 } = request.principal()
                 {
-                    if !validator_action_id
-                        .applies_to
-                        .is_applicable_principal_type(principal.entity_type())
-                    {
+                    if !validator_action_id.is_applicable_principal_type(principal.entity_type()) {
                         return Err(request_validation_errors::InvalidPrincipalTypeError {
                             principal_ty: principal.entity_type().clone(),
                             action: Arc::clone(action),
@@ -209,10 +206,7 @@ impl ast::RequestSchema for ValidatorSchema {
                     }
                 }
                 if let EntityUIDEntry::Known { euid: resource, .. } = request.resource() {
-                    if !validator_action_id
-                        .applies_to
-                        .is_applicable_resource_type(resource.entity_type())
-                    {
+                    if !validator_action_id.is_applicable_resource_type(resource.entity_type()) {
                         return Err(request_validation_errors::InvalidResourceTypeError {
                             resource_ty: resource.entity_type().clone(),
                             action: Arc::clone(action),

cedar-policy-validator/src/err.rs

@@ -150,11 +150,19 @@ pub enum SchemaError {
     #[error(transparent)]
     #[diagnostic(transparent)]
     UndeclaredActions(#[from] schema_errors::UndeclaredActionsError),
-    /// This error occurs when an undeclared common type appears in entity or context
-    /// attributes.
+    /// This error occurs when we cannot resolve a typename we found in an
+    /// entity or context attribute. (because it refers to an entity type
+    /// or common type that was not declared.)
     #[error(transparent)]
     #[diagnostic(transparent)]
-    UndeclaredCommonTypes(#[from] schema_errors::UndeclaredCommonTypesError),
+    TypeResolution(#[from] schema_errors::TypeResolutionError),
+    /// This error occurs when we cannot resolve a common-type reference.
+    /// Normally, such errors would be `TypeResolution` instead; I think
+    /// this variant should never occur, but not positive, and hard to guarantee
+    /// structurally.
+    #[error(transparent)]
+    #[diagnostic(transparent)]
+    UndeclaredCommonType(#[from] schema_errors::UndeclaredCommonTypeError),
     /// Duplicate specifications for an entity type. Argument is the name of
     /// the duplicate entity type.
     #[error(transparent)]
@@ -212,10 +220,6 @@ pub enum SchemaError {
     #[error(transparent)]
     #[diagnostic(transparent)]
     UnknownExtensionType(schema_errors::UnknownExtensionTypeError),
-    /// Common type names conflict with primitive types.
-    #[error(transparent)]
-    #[diagnostic(transparent)]
-    CommonTypeNameConflict(#[from] schema_errors::CommonTypeNameConflictError),
 }
 
 impl From<transitive_closure::TcError<EntityUID>> for SchemaError {
@@ -242,12 +246,13 @@ pub mod schema_errors {
     use std::{collections::BTreeSet, fmt::Display};
 
     use cedar_policy_core::{
-        ast::{EntityAttrEvaluationError, EntityType, EntityUID, Id, Name},
+        ast::{EntityAttrEvaluationError, EntityType, EntityUID, Name},
         parser::join_with_conjunction,
         transitive_closure,
     };
     use itertools::Itertools;
     use miette::Diagnostic;
+    use nonempty::NonEmpty;
     use smol_str::SmolStr;
     use thiserror::Error;
 
@@ -326,15 +331,51 @@ pub mod schema_errors {
         }
     }
 
-    /// Undeclared common types error
+    /// Undeclared common type error
     //
     // CAUTION: this type is publicly exported in `cedar-policy`.
     // Don't make fields `pub`, don't make breaking changes, and use caution
     // when adding public methods.
     #[derive(Debug, Diagnostic, Error)]
     #[error("undeclared common type: {0}")]
-    #[diagnostic(help("any common types used in entity or context attributes need to be declared in `commonTypes`"))]
-    pub struct UndeclaredCommonTypesError(pub(crate) Name);
+    pub struct UndeclaredCommonTypeError(
+        // Note this is a fully-qualified `Name`; we can't conclude a
+        // common-type reference is undeclared without first determining which
+        // fully-qualified `Name` it resolves to
+        pub(crate) Name,
+    );
+
+    /// Type resolution error
+    //
+    // CAUTION: this type is publicly exported in `cedar-policy`.
+    // Don't make fields `pub`, don't make breaking changes, and use caution
+    // when adding public methods.
+    #[derive(Debug, Diagnostic, Error)]
+    #[error("failed to resolve type{}: {}", if .0.len() > 1 { "s" } else { "" }, .0.iter().map(crate::ConditionalName::raw).join(", "))]
+    #[diagnostic(help("{}", .0.first().resolution_failure_help()))] // we choose to give only the help for the first failed-to-resolve name, because otherwise the help message would be too cluttered and complicated
+    pub struct TypeResolutionError(pub(crate) NonEmpty<crate::ConditionalName>);
+
+    impl TypeResolutionError {
+        /// Combine all the errors into a single `TypeResolutionError`.
+        ///
+        /// Returns `None` if the input `errs` was empty, otherwise returns `Some`.
+        pub(crate) fn join(errs: impl IntoIterator<Item = TypeResolutionError>) -> Option<Self> {
+            NonEmpty::collect(errs.into_iter().flat_map(|err| err.0)).map(Self)
+        }
+
+        /// Combine all the errors into a single `TypeResolutionError`.
+        ///
+        /// Unlike `join()`, this cannot fail, because `NonEmpty` guarantees
+        /// there is at least one error to join.
+        pub(crate) fn join_nonempty(errs: NonEmpty<TypeResolutionError>) -> Self {
+            // PANIC SAFETY: Since the input is `NonEmpty`, there must be at least one error, so `NonEmpty::collect()` must return `Some`
+            #[allow(clippy::expect_used)]
+            Self(
+                NonEmpty::collect(errs.into_iter().flat_map(|err| err.0))
+                    .expect("input contained at least one error, so NonEmpty must be nonempty"),
+            )
+        }
+    }
 
     /// Duplicate entity type error
     //
@@ -570,14 +611,4 @@ pub mod schema_errors {
             })
         }
     }
-
-    /// This error is thrown when a common type name conflicts with a primitive
-    /// type
-    //
-    // CAUTION: this type is publicly exported in `cedar-policy`.
-    // Don't make fields `pub`, don't make breaking changes, and use caution
-    // when adding public methods.
-    #[derive(Error, Debug, Diagnostic)]
-    #[error("Common type name `{0}` conflicts with primitive type")]
-    pub struct CommonTypeNameConflictError(pub(crate) Id);
 }

cedar-policy-validator/src/human_schema/fmt.rs

@@ -87,7 +87,8 @@ impl<N: Display> Display for SchemaType<N> {
                 SchemaTypeVariant::Set { element } => write!(f, "Set < {element} >"),
                 SchemaTypeVariant::String => write!(f, "__cedar::String"),
             },
-            SchemaType::CommonTypeRef { type_name } => write!(f, "{type_name}"),
+            SchemaType::CommonTypeRef { type_name }
+            | SchemaType::EntityOrCommonTypeRef { type_name } => write!(f, "{type_name}"),
         }
     }
 }
@@ -183,6 +184,23 @@ impl NameCollisionsError {
 }
 
 /// Convert a [`SchemaFragment`] to a string containing human schema syntax
+///
+/// REVIEW: The existing code throws an error if any fully-qualified name in a
+/// non-empty namespace is a valid common type and also a valid entity type.
+/// 1) I think this check is more conservative than necessary? Schemas are
+/// allowed to shadow an entity type with a common type declaration in the same
+/// namespace; see RFCs 24 and 70. What the human syntax can't express is if, in
+/// that situation, we then specifically refer to the shadowed entity type name.
+/// But it's harder to walk all type references than it is to walk all type
+/// declarations, so perhaps the conservative code here is fine.
+/// 2) At the same time, this check might miss some cases that are actually
+/// problematic. E.g., if a common type declaration in some namespace NS has the
+/// same basename as an entity type declaration in the empty namespace, and then
+/// the JSON syntax references the entity type declaration in the empty
+/// namespace (while the current namespace is NS). When converted to human
+/// syntax, the reference will resolve to the common type declaration in the
+/// current namespace instead. The existing code doesn't catch collisions with
+/// the empty namespace. But see RFC 70.
 pub fn json_schema_to_custom_schema_str<N: Display>(
     json_schema: &SchemaFragment<N>,
 ) -> Result<String, ToHumanSchemaSyntaxError> {

cedar-policy-validator/src/human_schema/parser.rs

@@ -25,7 +25,7 @@ use thiserror::Error;
 use super::{
     ast::Schema,
     err::{self, ParseError, ParseErrors, SchemaWarning, ToJsonSchemaErrors},
-    to_json_schema::{custom_schema_to_json_schema, custom_type_to_json_type},
+    to_json_schema::custom_schema_to_json_schema,
 };
 use cedar_policy_core::extensions::Extensions;
 
@@ -95,15 +95,6 @@ pub enum HumanSyntaxParseErrors {
     JsonError(#[from] ToJsonSchemaErrors),
 }
 
-/// Parse a type, in human syntax, into a [`crate::SchemaType`]
-pub fn parse_type(
-    src: &str,
-    extensions: Extensions<'_>,
-) -> Result<crate::SchemaType<crate::RawName>, HumanSyntaxParseErrors> {
-    let ty = parse_collect_errors(&*TYPE_PARSER, grammar::TypeParser::parse, src)?;
-    Ok(custom_type_to_json_type(ty, extensions)?)
-}
-
 /// Parse a schema fragment, in human syntax, into a [`crate::SchemaFragment`],
 /// possibly generating warnings
 pub fn parse_natural_schema_fragment<'a>(