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

Consistency pass over error messages #327

Merged
merged 7 commits into from
Oct 3, 2023
Merged

Conversation

khieta
Copy link
Contributor

@khieta khieta commented Sep 26, 2023

Description of changes

Another pass at standardizing error messages. My changes are meant to align with the following rules (loosely inspired by https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-structure):

  • Error messages should avoid capitalization and periods. If multiple sentences are needed then use grammatical capitalization and punctuation only on the message’s interior. Example: wrong number of arguments in extension function application. Expected {}, got {} (note the initial lowercase letter and lack of final period).
  • Error messages should not include line breaks.
  • User-provided input (e.g. attributes, entity uids, policy ids) should be surrounded with backticks or otherwise clearly separated from the rest of the message. Use of backticks is preferred. Examples: undeclared action `foo` , undeclared entity type(s): {"Foo"}
  • Related error messages should be concatenated with colons. Example: error occurred while evaluating policy `policy0`: `User::\"alive\"` does not have the attribute `foo`
  • Error messages should not use contractions (e.g., “don’t”). (This last one isn't as important as the others -- just my mood today.)

Issue #, if available

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar Dafny model or DRT infrastructure.

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@shaobo-he-aws shaobo-he-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link

@max2me max2me left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor suggestions/nitpicks

cedar-policy-core/src/entities.rs Outdated Show resolved Hide resolved
@@ -778,7 +778,7 @@ impl std::str::FromStr for Expr {
#[derive(Debug, Clone, Error)]
pub enum SubstitutionError {
/// The supplied value did not match the type annotation on the unknown.
#[error("Expected a value of type {expected}, got a value of type {actual}")]
#[error("expected a value of type {expected}, got a value of type {actual}")]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the {expected} and {actual} types be surrounded with backticks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been surrounding types with backticks -- see my comment below.

cedar-policy-core/src/entities/json/err.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/parser/err.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/parser/err.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/parser/err.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/str_checks.rs Outdated Show resolved Hide resolved
@@ -212,7 +212,7 @@ impl std::error::Error for TypeError {}
pub enum TypeErrorKind {
/// The typechecker expected to see a subtype of one of the types in
/// `expected`, but saw `actual`.
#[error("Unexpected type. Expected {} but saw {}",
#[error("unexpected type: expected {} but saw {}",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should types be surrounded with backticks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to surround types with backticks since they are not verbatim user input, and I felt that expected bool but saw string was just as clear as expected `bool` but saw `string` . Open to second opinions.

Note: Your comment made me realize that we weren't following the rules when printing types (we were printing entity of type Foo rather than entity of type `Foo` ). Fixing this required changing a bunch of the corpus tests, which is why the diff in my recent commit is so large.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, here is how we currently print types:

  • bool
  • long
  • string
  • set
  • record
  • (entity of unspecified type)
  • (entity of type `Foo`) where Foo is some user-specified name

cedar-policy-core/src/entities/err.rs Show resolved Hide resolved
cedar-policy-core/src/extensions/ipaddr.rs Outdated Show resolved Hide resolved
@khieta khieta merged commit 4c8c0c4 into main Oct 3, 2023
7 checks passed
@khieta khieta deleted the nit/khieta/error-messages branch October 3, 2023 13:32
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

Successfully merging this pull request may close these issues.

5 participants