Skip to content

Conversation

@quangloc99
Copy link
Contributor

Resolve #592

I think the linked issue explains the motivation well enough. I'll list the considerations and changes here for clarity:

  • I tried to preserve the ported code as much as possible. So code from Rustc
    that are duplicated or not well structured are kept.
  • There are some naming differences.
  • We do not need to handle translation like in Rustc.
  • In Rustc code, the parameter is consumed, while in Solar code, it is passed by reference or Cow.
    There are part that data is cloning to preserve the ported code behavior.
  • Some parts like FileName and source code handling are simplified as Solar
    does not need to handle as much cases as Rustc.
  • There is a minor change in the behavior of the emitter. Currently, Solar emits
    the diagnostic in 4 parts, while Rustc does not separate into parts at all.
    I made the change to keep the footer part, and the rest are combined into one like in Rustc.
  • This change make the old code in diagnostics/emitter/rustc.rs completely obsolete.
    I still kept it though in case that FileWithAnnotatedLines::collect_annotations's behavior is more desired.

Overall, there is only one test output changed, which is due to the mentioned change in emitter behavior.
I'm surprised that the behavior stayed mostly the same.

Please take a look and let me know if there are any issues. I think the code can be refactored further, either in
this PR or in a separate one.

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks right. I'd like @0xrusowsky to give a look first

Overall, there is only one test output changed, which is due to the mentioned change in emitter behavior.
I'm surprised that the behavior stayed mostly the same.

I guess that's a good thing!

Copy link
Contributor

@0xrusowsky 0xrusowsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it makes sense overall, just left a couple of nits

}
fn shrink_file(
spans: &[Span],
_file_name: &FileName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we drop this param if unused?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep as close as possible to upstream

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to move this macro from crates/interface/src/lib.rs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot be in the same crate due to macro_expanded_macro_exports_accessed_by_absolute_paths

@onbjerg onbjerg merged commit 251eb50 into paradigmxyz:main Dec 11, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update annotate_snippets emitter

5 participants