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

Add a spanned method for automatically wrapping the parser result in a Span #640

Open
TheOnlyTails opened this issue Jun 6, 2024 · 15 comments · May be fixed by #713
Open

Add a spanned method for automatically wrapping the parser result in a Span #640

TheOnlyTails opened this issue Jun 6, 2024 · 15 comments · May be fixed by #713
Labels
api A problem with the design of an API feature enhancement New feature or request

Comments

@TheOnlyTails
Copy link

This would be a very simple utility method I feel is missing right now, as a shorthand:

parser.map_extra(|it, e| (it, e.span()))
// into
parser.spanned()

It's not essential or anything, but it'll definitely be nice to have.

@zesterer
Copy link
Owner

I've considered this a few times, but in practice it's far from a convention to represent the two as a tuple: folks often create a struct Spanned<T>(T, Span); type that implements Deref<Target = T> for the same purpose.

I am tempted to make such a Spanned type a part of chumsky's API though. Would that suit you?

@TheOnlyTails
Copy link
Author

In my opinion, a library taking a position in cases like this usually makes it easier to solve problems and reduces fragmentation, so I'm all in favor of an official solution.

@zesterer
Copy link
Owner

What worries me about this is that such a Spanned type would effectively become a pervasive part of a user's compiler since it would be present in the AST too. Perhaps this isn't a problem in itself, but chumsky tries very hard to be domain-specific, not being opinionated about other aspects of a compiler.

@TheOnlyTails
Copy link
Author

IMO that's a good thing, it encourages and makes it easier to do good error reporting, since you can access the span right then and there. Besides, it's opt-in only for those who use the method.

@mlgiraud
Copy link

Is there any case where Spanned is something else than a token or element, and a span? In the current version of chumsky i also don't see an easy way to create a custom spanned type in a tokenizer that is then consumed by a parser, since the Input trait is sealed, and the SpannedInput type requires tuples as tokens. So either it would make sense to have a Spanned type that is provided by the library, or to have a trait that needs to be implemented by a "Spanned" type such that chumsky can handle it, right? Imho i believe both would make sens, i.e. providing a trait and a default "Spanned" type that implements this trait.

@zesterer
Copy link
Owner

or to have a trait that needs to be implemented by a "Spanned" type such that chumsky can handle it, right? Imho i believe both would make sens, i.e. providing a trait and a default "Spanned" type that implements this trait.

The problem with a trait is that you then end up needing to specify the implementor you want, adding more type annotations (and potentially confusing type errors)

One option might be to tie it to the Span trait itself. For example:

pub trait Span {
    type Spanned<T>;

    fn make_spanned<T>(self, item: T) -> Self::Spanned<T>;
}

...

pub struct Spanned<T, S = SimpleSpan>(T, S);

impl Span for SimpleSpan {
    type Spanned<T> = Spanned<T>;

    fn make_spanned<T>(self, item: T) -> Self::Spanned<T> { Spanned(item, self) }
}

Then a .spanned() combinator could be introduced that adds performs this spanning operation automatically.

How do you both feel about this?

@zesterer zesterer added enhancement New feature or request api A problem with the design of an API feature labels Oct 23, 2024
@TheOnlyTails
Copy link
Author

I really like this!

@MeGaGiGaGon
Copy link

I third this, trying to migrate from pom where Parser could be easily extended to add a spanned combinator, not having it sucks majorly.

@zesterer zesterer linked a pull request Jan 2, 2025 that will close this issue
@zesterer
Copy link
Owner

zesterer commented Jan 2, 2025

In #713 I've made a first pass attempt at this. Do you have any thoughts @TheOnlyTails, @MeGaGiGaGon, and @Hedgehogo?

@Hedgehogo
Copy link
Contributor

You could make a separate trait that would contain the associated Spanned type and be a subtrait for Span. Alternatively, you could just return From<(T, Span)> and hope that the type inference works out (e.g. based on the function type being returned). There is also an option to add some kind of map_into combinator, which would simplify the map(|i| From::from(i)) construction.

@zesterer
Copy link
Owner

zesterer commented Jan 2, 2025

I don't think that inference is likely to be powerful enough to infer the output type in enough cases that it would get very annoying.

.map(|i| From::from(i)) can already be simplified to .map(From::from), and I don't think there's much point going simpler than that. Users already understand both map and From, adding another combinator to replace both has few benefits but a big complexity drawback. To be honest, I think even .spanned() toes the line on this.

@Hedgehogo
Copy link
Contributor

I don't think just a tuple works for us, as it's too weak a simplification to be worthy of a combinator. So the choice is between three solutions: built in chumsky Spanned without trait binding, adding an associated type to the Span trait, adding a new trait with the required associated type. Between the first and the others, the question is: can the user use his own type?

Yes, it can, in case the user would like to somehow optimize spans for memory by pulling missing span information from T. However, this also imposes constraints on T, thus cutting off the option of putting an associated type inside Span as a solution. Further reasoning depends on whether we really care about the rare user who needs to optimize spans by memory?

If the answer to this question is no, then there is practically no applicability of own Spanned and the first option should be taken, the alternative requires additional discussion in the matter of the design of this very trait and its name. The second solution does not appear anywhere.

@zesterer
Copy link
Owner

zesterer commented Jan 6, 2025

The current implementation allows the user to specify their own spanning type.

@Hedgehogo
Copy link
Contributor

The current implementation allows the user to specify their own spanning type.

But it is useless as long as it does not allow to impose constraints on T.

@Hedgehogo
Copy link
Contributor

I will describe how the implementation should look like, at which it is possible to realize the situation described above.

We need a separate trait that allows us to match a span - Spanned:

trait ???: Span {
    type Spanned<T>;
}

But such a trait does not allow to impose restrictions on T, and this is necessary to get the missing information from it in case of memory-optimized Span. In this case, we can move T from the associated type to the trait:

trait ????<T>: Span {
    type Spanned;
}

This is actually a mapping of the form (T, S) -> Sp. It can be written in Rust in a different way:

trait ???<S: Span> {
    type Spanned;
}

In this form it looks as if some types can have their own Spanned, and since in a condensed form how to get Span depends on the type, it looks logical. It also makes it easy to pick up the name - WithSpanned. The final design of the trait, as well as its standard implementation in chumsky:

trait WithSpanned<S: Span> {
    type Spanned;

    fn new_spanned(self, span: S) -> Self::Spanned;
}

impl<T> WithSpanned<SimpleSpan> for T {
    type Spanned = Spanned<T>;

    fn new_spanned(self, span: SimpleSpan) -> Self::Spanned {
        Spanned(self, span)
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A problem with the design of an API feature enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants