Repository: clap-rs/clap (language: rust)

Issue:
`TypedValueParser` cannot parse/validate an `OsStr` with full error context
### Please complete the following tasks

- [X] I have searched the [discussions](https://github.com/clap-rs/clap/discussions)
- [X] I have searched the [open](https://github.com/clap-rs/clap/issues) and [rejected](https://github.com/clap-rs/clap/issues?q=is%3Aissue+label%3AS-wont-fix+is%3Aclosed) issues

### Clap Version

4.0.11 (and also 3.2)

### Describe your use case

There seems to be no way to simultaneously:

* run custom parsing or validation code on an argument (via `value_parser = ...`),
* accept `OsStr` input (non-UTF-8) rather than `str`, and
* produce errors with `context` information identifying the erroneous value etc.

There is [an implementation](https://docs.rs/clap/latest/clap/builder/trait.TypedValueParser.html#impl-TypedValueParser-for-F) of `TypedValueParser` for functions which adds context for the error but it only accepts `&str` input, and if one is writing one's own `impl TypedValueParser for …` then it is not possible to access the `Error` operations to add context. The documentation recommends `Command::error()`, but that takes a mutable `Command` which is not available from within a `TypedValueParser`.

### Describe the solution you'd like

It should be possible to construct a `clap::Error`, while implementing a parser, that has all the usual context.

### Alternatives, if applicable

A second implementation for functions that take `&OsStr` would work, but does not seem an ideal solution to me since it doesn't allow composing `TypedValueParser`s.

### Additional Context

In [previous discussion](https://github.com/clap-rs/clap/discussions/4140), epage wrote:

> But we should also support people writing their own value parsers and this is a bug hole for it. Can you create an issue?

No one filed that issue yet, so I am now. ([Another discussion post](https://github.com/clap-rs/clap/discussions/4029).)




Discussion / hints:
(I think this should be labeled a bug rather than an enhancement, since it's functionality that existed in clap 3, but the "enhancement" issue template was more fitting for what there was to say about it.)
For anyone else reading this, I would recommend double checking if you need to implement `TypedValueParser`.  Too frequently people immediately jump to implementing it when that is the harder and more verbose route

Current alternatives:
- Any `Fn(&str) -> Result<T, E>` already implements it
- `value_parser!` can adapt types that implement `FromStr`, `From<&str>`, `From<String>`, `From<&OsStr>` and `From<OsString>`
- You can take existing `TypedValueParser`s and call `TypedValueParser::map` on them
@kpreid could you expand on your use case?  I'm curious in what situations someone needs to parse `OsStr` or `OsString` types.  That'll also help in understanding if we should also look into supporting `TryFrom`, `TypedValueParser::try_map`, or a host of additional solutions on top of exposing more error context.
For this issue, there are two core problems
- We don't expose a way to set `Error::context`
- We don't expose a way to enable coloring and other similar features

Fix patch:
diff --git a/src/builder/value_parser.rs b/src/builder/value_parser.rs
index 9944f758bce..6aece7a78a3 100644
--- a/src/builder/value_parser.rs
+++ b/src/builder/value_parser.rs
@@ -599,6 +599,54 @@ where
 }
 
 /// Parse/validate argument values
+///
+/// As alternatives to implementing `TypedValueParser`,
+/// - Use `Fn(&str) -> Result<T, E>` which implements `TypedValueParser`
+/// - [`TypedValueParser::map`] to adapt an existing `TypedValueParser`
+///
+/// See `ValueParserFactory` for register `TypedValueParser::Value` with
+/// [`value_parser!`][crate::value_parser].
+///
+/// # Example
+///
+#[cfg_attr(not(feature = "error-context"), doc = " ```ignore")]
+#[cfg_attr(feature = "error-context", doc = " ```")]
+/// # use clap::error::ErrorKind;
+/// # use clap::error::ContextKind;
+/// # use clap::error::ContextValue;
+/// #[derive(Clone)]
+/// struct Custom(u32);
+///
+/// #[derive(Clone)]
+/// struct CustomValueParser;
+///
+/// impl clap::builder::TypedValueParser for CustomValueParser {
+///     type Value = Custom;
+///
+///     fn parse_ref(
+///         &self,
+///         cmd: &clap::Command,
+///         arg: Option<&clap::Arg>,
+///         value: &std::ffi::OsStr,
+///     ) -> Result<Self::Value, clap::Error> {
+///         let inner = clap::value_parser!(u32);
+///         let val = inner.parse_ref(cmd, arg, value)?;
+///
+///         const INVALID_VALUE: u32 = 10;
+///         if val == INVALID_VALUE {
+///             let mut err = clap::Error::new(ErrorKind::ValueValidation)
+///                 .with_cmd(cmd);
+///             if let Some(arg) = arg {
+///                 err.insert(ContextKind::InvalidArg, ContextValue::String(arg.to_string()));
+///             }
+///             err.insert(ContextKind::InvalidValue, ContextValue::String(INVALID_VALUE.to_string()));
+///             return Err(err);
+///         }
+///
+///         Ok(Custom(val))
+///     }
+/// }
+/// ```
 pub trait TypedValueParser: Clone + Send + Sync + 'static {
     /// Argument's value type
     type Value: Send + Sync + Clone;
diff --git a/src/error/mod.rs b/src/error/mod.rs
index 12622924767..932e9065073 100644
--- a/src/error/mod.rs
+++ b/src/error/mod.rs
@@ -98,6 +98,55 @@ impl<F: ErrorFormatter> Error<F> {
         self.with_cmd(cmd)
     }
 
+    /// Create an error with a pre-defined message
+    ///
+    /// See also
+    /// - [`Error::insert`]
+    /// - [`Error::with_cmd`]
+    ///
+    /// # Example
+    ///
+    #[cfg_attr(not(feature = "error-context"), doc = " ```ignore")]
+    #[cfg_attr(feature = "error-context", doc = " ```")]
+    /// # use clap::error::ErrorKind;
+    /// # use clap::error::ContextKind;
+    /// # use clap::error::ContextValue;
+    ///
+    /// let cmd = clap::Command::new("prog");
+    ///
+    /// let mut err = clap::Error::new(ErrorKind::ValueValidation)
+    ///     .with_cmd(&cmd);
+    /// err.insert(ContextKind::InvalidArg, ContextValue::String("--foo".to_owned()));
+    /// err.insert(ContextKind::InvalidValue, ContextValue::String("bar".to_owned()));
+    ///
+    /// err.print();
+    /// ```
+    pub fn new(kind: ErrorKind) -> Self {
+        Self {
+            inner: Box::new(ErrorInner {
+                kind,
+                #[cfg(feature = "error-context")]
+                context: FlatMap::new(),
+                message: None,
+                source: None,
+                help_flag: None,
+                color_when: ColorChoice::Never,
+                color_help_when: ColorChoice::Never,
+                backtrace: Backtrace::new(),
+            }),
+            phantom: Default::default(),
+        }
+    }
+
+    /// Apply [`Command`]'s formatting to the error
+    ///
+    /// Generally, this is used with [`Error::new`]
+    pub fn with_cmd(self, cmd: &Command) -> Self {
+        self.set_color(cmd.get_color())
+            .set_colored_help(cmd.color_help())
+            .set_help_flag(format::get_help_flag(cmd))
+    }
+
     /// Apply an alternative formatter to the error
     ///
     /// # Example
@@ -138,6 +187,13 @@ impl<F: ErrorFormatter> Error<F> {
         self.inner.context.get(&kind)
     }
 
+    /// Insert a piece of context
+    #[inline(never)]
+    #[cfg(feature = "error-context")]
+    pub fn insert(&mut self, kind: ContextKind, value: ContextValue) -> Option<ContextValue> {
+        self.inner.context.insert(kind, value)
+    }
+
     /// Should the message be written to `stdout` or not?
     #[inline]
     pub fn use_stderr(&self) -> bool {
@@ -216,34 +272,11 @@ impl<F: ErrorFormatter> Error<F> {
         self.formatted().into_owned()
     }
 
-    fn new(kind: ErrorKind) -> Self {
-        Self {
-            inner: Box::new(ErrorInner {
-                kind,
-                #[cfg(feature = "error-context")]
-                context: FlatMap::new(),
-                message: None,
-                source: None,
-                help_flag: None,
-                color_when: ColorChoice::Never,
-                color_help_when: ColorChoice::Never,
-                backtrace: Backtrace::new(),
-            }),
-            phantom: Default::default(),
-        }
-    }
-
     #[inline(never)]
     fn for_app(kind: ErrorKind, cmd: &Command, styled: StyledStr) -> Self {
         Self::new(kind).set_message(styled).with_cmd(cmd)
     }
 
-    pub(crate) fn with_cmd(self, cmd: &Command) -> Self {
-        self.set_color(cmd.get_color())
-            .set_colored_help(cmd.color_help())
-            .set_help_flag(format::get_help_flag(cmd))
-    }
-
     pub(crate) fn set_message(mut self, message: impl Into<Message>) -> Self {
         self.inner.message = Some(message.into());
         self


Test patch: