Replace DUMMY_SPAN with Span::Panic and Span::Rust
#498
Conversation
Alex-Fischman
requested review from
saulshanabrook and
oflatt
and removed request for
saulshanabrook and
oflatt
CodSpeed Performance ReportMerging #498 will not alter performanceComparing Summary
|
|
For Python code I have been using a dummy span for all nodes. The thought would be eventually I could add tracebacks to the Python source lines but I haven't done that yet. For now, I guess I could just make up a dummy rust span that doesn't exist? Like it won't crash if the line/file aren't real right? |
|
Oh, I wasn't thinking about Python. Yeah, |
|
I think the performance regression is coming from spans getting larger from 3 words to 4, since the compiler can't distinguish between The only potential fix I can think to try would be to wrap spans in an |
|
Okay, that seems to have improved things. Opening for review. |
Alex-Fischman
requested review from
FTRobbin,
saulshanabrook,
oflatt and
yihozhang
and removed request for
a team and
FTRobbin
Alex-Fischman
force-pushed
the
span-macro
branch
from
eaec7e1 to
c9fa133
Compare
What about just File? Or SourceFile? --+ I'm not sure also. I wouldn't worry about it now actually and can just leave it as rust and I can set it to a dummy file for now. We could revisit this once I add proper source tracking into egglog Python, to carry that info forward and add the proper spans. |
|
If this gets merged, it would be cool to have |
|
@RiscInside Done, assuming that by |
|
Great, thank you!
Indeed, my bad. |
| @@ -321,7 +317,7 @@ impl EGraph { | |||
| .extract_term(self, *value, &extractor, &mut termdag) | |||
| .expect("Extraction should be successful since extractor has been fully initialized"); | |||
|
|
|||
| termdag.term_to_expr(&term).to_string() | |||
| termdag.term_to_expr(&term, Span::Panic).to_string() | |||
There was a problem hiding this comment.
Why not use the span!() macro here? Just trying to understand when you want panic vs that one.
There was a problem hiding this comment.
The to_string method ignores spans, so using Span::Panic is just a runtime check that the spans are in fact ignored.
In general, I think Span::Panic is useful when you know that the spans are not observable by outside code; i.e. I would avoid it if the function returned an Expr.
There was a problem hiding this comment.
This seems good, better than what we have now, thank you @Alex-Fischman!
This PR improves error messages by introducing two new types of spans. The first is
Span::Rust, which stores a location in a Rust file using thefile!macro family. The second isSpan::Panic, which can be used when you know that the span should not be introspected; for an example, see the changes inserialize.rs.