Reporting source locations for errors #398
Conversation
yihozhang
commented
Jul 24, 2024
yihozhang
commented
src/ast/desugar.rs
Outdated
| @@ -339,6 +358,7 @@ pub(crate) fn desugar_command( | |||
| }; | |||
|
|
|||
| desugar.desugar_program( | |||
| // TODO: all spans should be that of the original query | |||
| desugar.parse_program(None, &desugaring).unwrap(), | |||
There was a problem hiding this comment.
We could panic when this unwrap fails, and point to the span for the query extract. It would be an okay way to fix the error message here
There was a problem hiding this comment.
This would not work in a lot of cases since the failure usually happens at a later stage (typechecking or run time). One way to do it right is to write a subst_span : Command -> (Span -> Span) -> Command function that substitutes the spans. However, since this is the only case where we need to give spans right now manually, I just changed the implementation to build the AST directly. In the future we can implement this subst_span function if needed.
src/ast/desugar.rs
Outdated
| @@ -460,16 +481,7 @@ impl Desugar { | |||
| } | |||
|
|
|||
| pub fn desugar_function(&mut self, fdecl: &FunctionDecl) -> Vec<NCommand> { | |||
There was a problem hiding this comment.
perhaps we don't need desugar_funciton anymore?
| /// A [`Span`] contains the file name and a pair of offsets representing the start and the end. | ||
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| pub struct Span(pub Arc<SrcFile>, pub usize, pub usize); | ||
|
|
||
| impl Span { |
There was a problem hiding this comment.
Why have Quote and Span? Why not just Span with a display that does the same thing as a Quote?
Looks like we don't build a quote manually anywhere
There was a problem hiding this comment.
You're right no one should build a quote manually. However, computing Quote from Span takes O(N) time so fmt might not be the best place to do it. However, the original implementation needed to clone the program texts which is costly, so I changed the implementation to use a reference instead.
| Output { | ||
| span: Span, | ||
| file: String, | ||
| exprs: Vec<GenericExpr<Head, Leaf>>, | ||
| }, | ||
| Push(usize), |
There was a problem hiding this comment.
How did you decide what gets a span? Looks like Push doesn't have one. Perhaps because there are no error messages with it?
There was a problem hiding this comment.
Yeah there's no error message associated with push right now. For now I just added spans to commands that may report a TypeError.
