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

Some Diagnostic implementations set labels without source_code #977

Closed
2 tasks
khieta opened this issue Jun 13, 2024 · 3 comments · Fixed by #1351
Closed
2 tasks

Some Diagnostic implementations set labels without source_code #977

khieta opened this issue Jun 13, 2024 · 3 comments · Fixed by #1351
Labels
papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request

Comments

@khieta
Copy link
Contributor

khieta commented Jun 13, 2024

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?

  • 👋 I may be able to implement this internal improvement
  • ⚠️ This feature might incur a breaking change
@khieta khieta added backlog internal-improvement Refactoring, minor performance improvement, or other changes that Cedar users may never notice labels Jun 13, 2024
@john-h-kastner-aws
Copy link
Contributor

john-h-kastner-aws commented Jun 13, 2024

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
Copy link
Contributor Author

khieta commented Jun 13, 2024

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 😉

@khieta khieta added the papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request label Jun 17, 2024
@cdisselkoen cdisselkoen mentioned this issue Jul 3, 2024
3 tasks
@john-h-kastner-aws john-h-kastner-aws removed internal-improvement Refactoring, minor performance improvement, or other changes that Cedar users may never notice backlog labels Oct 1, 2024
@shaobo-he-aws shaobo-he-aws linked a pull request Dec 2, 2024 that will close this issue
4 tasks
@shaobo-he-aws
Copy link
Contributor

shaobo-he-aws commented Dec 3, 2024

ToJsonSchemaError, ShadowsBuiltinWarning, and ShadowsEntityWarning are already handled via #1124 and #1136.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants