cedar-policy / cedar

Implementation of the Cedar Policy Language
https://www.cedarpolicy.com
Apache License 2.0
765 stars 63 forks source link

Some `Diagnostic` implementations set `labels` without `source_code` #977

Open khieta opened 2 weeks ago

khieta commented 2 weeks ago

Describe the improvement you'd like to request

Some of our Diagnostic implementations set the labels function without setting source_code. These two fields are related (see the excerpt from miette below), so it doesn't make sense to set one without the other. The relevant error types seem to print fine (for some reason), but users who are consuming the miette errors programmatically may observe unexpected behavior. For example: I was trying to update this testing code to use the error's source text instead of the passed in source (which may or may not match the error), but found that the source text didn't exist for ToCSTErrors.

    /// Source code to apply this `Diagnostic`'s [`Diagnostic::labels`] to.
    fn source_code(&self) -> Option<&dyn SourceCode> {
        None
    }

    /// Labels to apply to this `Diagnostic`'s [`Diagnostic::source_code`]
    fn labels(&self) -> Option<Box<dyn Iterator<Item = LabeledSpan> + '_>> {
        None
    }

Here's the list of error types doing this, from a quick scan through the code:

Describe alternatives you've considered

No response

Additional context

No response

Is this something that you'd be interested in working on?

john-h-kastner-aws commented 2 weeks ago

This is the root cause of #948 (although we could fix that issue by attaching source in the CLI if this larger issue is more difficult to fix)

khieta commented 2 weeks ago

I don't think this issue is too much work to fix. I'm just tired of playing with the parsing code, so I figured I'd throw it out there for someone else to pick up 😉