-
Notifications
You must be signed in to change notification settings - Fork 9
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
Doc comments #63
base: main
Are you sure you want to change the base?
Doc comments #63
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,226 @@ | ||
# Policy and Schema Doc Comments | ||
|
||
## Related issues and PRs | ||
|
||
- Reference Issues: | ||
- Implementation PR(s): | ||
|
||
## Timeline | ||
|
||
- Started: 2024-04-12 | ||
|
||
## Summary | ||
|
||
Cedar should support documentation comments (doc comments) on both policy and schema elements. | ||
|
||
Doc comments are a refinement to our current comments, distinguished by a third `/` and restricted in where they can appear. | ||
These comments are designed specifically to support rendering documentation as HTML pages (or similar), but this RFC does not propose what the final rendered form should be. | ||
|
||
## Basic example | ||
|
||
A policy can be documented by placing a doc comment, indicated by `///` following the style of Rust, immediately before the policy and on scope conditions. | ||
|
||
```cedar | ||
/// Allow users to create list and see the lists they own. | ||
@id("policy0") | ||
permit ( | ||
principal, | ||
/// The `GetLists` action only returns the lists owned by that users, so this | ||
/// policy does not allow users to see lists created by others. | ||
action in [Action::"CreateList", Action::"GetLists"], | ||
resource == Application::"TinyTodo" | ||
); | ||
``` | ||
|
||
Doc comments may also be placed immediately prior schema elements to document the purpose of entity types and actions and before each attribute for an entity type or action context. | ||
|
||
```cedarschema | ||
/// This entity defines our central user type | ||
entity User { | ||
/// The location where this user works. E.g., ABC17 or XYZ77. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In go doc comments are just comments Here's an excerpt:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd also prefer a different syntax for doc comments vs. normal comments. Otherwise the two will only be distinguished by location in the policy/schema, which might make it difficult to figure out why a comment is (or isn't) rendering in the auto-generated documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw it referenced but can you add the link to Rust Doc comments in the RFC. C# also uses |
||
location : String, | ||
/// A numeric level describing this users job. | ||
jobLevel : Long, | ||
} | ||
``` | ||
|
||
## Motivation | ||
|
||
Even though the syntax of Cedar policies is designed to be easy to read and understand, policies will still require some amount of documentation. | ||
Cedar naturally supports comments so policy authors can leave remarks in their code, but these comments do not exist in our policy JSON format or in our internal AST. | ||
Automated generation of documentation from comments is a common capability found in almost all modern programming languages, so we should update Cedar to include automatically processable comments that support this use case. | ||
|
||
This RFC is largely motived by [this comment on RFC 48](https://github.com/cedar-policy/rfcs/pull/48#issuecomment-1937735924). | ||
It also addresses the request in [Cedar issue 660](https://github.com/cedar-policy/cedar/issues/660) for comments in the JSON policy format. | ||
|
||
## Detailed design | ||
|
||
Doc comments are designed to be very similar to annotations, and could reasonably be implemented as syntactic sugar for a special annotation annotation key. | ||
The primary difference is that, in order to fully satisfy the documentation use case, doc comments need to appear in more positions than are supported for annotations. | ||
We may choose to extend annotation to be supported in all positions proposed for doc comments, but this is not a necessary extension to accept this RFC. | ||
|
||
Doc comments will be defined by this grammar. | ||
|
||
``` | ||
DocCommentLine ::= '///' [^\r\n]* [\r\n] | ||
DocComment ::= {DocCommentLine} | ||
``` | ||
|
||
### Policies | ||
|
||
Documentation for the policy as a whole appears before the policy effect. | ||
It may be anywhere in relation to the annotations on that policy. | ||
Documentation for individual policy scope clauses must appear immediately before that clause. | ||
|
||
The policy grammar is now: | ||
|
||
``` | ||
Policy ::= {(Annotation|DocComment)} Effect '(' Scope ')' {Conditions} ';' | ||
Scope ::= [DocComment] Principal ',' [DocComment] Action ',' [DocComment] Resource | ||
``` | ||
|
||
### Schema | ||
|
||
Documentation for a declaration in a schema appears immediately before the `entity`, `action`, `type`, or `namespace` keyword at the start of the declaration. | ||
The fields of the attributes in a record type can also be documented with the comment appearing directly before the attribute name. | ||
A record type might be written for an entity's attributes, action's context, or common type definition, or it might be nested inside of some other type. | ||
Doc comments are supported in all of these locations. | ||
|
||
The schema grammar is now: | ||
|
||
``` | ||
Namespace := ([DocComment] 'namespace' Path '{' {Decl} '}') | Decl | ||
Decl := [DocComment] (Entity | Action | TypeDecl) | ||
AttrDecls := [DocComment] Name ['?'] ':' Type [',' | ',' AttrDecls] | ||
``` | ||
|
||
### Improperly placed comments | ||
|
||
Comments starting with the `///` that are not in a legal position for doc comments are an error. | ||
The definition of comments is accordingly updated to | ||
|
||
``` | ||
COMMENT ::= '//' ([^/\r\n] [^\r\n]*)? [\r\n] | ||
``` | ||
|
||
This makes the RFC a breaking change for any users who wrote comments in this style, but I believe that is preferable to silently ignoring what a user intended to be a doc comment. | ||
For an initial and backwards compatible change, improperly placed doc comments could be allowed with a warning. | ||
We can then make this an error in a future major version of Cedar, or never make it an error if the break is not acceptable. | ||
|
||
### Alternative doc comment syntax | ||
|
||
Choosing to use Rust style doc comments feels natural after developing Cedar in Rust, but we should consider alternative syntax options. | ||
Some options include using block style doc comments (`/** .. */`), using another common comment character for doc comments (e.g., `#`), and picking a different third character following the usual two slashes (e.g., `//@` to emphasize the connection with annotations). | ||
|
||
### JSON Formats | ||
|
||
Doc comments will be represented in the policy and schema JSON formats by adding a `doc` property to relevant nodes. | ||
|
||
In a policy: | ||
|
||
```json | ||
{ | ||
"doc": "Allow users to create list and see the lists they own.", | ||
"effect": "permit", | ||
"principal": { | ||
"op": "All" | ||
}, | ||
"action": { | ||
"doc": "The `GetLists` action only returns the lists owned by that users, so this policy does not allow users to see lists created by others", | ||
"op": "in", | ||
"entities": [ | ||
{ "type": "Action", "id": "CreateList" }, | ||
{ "type": "Action", "id": "GetList" } | ||
] | ||
}, | ||
"resource": { | ||
"op": "==", | ||
"entity": { "type": "Application", "id": "TinyTodo" } | ||
} | ||
} | ||
``` | ||
|
||
and in a schema: | ||
|
||
```json | ||
{ | ||
"entityTypes": { | ||
"User": { | ||
"doc": "This entity defines our central user type", | ||
"shape": { | ||
"type": "Record", | ||
"attributes": { | ||
"location": { | ||
"doc": "The location where this user works. E.g., ABC17 or XYZ77.", | ||
"type": "String" | ||
}, | ||
"jobLevel": { | ||
"doc": "A numeric level describing this users job.", | ||
"type": "Long" | ||
}, | ||
} | ||
} | ||
} | ||
} | ||
} | ||
``` | ||
|
||
## Drawbacks | ||
|
||
1. This is a significant change to the parsers for policies and schema, both in their human readable and JSON formats. | ||
There is always a possibility of mistakes when making changes to our parsers, especially given that they are not modeled and differentially tested. | ||
2. This will eventually be a breaking change to the policy format. | ||
Although, as noted above, we don't need to make this change immediately, and we could even opt to never make the change. | ||
3. We are likely to find that users want to add doc comments in a position we didn't anticipate. We will be able to introduce doc comments in more positions in the future without a breaking change, so argument over where else we should support doc comments should ideally not delay acceptance of this RFC if we are confident that the currently proposed positions should be supported. | ||
4. It partially duplicates the functionality of annotations, particularly given that we can use multi-line strings as annotation values. | ||
|
||
## Alternatives | ||
|
||
### Regular comments are good enough | ||
|
||
Our position would be that documentation should be written in standard Cedar comments. | ||
If someone wants to attach a particular meaning to and write documentation tools around comments starting with `///`, they are free to do so, but we do not explicitly support this. | ||
This requires no changes to Cedar, and is maximally flexible (standard comments can appear anywhere). | ||
|
||
Asking users to develop their own documentation tools could resulted in multiple ad-hoc tools that are likely to fail to handle certain edge cases (e.g., `///` in a string) unless they substantially re-implement the Cedar parser. | ||
Alternatively, we could provide the tools for generating documentation in this manner, but this implies standardizing the format and position of these comments, which is essentially what this RFC proposes, just without considering JSON formats. | ||
|
||
Comments do not exist in the JSON format, so this alternative provides nothing for documenting a JSON formatted policy or schema. | ||
|
||
### Annotations as documentation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting option to me, but I still prefer the look of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The doc comment syntax being syntactic sugar under the hood is something I had in mind while writing this. It requires a substantial extension to extensions beyond ever RFC #48, but perhaps people will find other application of generalized annotations. |
||
|
||
We use the existing annotation feature to hold documentation. | ||
Annotations already supports multi-line strings, so documentation can still be written without too much syntax overhead. | ||
|
||
```cedar | ||
@doc(" | ||
Allow users to create list and see the lists they own. | ||
|
||
The `GetLists` action only returns the lists owned by that users, so this policy | ||
does not allow users to see lists created by others. | ||
") | ||
@id("policy0") | ||
permit ( | ||
principal, | ||
action in [Action::"CreateList", Action::"GetLists"], | ||
resource == Application::"TinyTodo" | ||
); | ||
``` | ||
|
||
The Cedar AST (and JSON formatted EST) contains attributes, making it unambiguous what a comment is documenting, | ||
and the Cedar Rust SDK provides functions for extracting annotations which could be used to build a documentation tool without re-implementing the Cedar parser. | ||
|
||
The design of annotations only supports annotations at the start of policies, but we could extend this to support annotations in new positions as easily as we could introduce doc comments in those positions. | ||
We would need to accept at least [RFC 48](https://github.com/cedar-policy/rfcs/pull/48) to document schema elements. | ||
|
||
### Existing tools for literate programming | ||
|
||
Rather of building a custom solution for Cedar, we could point users to existing literate programming tools. | ||
One powerful option is [Entangled](https://entangled.github.io/) which would allow users to structure their documentation however they like. | ||
|
||
The drawback to this alternative is that it would require that users install a second tool distributed separately from Cedar. | ||
Different users may also decide to use different literate programming tools, meaning their policy and schema source may not be directly compatible without a pre-processing pass to remove the literate comments. | ||
This concern could be addressed by explicitly recommending particular tools, or making Cedar parsers directly depend on Rust crates implementing literate programming. | ||
|
||
Because this alternative amounts to using some tool as pre-processing pass before passing policies and schema to Cedar, it remains available to users regardless of what opinionated stance we choose for the Cedar language. | ||
Users who want more sophisticated literate programming features can always use their tool of choice. |
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.
what about before
principal
andresource
in anappliesTo
block?(I do acknowledge the comment in Drawbacks that we need not block this RFC on discussing additional positions for doc comments, and additional ones can be added later as a non-breaking change. We can punt on my comment here if we want.)
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.
These would also be good additions. I wasn't sure where to draw the line for what to include in this RFC. Hopefully extra comments positions can be added later without another RFC.