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

Support String type in ContextError #744

Open
2 tasks done
Nukesor opened this issue Feb 15, 2025 · 5 comments
Open
2 tasks done

Support String type in ContextError #744

Nukesor opened this issue Feb 15, 2025 · 5 comments
Labels
A-error Area: error reporting C-enhancement Category: Raise on the bar on expectations

Comments

@Nukesor
Copy link
Contributor

Nukesor commented Feb 15, 2025

Please complete the following tasks

winnow version

0.7.2

Describe your use case

I'm currently writing a parser that detects a rather large set of keywords.
The keywords are represented as multiple enum types, that all implement FromStr.
Multiple enums were chosen it makes it easier to look at certain sets of keywords in the scope of a specific topic.

When the parser cannot match any of the keywords, an error should be thrown that shows the list of all available keywords in the current scope. To do this programatically, the idea is to use strum's VariantNames. This provides an easy and most importantly reliable automatic way of concatenating all keywords of the utilized keywords. Manual solutions are likely to go out of sync over time, as one has to adjust multiple error messages and go through a rather large list of keywords to keep them in sync.

In practice the error message creation would look something like this:

let mut concatenated = [
    SourceKeyword::VARIANTS,
    MetaKeyword::VARIANTS,
    RelationKeyword::VARIANTS,
]
.concat()
.join(", ");
concatenated.insert_str(0, "Expected one of the allowed properties: [");
concatenated.push(']');

The discussion started over here in a different issue:
#496 (comment)

Right now it's possible to create a custom StringContext type in the likes of this:

#[derive(Clone, Debug)]
pub struct StringContext(String);

impl StringContext {
    pub fn new(value: impl Into<String>) -> Self {
        Self(value.into())
    }
}

impl std::fmt::Display for StringContext {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.0)
    }
}

Which can be called and used like this:

    fn keyword_parser(input: &mut &str) -> ModalResult<MyKeyword,  ContextError<StringContext>> {

This works fine as long as one works with ContextErrors. But as soon as parse instead of parse_next is called, which returns a ParseError instead, this is no longer compatible:

Display is implemented for ParseError, with E being the inner error, which is the ContextError<StringContext> in this case.

impl<I, E> core::fmt::Display for ParseError<I, E>
where
    I: AsBStr,
    E: core::fmt::Display,
{

Currently, Display is only implemented for ContextError<StrContext>:

impl crate::lib::std::fmt::Display for ContextError<StrContext> {

Since it's a foreign type, I'm not allowed to impl Display for ContextError<StringContext>.

Describe the solution you'd like

An easy way to use String type context errors that may be used in parse, so that we can write dynamic error messages that're build during runtime.

The nicest solution would be to provide an StringContext context type upstream. That would make it very ergonomic to just use Strings in error messages and I hope it shouldn't be too much of a hassle to implement.

Alternatives, if applicable

I tried using const functions to create static strings, but sadly the Rust language isn't quite there yet, or at least I couldn't find a non-obstruse way to do that.

While writing this I came up with the idea that once_cell could be a solution for my problem at hand. But even if that works, this feature would still be a quality of life improvement, as it would allow easier debugging and error message creation without having to come up with a way of creating static strings first.

Another alternative would be to implement a custom Error type, but that feels like overkill to achieve this.

Additional Context

No response

@Nukesor Nukesor added the C-enhancement Category: Raise on the bar on expectations label Feb 15, 2025
@Nukesor
Copy link
Contributor Author

Nukesor commented Feb 15, 2025

I tried the OnceLock solution and it works out. It requires a Box::leak, which should be fine in this context, but it still feels a bit dirty to have to do it this way:

/// A static error message string that contains all keywords that're valid in the current section.
static VALID_KEYWORDS_ERROR: OnceLock<&'static str> = OnceLock::new();

fn get_valid_keywords_error() -> &'static str {
    VALID_KEYWORDS_ERROR.get_or_init(|| {
        let mut concatenated = [
            SourceKeyword::VARIANTS,
            MetaKeyword::VARIANTS,
            RelationKeyword::VARIANTS,
        ]
        .concat()
        .join(", ");
        concatenated.insert_str(0, "one of the allowed properties: [");
        concatenated.push(']');

        Box::leak(concatenated.into_boxed_str())
    })
}

@Nukesor
Copy link
Contributor Author

Nukesor commented Feb 17, 2025

I just stumbled over another use-case for such a string context type.
In the alpm-types repository, there's the Checksum type that wraps a bunch of different Digests.

If we were to write a simple parser for that, it would be nice to have the digest type and the expected length for the given digest in the error message.

@epage epage added the A-error Area: error reporting label Feb 17, 2025
@epage
Copy link
Collaborator

epage commented Feb 17, 2025

I tried the OnceLock solution and it works out. It requires a Box::leak, which should be fine in this context, but it still feels a bit dirty to have to do it this way:

While leaking or not probably doesn't make a difference, you might not need to. For example, clap uses OnceLock<String> to get a &'static str, see https://github.com/clap-rs/clap/blob/246d972a6c5bef724ce5fbaac4a97cffa10a77c8/clap_derive/src/item.rs#L578-L584

@epage
Copy link
Collaborator

epage commented Feb 17, 2025

An easy way to use String type context errors that may be used in parse, so that we can write dynamic error messages that're build during runtime.

The nicest solution would be to provide an StringContext context type upstream. That would make it very ergonomic to just use Strings in error messages and I hope it shouldn't be too much of a hassle to implement.

The big problem is knowing how we should format a ContextError<String>. The intention of how the String is meant to be used is dependent on the users context.

One option is for us to explore making StrContext / StrContextValue generic over their string type.

A workaround is to create your own adapter from ContextError<String> to your application's error type that allows the type of rendering intended for your application. This should probably be called out in the docs.

@Nukesor
Copy link
Contributor Author

Nukesor commented Feb 19, 2025

Making StrContext generic over the string type would be the optimal solution for me.
Just having something that behaves exactly like StrContext but with String would work perfectly fine for my usecase.

A workaround is to create your own adapter from ContextError to your application's error type that allows the type of rendering intended for your application. This should probably be called out in the docs.

That would be a nice :)

epage added a commit to epage/winnow that referenced this issue Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error Area: error reporting C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants