Skip to content

internal: move diagnostics to hir #8973

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 25, 2021
Merged

internal: move diagnostics to hir #8973

merged 1 commit into from
May 25, 2021

Conversation

matklad
Copy link
Member

@matklad matklad commented May 24, 2021

No description provided.

@matklad matklad force-pushed the nodiag branch 7 times, most recently from 2594219 to 57b024d Compare May 25, 2021 14:47
@matklad matklad changed the title wip: move diagnostics to hir internal: move diagnostics to hir May 25, 2021
@matklad
internal: move diagnostics to hir
5c9f31d 
The idea here is to eventually get rid of `dyn Diagnostic` and
`DiagnosticSink` infrastructure altogether, and just have a `enum
hir::Diagnostic` instead.

The problem with `dyn Diagnostic` is that it is defined in the lowest
level of the stack (hir_expand), but is used by the highest level (ide).

As a first step, we free hir_expand and hir_def from `dyn Diagnostic`
and kick the can up to `hir_ty`, as an intermediate state. The plan is
then to move DiagnosticSink similarly to the hir crate, and, as final
third step, remove its usage from the ide.

One currently unsolved problem is testing. You can notice that the test
which checks precise diagnostic ranges, unresolved_import_in_use_tree,
was moved to the ide layer. Logically, only IDE should have the infra to
render a specific range.

At the same time, the range is determined with the data produced in
hir_def and hir crates, so this layering is rather unfortunate. Working
on hir_def shouldn't require compiling `ide` for testing.
@matklad
Copy link
Member Author

matklad commented May 25, 2021

r? @flodiebold, ready for review now. Does this match your vision for diagnostics=data?

@flodiebold
Copy link
Member

Hmm yes it goes in the direction, but I wouldn't have expected the diagnostics to just move to hir, rather than all the way to ide? I think my vision is basically how the field shorthand diagnostic is already implemented, actually. But there would have to be an interface for the various kinds of diagnostics in hir, of course 🤔

@matklad
Copy link
Member Author

matklad commented May 25, 2021 •
edited
Loading

Argh, I think I shifted one time to much. This moves diagnostic infra to hir_ty. I want to move it to hir as the end state, not to the ide. The fixes will leave in the ide though.

Not sure where most of diagnostic tests should live: I kinda want all tests to belong to crates actually emitting the specific diagnostics, but it certainly is easier to test at the level where you have generic infra.

still can’t read, negation is hard.

@flodiebold
Copy link
Member

I thought I wrote that badly 😅 I'd have expected diagnostics infra to move to ide in the end. I'd see testing for when diagnostics are emitted in the respective crates, and testing diagnostic descriptions and quickfixes in ide.

@matklad
Copy link
Member Author

matklad commented May 25, 2021

So what I think we want in the end is

  • hir_xxx emit raw diagnostics, something like DefDiagnostic which has local id in it as a public filed.
  • in hir, we re-formulate that it terms of hir-types. We go from (potentially local) ids to global unique ids. This step doesn't involve traits -- just translating one struct into another
  • in ide_diagnostics, we have have traits like DiagnosticWithFix and Diagnostic, which we implement for hir types. We might even end-up not using traits there, as there's no real need (and definitelly no dynamic dispatch)
  • ide_diagnostics also implements a bunch of lints which do not need privileged access to compiler data structures

In terms of diff with this PR, the type definitions in hir::diagnostic stay, while impl Diagnostic for go away.

@flodiebold
Copy link
Member

Agree, I think we probably don't need the traits.

@matklad
Copy link
Member Author

matklad commented May 25, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented May 25, 2021

Build succeeded:

@bors bors bot merged commit 5587d0a into rust-lang:master May 25, 2021
@kartva kartva mentioned this pull request Aug 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matklad @flodiebold