Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More restrictions on type shadowing / collisions #1139

Closed
1 of 2 tasks
cdisselkoen opened this issue Aug 9, 2024 · 2 comments · Fixed by #1150
Closed
1 of 2 tasks

More restrictions on type shadowing / collisions #1139

cdisselkoen opened this issue Aug 9, 2024 · 2 comments · Fixed by #1150
Assignees
Labels
feature-request This issue requets a substantial new feature

Comments

@cdisselkoen
Copy link
Contributor

Category

Cedar language or syntax features/changes

Describe the feature you'd like to request

We recently decided to disallow Set, Record, Entity, and Extension as common type names, reverting a change that was originally planned for 4.0. (#1070)

Since that issue was created, #1060 improved our typename resolution algorithms considerably, with two consequences relevant for this issue:

  1. on main (but in no release as of this writing), primitive type names like Long and String are also allowed as common type names
  2. on main (but in no release as of this writing), both primitive type names (Long, String, etc) and the other reserved names (Set, Entity, etc) are allowed as common type names in nonempty namespaces. Reserve Set, Record, Entity, Extension in schemas #1070 was ambiguous about whether it applies only to common types in the empty namespaces, or to common types in all namespaces.

This issue asks that we revert the two consequences listed above, returning to the released behavior. Specifically, both primitive type names (Long, String, etc) and the other reserved names in #1070 (Set, Entity, etc) should be disallowed as common type names, in all namespaces.

This issue specifically does not ask to disallow any of these names as entity type names, only common type names. All of these are allowed today (in released versions of Cedar) as entity type names, so that would be a breaking change. One consequence of this is that we will still need the shadowing checks proposed in RFC 70.

Describe alternatives you've considered

We could make more of these names legal as common types, but that would make it harder for folks programmatically consuming the JSON schema format, and there doesn't seem to be a good reason to allow these names as common types (in any namespace), as it's more likely to create confusion than be useful.

Additional context

No response

Is this something that you'd be interested in working on?

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change
@cdisselkoen cdisselkoen added backlog feature-request This issue requets a substantial new feature labels Aug 9, 2024
@cdisselkoen cdisselkoen self-assigned this Aug 9, 2024
@khieta khieta assigned shaobo-he-aws and unassigned cdisselkoen Aug 20, 2024
@shaobo-he-aws
Copy link
Contributor

Upon checking the status quo, it seems that we do not forbid declarations of common types of such names. Instead, we disallow (i.e., throw an unknown field ... error) when they are referenced as common types. I assume that we want to error on such declarations.

@cdisselkoen
Copy link
Contributor Author

I believe that's the status quo only on current main. The 3.3.0 status quo, in my understanding (I haven't confirmed), is that we do forbid declarations of common types of these names?

shaobo-he-aws added a commit that referenced this issue Aug 26, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
@shaobo-he-aws shaobo-he-aws linked a pull request Aug 26, 2024 that will close this issue
3 tasks
@shaobo-he-aws shaobo-he-aws mentioned this issue Aug 26, 2024
3 tasks
shaobo-he-aws added a commit that referenced this issue Aug 28, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Co-authored-by: Craig Disselkoen <cdiss@amazon.com>
@github-project-automation github-project-automation bot moved this from Pending to Done in Cedar 4.0 Status Aug 28, 2024
oflatt pushed a commit to oflatt/cedar that referenced this issue Aug 29, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Co-authored-by: Craig Disselkoen <cdiss@amazon.com>
oflatt pushed a commit to oflatt/cedar that referenced this issue Aug 29, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Co-authored-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: oflatt <oflatt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requets a substantial new feature
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants