Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Diagnostic refactor #1048

Merged
merged 4 commits into from
Jan 27, 2025
Merged

Diagnostic refactor #1048

merged 4 commits into from
Jan 27, 2025

Conversation

sbillig
Copy link
Collaborator

@sbillig sbillig commented Jan 24, 2025

Goal:

  • Diagnostic enums should contain the (salsa) ids of the things involved in the error (eg the TyIds of the types for a type mismatch). These enum variants should generally be constructed directly, rather than via constructor functions that might generate strings, etc.
  • The complexity of constructing helpful messages/labels for the user should be consolidated in the to_complete(..) -> CompleteDiagnostic functions.
  • We should add convenience functions to the (salsa) id types where it's helpful.

I scrapped the LazyDiagnostic idea that I showed during the meeting, in favor of just moving the diagnostic stuff from the hir crate to the hir-analysis crate, so now DiagnosticVoucher::to_complete takes a SpannedHirAnalysisDb instead of a SpannedHirDb, etc. Its useful to have concrete span locations when generating the diagnostic text, so we can sort the labels by location and use text like "x defined first here", "x redefined here", etc and have it make sense.

I'll submit some follow-up PRs that actually improve diagnostics in the future, and to do a little more code shuffling. I'd like to merge this because it touches a lot of lines and could lead to conflict hassles for other stuff.

@sbillig sbillig force-pushed the diag-refactor branch 2 times, most recently from 6a41ba7 to 3194e40 Compare January 26, 2025 00:31
Copy link
Member

@g-r-a-n-t g-r-a-n-t left a comment

Choose a reason for hiding this comment

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

👍

@sbillig sbillig removed the request for review from micahscopes January 27, 2025 19:05
@sbillig sbillig merged commit f704df1 into ethereum:master Jan 27, 2025
6 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants