From 7aaa845c09142eaf7f1c6f62929153e62edb46cb Mon Sep 17 00:00:00 2001 From: Techassi Date: Wed, 21 May 2025 16:58:00 +0200 Subject: [PATCH 1/6] docs(code-style-guide): Add section about unwraps --- .../contributor/pages/code-style-guide.adoc | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc index 0c46ab02e..3de63f46b 100644 --- a/modules/contributor/pages/code-style-guide.adoc +++ b/modules/contributor/pages/code-style-guide.adoc @@ -531,6 +531,41 @@ enum Error { . `unable to read config file from ...` to indicate that the file could not be loaded (for example because the file doesn't exist). . `unable to parse value ...` to indicate that parsing a user provided value failed (for example because it didn't conform to the expected syntax). +=== Using `unwrap` + +Generally, it is not recommended to use `unwrap` (or any other method which consumes the error) in any fallible code path. +Instead, proper error handling like above should be used. +There are however cases, where it is fine to use `unwrap` or friends. + +One such an example is when compiling regular expressions inside const/static environments. +For such cases code must use `expect` instead of `unwrap` to provide additional context why a particular piece of code should never fail. + +// Do we want to mention that this is enforced via clippy and that we actually enable that lint in our repos? + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +static VERSION_REGEX: LazyLock = LazyLock::new(|| { + Regex::new(r".*").expect("failed to compile regex") +}); +---- + +==== + +[WARNING.code-rule,caption=Examples of incorrect code for this rule] +==== + +[source,rust] +---- +static VERSION_REGEX: LazyLock = LazyLock::new(|| { + Regex::new(r".*").unwrap() +}); +---- + +==== + == String formatting === Named versus unnamed format string identifiers @@ -737,3 +772,27 @@ mod test { ---- ==== + +=== Using `unwrap` + +The usage of `unwrap` in unit tests is not recommended for the same reasons as mentioned above, but allowed. + +[TIP.code-rule,caption=Examples of correct code for this rule] +==== + +[source,rust] +---- +#[test] +fn deserialize() { + let input: String = serde_yaml::from_str("my string").expect("input string must deserialize"); + assert_eq(&input, "my string"); +} + +#[test] +fn serialize() { + let serialized = serde_yaml::to_string(&String::from("my string")).unwrap(); + println!("{serialized}"); +} +---- + +==== From 0929018bb3a54027be5228cbb44e7f94382731bc Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 26 May 2025 08:49:09 +0200 Subject: [PATCH 2/6] chore: Apply suggestions Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- modules/contributor/pages/code-style-guide.adoc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc index 3de63f46b..427c34e67 100644 --- a/modules/contributor/pages/code-style-guide.adoc +++ b/modules/contributor/pages/code-style-guide.adoc @@ -533,12 +533,14 @@ enum Error { === Using `unwrap` -Generally, it is not recommended to use `unwrap` (or any other method which consumes the error) in any fallible code path. -Instead, proper error handling like above should be used. +The `unwrap` function must not be used in any fallible code paths. +Instead, proper error handling like above should be used, unless there is a valid reason to use `expect` described below. +Using https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or[`unwrap_or`], https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default[`unwrap_or_default`] or https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_else[`unwrap_or_else`] is allowed because these functions will not panic. + There are however cases, where it is fine to use `unwrap` or friends. -One such an example is when compiling regular expressions inside const/static environments. -For such cases code must use `expect` instead of `unwrap` to provide additional context why a particular piece of code should never fail. +The `expect` function can be used when external factors cannot influence whether a panic will happen. For example, when compiling regular expressions inside const/static environments. +For such cases code must use `expect` instead of `unwrap` to provide additional context for why a particular piece of code should never fail. // Do we want to mention that this is enforced via clippy and that we actually enable that lint in our repos? @@ -775,7 +777,7 @@ mod test { === Using `unwrap` -The usage of `unwrap` in unit tests is not recommended for the same reasons as mentioned above, but allowed. +The usage of `unwrap` in unit tests is also not allowed for the same reasons as mentioned above. [TIP.code-rule,caption=Examples of correct code for this rule] ==== From 1c9a496a444735195ce152164fe81ac6a0d42610 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 26 May 2025 08:58:51 +0200 Subject: [PATCH 3/6] chore(code-style-guide): Use link definitions --- modules/contributor/pages/code-style-guide.adoc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc index 427c34e67..4338e686e 100644 --- a/modules/contributor/pages/code-style-guide.adoc +++ b/modules/contributor/pages/code-style-guide.adoc @@ -533,11 +533,13 @@ enum Error { === Using `unwrap` +:unwrap_or: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or +:unwrap_or_default: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default +:unwrap_or_else: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_else + The `unwrap` function must not be used in any fallible code paths. Instead, proper error handling like above should be used, unless there is a valid reason to use `expect` described below. -Using https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or[`unwrap_or`], https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default[`unwrap_or_default`] or https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_else[`unwrap_or_else`] is allowed because these functions will not panic. - -There are however cases, where it is fine to use `unwrap` or friends. +Using link:{unwrap_or}[`unwrap_or`], link:{unwrap_or_default}[`unwrap_or_default`] or link:{unwrap_or_else}[`unwrap_or_else`] is allowed because these functions will not panic. The `expect` function can be used when external factors cannot influence whether a panic will happen. For example, when compiling regular expressions inside const/static environments. For such cases code must use `expect` instead of `unwrap` to provide additional context for why a particular piece of code should never fail. From 5de8306ee7a1b0704bfabebe46023eed9d62af81 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 26 May 2025 08:59:20 +0200 Subject: [PATCH 4/6] chore(code-style-guide): Add bad unwrap example for tests --- modules/contributor/pages/code-style-guide.adoc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc index 4338e686e..a597cc8d0 100644 --- a/modules/contributor/pages/code-style-guide.adoc +++ b/modules/contributor/pages/code-style-guide.adoc @@ -791,7 +791,15 @@ fn deserialize() { let input: String = serde_yaml::from_str("my string").expect("input string must deserialize"); assert_eq(&input, "my string"); } +---- + +==== + +[WARNING.code-rule,caption=Examples of incorrect code for this rule] +==== +[source,rust] +---- #[test] fn serialize() { let serialized = serde_yaml::to_string(&String::from("my string")).unwrap(); From 7dc644e8634f5ff2848cc249d0541dfef6f5ca07 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 2 Jun 2025 16:00:03 +0200 Subject: [PATCH 5/6] chore: Apply suggestions from code review Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- modules/contributor/pages/code-style-guide.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc index a597cc8d0..9bf1e0bda 100644 --- a/modules/contributor/pages/code-style-guide.adoc +++ b/modules/contributor/pages/code-style-guide.adoc @@ -537,7 +537,7 @@ enum Error { :unwrap_or_default: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default :unwrap_or_else: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_else -The `unwrap` function must not be used in any fallible code paths. +The `unwrap` function must not be used in any code. Instead, proper error handling like above should be used, unless there is a valid reason to use `expect` described below. Using link:{unwrap_or}[`unwrap_or`], link:{unwrap_or_default}[`unwrap_or_default`] or link:{unwrap_or_else}[`unwrap_or_else`] is allowed because these functions will not panic. @@ -788,7 +788,7 @@ The usage of `unwrap` in unit tests is also not allowed for the same reasons as ---- #[test] fn deserialize() { - let input: String = serde_yaml::from_str("my string").expect("input string must deserialize"); + let input: String = serde_yaml::from_str("my string").expect("constant input string must deserialize"); assert_eq(&input, "my string"); } ---- From a2ab5a6129a8aeeb95bf3fc01d8cbefbdfbab030 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 2 Jun 2025 16:12:12 +0200 Subject: [PATCH 6/6] chore: Apply suggestion Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- modules/contributor/pages/code-style-guide.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/contributor/pages/code-style-guide.adoc b/modules/contributor/pages/code-style-guide.adoc index 9bf1e0bda..696fbf09a 100644 --- a/modules/contributor/pages/code-style-guide.adoc +++ b/modules/contributor/pages/code-style-guide.adoc @@ -552,7 +552,7 @@ For such cases code must use `expect` instead of `unwrap` to provide additional [source,rust] ---- static VERSION_REGEX: LazyLock = LazyLock::new(|| { - Regex::new(r".*").expect("failed to compile regex") + Regex::new(r".*").expect("valid regular expression") }); ----