cedar-policy / rfcs

Apache License 2.0
10 stars 8 forks source link

Referring to types in the empty namespace #64

Closed cdisselkoen closed 4 months ago

cdisselkoen commented 6 months ago

Rendered

chrnorm commented 6 months ago

+1 to the ::EntityType syntax, especially the implicit fallback to the default namespace. I recently hit an issue similar to https://github.com/cedar-policy/cedar/issues/579 myself. My use case was similar to this:

entity User {};

namespace MyService {
    entity Customer {};

    action GetCustomer appliesTo {
        principal: User,
        resource: Customer
    };
}

As a developer using Cedar my expectation was that User would be inferred to be in the default namespace automatically. I worked around this by shifting User into a namespace:

namespace MyService {
    entity User {};
    entity Customer {};

    action GetCustomer appliesTo {
        principal: User,
        resource: Customer
    };
}

This is a pain though because it makes policies less concise, now I have to use MyService::User instead of User.

No comment on the parsing trade-offs as I haven't worked with the Cedar parsing internals -- more so a comment as a Cedar user that I would definitely benefit from this feature :)

mwhicks1 commented 6 months ago

Can you comment about how these issues are handled in other languages that have namespaces?

For example, other languages have an explicit import construct that controls name resolution. Suppose I defined User in Foo::Bar and defined User in the top-level namespace. A reference to User will resolve to the first only if it is fully qualified. If you try to import it, that creates an ambiguity: User could refer to Foo::Bar::User or User. In Java, such an import would be an error. What about other languages?

andrewmwells-amazon commented 6 months ago

Happy to take this as-is. I prefer the ::Foo syntax to _cedar::Foo. I don't think we need to allow ::Foo in policies since it's easy to suggest Foo in a linter.

andrewmwells-amazon commented 6 months ago

If you try to import it, that creates an ambiguity: User could refer to Foo::Bar::User or User. In Java, such an import would be an error. What about other languages?

I'm not aware of a language where that isn't an error.

Some language allow you to alias. E.g., in Rust you can do:

mod Foo {
    pub fn my_fun() -> usize {
        0
    }
}

mod Bar {
    pub fn my_fun() -> usize {
        1
    }
}

fn main() {
    use Foo::my_fun;
    use Bar::my_fun as other_my_fun;

    println!("Hello, world! {}", my_fun());
    println!("Good bye! {}", other_my_fun());
}
hakanson commented 6 months ago

_root_ in Scala (see https://docs.scala-lang.org/style/naming-conventions.html#root) was a "fully-qualify" escape hatch.

I might prefer something like __::User over ::User - I think it needs to be different that __cedar

I also wondered why ::User only works in schemas vs also in Cedar Policies (e.g. principal is ::User)

cdisselkoen commented 6 months ago

how these issues are handled in other languages that have namespaces?

Python's LEGB rule and JavaScript's identifier resolution rules are both similar to this RFC's Change 2.

The comparison to import in Java or use in Rust is not quite apples-to-apples in my mind. import and use are about bringing items from other non-empty namespaces into the empty namespace so that they can be referenced without qualification. Cedar doesn't have a way to do this, and this RFC doesn't propose one. In both Java and Rust, to my knowledge, you can always refer to items in the empty namespace without qualification (which is what this RFC's Change 2 proposes). Java and Rust prohibit shadowing these definitions with definitions in the active namespace; Cedar, Python, and JS do not prohibit this, and changing our rules to prohibit this would be a breaking change.

shaobo-he-aws commented 6 months ago

In both Java and Rust, to my knowledge, you can always refer to items in the empty namespace without qualification (which is what this RFC's Change 2 proposes).

IIRC, in Rust, we need to use super to refer to names of the "empty namespace", should there exist name conflicts.

shaobo-he-aws commented 6 months ago

Now my understanding is that we don't want to design a hierarchy of namespaces in a schema. And hence type names like ::id make sense. It's just the anomalies in names that worry me.

cdisselkoen commented 6 months ago

IIRC, in Rust, we need to use super to refer to names of the "empty namespace", should there exist name conflicts.

I don't think this is quite accurate -- super allows you to refer to names in your parent namespace. E.g., if the active namespace is A::B::C, thensuper::foo refers to A::B::foo

https://doc.rust-lang.org/std/keyword.super.html

cdisselkoen commented 5 months ago

After some offline discussion, pulled Change 2 back into cedar-policy/cedar#579, as it's uncontroversial, better characterized as a bugfix, and doesn't need an RFC. This RFC will remain open, for discussion of Changes 1 and 3 only. Revised the RFC text accordingly.

cdisselkoen commented 4 months ago

After the above comments and offline discussion, we failed to arrive at a satisfactory syntax for Change 1, and instead the group's conclusion was in favor of disallowing this kind of collision altogether, mooting Change 1, and making Change 3 an error (at least for the case of shadowing an empty-namespace definition). Note again that Change 2 was pulled back into cedar-policy/cedar#579 where it will be addressed. As a result, I am closing this RFC and opening #70 instead as the successor.