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

Improve parse error message on incorrect template slot #391

Closed
john-h-kastner-aws opened this issue Oct 27, 2023 · 5 comments
Closed

Improve parse error message on incorrect template slot #391

john-h-kastner-aws opened this issue Oct 27, 2023 · 5 comments

Comments

@john-h-kastner-aws
Copy link
Contributor

john-h-kastner-aws commented Oct 27, 2023

For a policy permit(principal == ?resource, action, resource) the parse error message is expected a entity uid or template slot, found a ??resource statement. First, it's printing ??resource instead of ?resource (easy fix) and second it's confusing that it expects a templates slot even though it found ?resource. It should clarify that is specifically expects ?principal.

@john-h-kastner-aws john-h-kastner-aws added bug Something isn't working. This is as high priority issue. good-first-issue Good for newcomers. A smaller issue that someone new to the Cedar codebase should be able to tackle and removed bug Something isn't working. This is as high priority issue. labels Oct 27, 2023
@svanderbleek
Copy link
Contributor

svanderbleek commented Nov 8, 2023

I found where the ParseError is happening but the context is lost on me. I see the ? being inserted but can I simply remove that there without breaking other uses? Then I'm not sure how to clarify as described. Maybe this is a little complicated for a first issue.

@john-h-kastner-aws
Copy link
Contributor Author

The parser and following CST to AST conversion is a little convoluted, so perhaps not a first issue.

For the ?, I'm confident that's just a matter of deleting the extra ?.

For the rest of the error text improvement, it will come down to tracking down where the value of T::err_str() is coming from. It looks like there's some type level shenanigans happening there.

@john-h-kastner-aws john-h-kastner-aws added backlog and removed good-first-issue Good for newcomers. A smaller issue that someone new to the Cedar codebase should be able to tackle labels Nov 8, 2023
@svanderbleek
Copy link
Contributor

@john-h-kastner-aws I believe it is here.

@svanderbleek
Copy link
Contributor

By taking var to string here we can see the mismatch, how to communicate that exactly I'm not sure though.

@cdisselkoen
Copy link
Contributor

Resolved by #411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants