[Progress] Expose diagnostic code and related information #14904

ckipp01 · 2022-04-11T10:29:41Z

I've created this as a follow-up to the discussion started in the contributor forum. Since there was no push back, I've gone ahead and started the process. I'll use this issue as a way to track where the progress is at and to also communicate ahead of time the changes that I intend to give full chance for someone to spot something I may not be thinking of.

Short recap

If you don't want to read through the full contributors thread, the TLDR is this:

Current diagnostics only report a message, a source position, and a severity level
This makes it tricky for tooling (like Metals for example) to reliably offer code actions or quick fixes on diagnostics unless we do stuff like regex the message to determine the error type
To fix this we can expose the ErrorMessageID during reporting
As a bonus, we can also take situations where we have multiple positions relating to a diagnostic (like inlined positions) and expose them in a structured way.

While the code changes to do this won't be super large, it will impact quite a few places, so I'll outline the plan down below.

The plan

Some other notes

With the above, there is also some other assumptions. There are a ton of places we don't create a Message for an error. I know there was a big effort for this in #1589 but it was paused with the following message

The architecture for errors described in this issue proved to be heavy and impractical. The migration process frequently interferes with in-flight PRs and the benefits are marginal. We should find a more light-weight way to represent errors.

I'd challenge the idea of benefits are marginal, but as far as I'm aware, there is no alternative solution yet correct? Are we ok to start path on this path? I'm assuming yes, seeing that there has been no opposition yet on the forum? If this isn't ok, please speak up now as the ball has started rolling on the changes already. I've tried to over-communicate on this in multiple places to give ample opportunity for feedback, and I'm interpreting silence as support. So it'd be nice to have some explicit 🆗's from the core team before pushing this even further.

I also see that there were some changes to rename ErrorMessageIDs to UNUSED in 2e5df79. I recommend not to do this and to instead represent some sort of active indication in the actual structure instead. If we change them to UNUSED we sort of screw up the documentation, even if those IDs will no longer be used. I think we can follow a similar pattern as Rust in just marking them as "The compiler no longer emits this error" in the docs. That's what I've been doing here as an example.

There are also some things that are a bit foggy at the moment, like the case of the diagnostic that multiple inline positions attached to it. Currently this is all contained in the message, and we can expose that more structured in DiagnosticRelatedInformation, which works great for tooling, but not for console reporting. So we also need to figure out a good way to determine how that is displayed based on where it's being displayed. But maybe this isn't an issue as Dotty can just expose the diagnostic, and then the report is in charge of pulling what it needs out of DiagnosticRelatedInformation if there is anything and displaying it nicely.

If there are issues with anything outline above, please do speak up.

The text was updated successfully, but these errors were encountered:

Currently there is no way to tell if a given `ErrorMessageID` is emitted at all from the compiler. While working on an index of all the IDs I keep needing to dig into the code to see if there is a message that uses an `ErrorMessageID` and then see if that message is actually used anywhere. From what I can gather so far, the ones that I've marked as `false` are no longer emitted and therefore marked `isActive = false`. refs: scala#14904

Previously, when reducing `a.T` we checked if the type of `a` was a subtype of `RefinedType(.., T, TypeAlias(...))`, now we extend this check to handle refinements where the `info` is a `TypeBounds` where both bounds are equal. This solves two big issues at once: - We can restore tests/pos/13491.scala to its original form from before scala#13780. The check for abstract types introduced by scala#13780 for soundness reasons is no longer hit because the type selection is reduced before we get to that point. This is important because parboiled2 relies on this and is therefore currently broken on 3.1.3-RC1 and main (sirthias/parboiled2#365). - This fixes scala#14904 (slow compilation issue affecting parboiled2) without caching skolems (as in the alternative fix scala#14909). Again, this is due to the type containing skolem being reducible to a simpler type and therefore cacheable.

Currently there is no way to tell if a given `ErrorMessageID` is emitted at all from the compiler. While working on an index of all the IDs I keep needing to dig into the code to see if there is a message that uses an `ErrorMessageID` and then see if that message is actually used anywhere. From what I can gather so far, the ones that I've marked as `false` are no longer emitted and therefore marked `isActive = false`. refs: scala#14904

NOTE: It's important that in `Problem` we don't construct the actual `DiagnosticCode` _unless_ the method is called to get it. By doing this we keep compatibility ensuring that older build tools can still use the bridge just fine. That's what the `sbt-test/sbt-bridge` tests ensuring that this is still usable by an older tool using the old `Problem`. This then unlocks forwarding the diagnostic code on for tools to use. refs: scala#14904

At the moment this is just testing a bit. I see you already have a test PR to bump zinc in com-lihaoyi#1845 but one of the things that this brings in is the changes to `Problem` I made in sbt/sbt#6874 that expose the diagnostic code of the diagnostic coming from dotc. I have been doing some work on that on the compiler side in scala/scala3#15565 and wanted to try it out with Mill. I tried to mimic the way you currently have it set up, so let me know if it's not the direction you'd want to go. However, the idea here would be that the diagnostic code is forwarded when diagnostics are published via BSP so that Metals could then capture that code and know what code actions to offer. You can see more of the big picture in scala/scala3#14904.

NOTE: It's important that in `Problem` we don't construct the actual `DiagnosticCode` _unless_ the method is called to get it. By doing this we keep compatibility ensuring that older build tools can still use the bridge just fine. That's what the `sbt-test/sbt-bridge` tests ensuring that this is still usable by an older tool using the old `Problem`. This then unlocks forwarding the diagnostic code on for tools to use. refs: scala#14904

At the moment this is just testing a bit. I see you already have a test PR to bump zinc in com-lihaoyi#1845 but one of the things that this brings in is the changes to `Problem` I made in sbt/sbt#6874 that expose the diagnostic code of the diagnostic coming from dotc. I have been doing some work on that on the compiler side in scala/scala3#15565 and wanted to try it out with Mill. I tried to mimic the way you currently have it set up, so let me know if it's not the direction you'd want to go. However, the idea here would be that the diagnostic code is forwarded when diagnostics are published via BSP so that Metals could then capture that code and know what code actions to offer. You can see more of the big picture in scala/scala3#14904.

NOTE: It's important that in `Problem` we don't construct the actual `DiagnosticCode` _unless_ the method is called to get it. By doing this we keep compatibility ensuring that older build tools can still use the bridge just fine. That's what the `sbt-test/sbt-bridge` tests ensuring that this is still usable by an older tool using the old `Problem`. This then unlocks forwarding the diagnostic code on for tools to use. refs: scala#14904

At the moment this is just testing a bit. I see you already have a test PR to bump zinc in com-lihaoyi#1845 but one of the things that this brings in is the changes to `Problem` I made in sbt/sbt#6874 that expose the diagnostic code of the diagnostic coming from dotc. I have been doing some work on that on the compiler side in scala/scala3#15565 and wanted to try it out with Mill. I tried to mimic the way you currently have it set up, so let me know if it's not the direction you'd want to go. However, the idea here would be that the diagnostic code is forwarded when diagnostics are published via BSP so that Metals could then capture that code and know what code actions to offer. You can see more of the big picture in scala/scala3#14904.

This change is a continuation of the work that was done in sbt#6874 to allow the Scala 3 compiler to forward the `diagnosticCode` of an error as well as the other normal things. This change in dotty was merged in scala/scala3#15565 and also backported so it will be in the 3.2.x series release. This change captures the `diagnosticCode` and forwards it on via BSP so that tools like Metals can can use this code. You can see this in the BSP trace now for a diagnostic: For example with some code that contains the following: ```scala val x: Int = "hi" ``` You'll see the code in the BSP diagnostic: ``` "diagnostics": [ { "range": { "start": { "line": 9, "character": 15 }, "end": { "line": 9, "character": 19 } }, "severity": 1, "code": "7", "source": "sbt", "message": "Found: (\u001b[32m\"hi\"\u001b[0m : String)\nRequired: Int\n\nThe following import might make progress towards fixing the problem:\n\n import sourcecode.Text.generate\n\n" } ] ``` Co-authored-by: Adrien Piquerez <[email protected]> Refs: scala/scala3#14904

This relates to the grand plan of scala/scala3#14904 and recently forwarding the `diagnosticCode` has been merged in scala/scala3#15565 and also backported so it should show up in the 3.2.x series. While this pr isn't super exciting, it's just making sure we capture the code and forward it, this should unlock _much_ better ways to determine what code actions are available for a given diagnostic. Meaning we don't have to do lovely things like regex on the diagnostic message for Scala 3 diagnostics. NOTE: that this does need some more changes in the build servers before this is usable. So we can wait for those to be merged in if you'd like. - [ ] sbt - sbt/sbt#6998 - [ ] Bloop - scalacenter/bloop#1750 - [ ] Mill - com-lihaoyi/mill#1912 Now if you look at the trace file for a diagnostic you'll see the addition of the code: ``` "diagnostics": [ { "range": { "start": { "line": 9, "character": 15 }, "end": { "line": 9, "character": 19 } }, "severity": 1, "code": "7", "source": "sbt", "message": "Found: (\u001b[32m\"hi\"\u001b[0m : String)\nRequired: Int\n\nThe following import might make progress towards fixing the problem:\n\n import sourcecode.Text.generate\n\n" } ], ``` Refs: scala/scala3#14904

This change is a continuation of the work that was done in sbt#6874 to allow the Scala 3 compiler to forward the `diagnosticCode` of an error as well as the other normal things. This change in dotty was merged in scala/scala3#15565 and also backported so it will be in the 3.2.x series release. This change captures the `diagnosticCode` and forwards it on via BSP so that tools like Metals can can use this code. You can see this in the BSP trace now for a diagnostic: For example with some code that contains the following: ```scala val x: Int = "hi" ``` You'll see the code in the BSP diagnostic: ``` "diagnostics": [ { "range": { "start": { "line": 9, "character": 15 }, "end": { "line": 9, "character": 19 } }, "severity": 1, "code": "7", "source": "sbt", "message": "Found: (\u001b[32m\"hi\"\u001b[0m : String)\nRequired: Int\n\nThe following import might make progress towards fixing the problem:\n\n import sourcecode.Text.generate\n\n" } ] ``` Co-authored-by: Adrien Piquerez <[email protected]> Refs: scala/scala3#14904

@adpi

Looks like I missed this in sbt#6874 and I hit on it in Mill when I couldn't figure out why it was also empty, and thanks to @adpi realized it was because of the `LoggedReporter` in zinc not taking it into account. However before I can bump that this needs to be bumped as well. refs: scala/scala3#14904

@adpi

Looks like I missed this in sbt#6874 and I hit on it in Mill when I couldn't figure out why it was also empty, and thanks to @adpi realized it was because of the `LoggedReporter` in zinc not taking it into account. However before I can bump that this needs to be bumped as well. refs: scala/scala3#14904

@adpi

Looks like I missed this in sbt#6874 and I hit on it in Mill when I couldn't figure out why it was also empty, and thanks to @adpi realized it was because of the `LoggedReporter` in zinc not taking it into account. However before I can bump that this needs to be bumped as well. refs: scala/scala3#14904

This change is a continuation of the work that was done in sbt#6874 to allow the Scala 3 compiler to forward the `diagnosticCode` of an error as well as the other normal things. This change in dotty was merged in scala/scala3#15565 and also backported so it will be in the 3.2.x series release. This change captures the `diagnosticCode` and forwards it on via BSP so that tools like Metals can can use this code. You can see this in the BSP trace now for a diagnostic: For example with some code that contains the following: ```scala val x: Int = "hi" ``` You'll see the code in the BSP diagnostic: ``` "diagnostics": [ { "range": { "start": { "line": 9, "character": 15 }, "end": { "line": 9, "character": 19 } }, "severity": 1, "code": "7", "source": "sbt", "message": "Found: (\u001b[32m\"hi\"\u001b[0m : String)\nRequired: Int\n\nThe following import might make progress towards fixing the problem:\n\n import sourcecode.Text.generate\n\n" } ] ``` Co-authored-by: Adrien Piquerez <[email protected]> Refs: scala/scala3#14904

@adpi

Looks like I missed this in sbt#6874 and I hit on it in Mill when I couldn't figure out why it was also empty, and thanks to @adpi realized it was because of the `LoggedReporter` in zinc not taking it into account. However before I can bump that this needs to be bumped as well. refs: scala/scala3#14904

@adpi

Looks like I missed this in sbt#6874 and I hit on it in Mill when I couldn't figure out why it was also empty, and thanks to @adpi realized it was because of the `LoggedReporter` in zinc not taking it into account. However before I can bump that this needs to be bumped as well. refs: scala/scala3#14904

At the moment this is just testing a bit. I see you already have a test PR to bump zinc in com-lihaoyi#1845 but one of the things that this brings in is the changes to `Problem` I made in sbt/sbt#6874 that expose the diagnostic code of the diagnostic coming from dotc. I have been doing some work on that on the compiler side in scala/scala3#15565 and wanted to try it out with Mill. I tried to mimic the way you currently have it set up, so let me know if it's not the direction you'd want to go. However, the idea here would be that the diagnostic code is forwarded when diagnostics are published via BSP so that Metals could then capture that code and know what code actions to offer. You can see more of the big picture in scala/scala3#14904.

Newer zinc brings in is the changes to `Problem` I made in sbt/sbt#6874 that expose the diagnostic code of the diagnostic coming from dotc. I have been doing some work on that on the compiler side in scala/scala3#15565 and wanted to try it out with Mill. I tried to mimic the way you currently have it set up, so let me know if it's not the direction you'd want to go. However, the idea here would be that the diagnostic code is forwarded when diagnostics are published via BSP so that Metals could then capture that code and know what code actions to offer. You can see more of the big picture in scala/scala3#14904. Pull request: #1912

Currently there is no way to tell if a given `ErrorMessageID` is emitted at all from the compiler. While working on an index of all the IDs I keep needing to dig into the code to see if there is a message that uses an `ErrorMessageID` and then see if that message is actually used anywhere. From what I can gather so far, the ones that I've marked as `false` are no longer emitted and therefore marked `isActive = false`. refs: scala#14904

NOTE: It's important that in `Problem` we don't construct the actual `DiagnosticCode` _unless_ the method is called to get it. By doing this we keep compatibility ensuring that older build tools can still use the bridge just fine. That's what the `sbt-test/sbt-bridge` tests ensuring that this is still usable by an older tool using the old `Problem`. This then unlocks forwarding the diagnostic code on for tools to use. refs: scala#14904

This relates to the grand plan of scala/scala3#14904 and recently forwarding the `diagnosticCode` has been merged in scala/scala3#15565 and also backported so it should show up in the 3.2.x series. While this pr isn't super exciting, it's just making sure we capture the code and forward it, this should unlock _much_ better ways to determine what code actions are available for a given diagnostic. Meaning we don't have to do lovely things like regex on the diagnostic message for Scala 3 diagnostics. NOTE: that this does need some more changes in the build servers before this is usable. So we can wait for those to be merged in if you'd like. - [ ] sbt - sbt/sbt#6998 - [ ] Bloop - scalacenter/bloop#1750 - [ ] Mill - com-lihaoyi/mill#1912 Now if you look at the trace file for a diagnostic you'll see the addition of the code: ``` "diagnostics": [ { "range": { "start": { "line": 9, "character": 15 }, "end": { "line": 9, "character": 19 } }, "severity": 1, "code": "7", "source": "sbt", "message": "Found: (\u001b[32m\"hi\"\u001b[0m : String)\nRequired: Int\n\nThe following import might make progress towards fixing the problem:\n\n import sourcecode.Text.generate\n\n" } ], ``` Refs: scala/scala3#14904

smarter · 2023-03-28T17:22:23Z

Some inspiration: https://discourse.haskell.org/t/announcing-the-haskell-error-index/5195 / https://errors.haskell.org/

ckipp01 added the stat:needs triage Every issue needs to have an "area" and "itype" label label Apr 11, 2022

ckipp01 mentioned this issue Apr 18, 2022

refactor: add isActive to ErrorMessageID #14965

Merged

romanowski added area:tooling and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Apr 19, 2022

smarter mentioned this issue Apr 20, 2022

More aggressive reduction of type selection (fixes parboiled2) #14987

Merged

ckipp01 mentioned this issue Jul 1, 2022

bump compiler-interface and capture the diagnosticCode #15565

Merged

ckipp01 mentioned this issue Jul 1, 2022

Bump zinc and account for diagnostic code com-lihaoyi/mill#1912

Merged

ckipp01 mentioned this issue Jul 27, 2022

dep: bump sbt to 1.7.1 #15728

Merged

dwijnand mentioned this issue Aug 1, 2022

[bp] bump compiler-interface and capture the diagnosticCode #15794

Merged

ckipp01 mentioned this issue Aug 9, 2022

feat: start forwarding diagnosticCode via BSP sbt/sbt#6998

Merged

ckipp01 mentioned this issue Aug 9, 2022

feat: capture and forward diagnosticCode scalameta/metals#4239

Merged

3 tasks

eed3si9n mentioned this issue Aug 10, 2022

[1.7.x] feat: start forwarding diagnosticCode via BSP sbt/sbt#7001

Merged

This was referenced Aug 21, 2022

fix: also include diagnosticCode and related info in InterfaceUtil ckipp01/sbt#1

Closed

fix: also include diagnosticCode and related info in InterfaceUtil sbt/sbt#7006

Merged

ckipp01 mentioned this issue Aug 25, 2022

Scala cli actionable diagnostic scalameta/metals#4297

Merged

eed3si9n mentioned this issue Oct 2, 2022

[1.7.x] feat: start forwarding diagnosticCode via BSP, take 2 sbt/sbt#7033

Merged

This was referenced Oct 2, 2022

[1.8.x] fix: also include diagnosticCode and related info in InterfaceUtil sbt/sbt#7039

Merged

[1.7.x] fix: also include diagnosticCode and related info in InterfaceUtil sbt/sbt#7040

Merged

ckipp01 mentioned this issue Oct 5, 2022

ErrorID no longer being reported for 54 #16130

Closed

ckipp01 mentioned this issue Apr 24, 2023

[Progress] - start utilizing data in diagnostics to forward "quickfixes" to tools #17337

Closed

5 tasks

lrytz mentioned this issue Feb 28, 2024

Add page about Scala 2 -Xsource:3 to migration guide scala/docs.scala-lang#2966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Progress] Expose diagnostic code and related information #14904

[Progress] Expose diagnostic code and related information #14904

ckipp01 commented Apr 11, 2022 •

edited

Loading

smarter commented Mar 28, 2023

[Progress] Expose diagnostic code and related information #14904

[Progress] Expose diagnostic code and related information #14904

Comments

ckipp01 commented Apr 11, 2022 • edited Loading

Short recap

The plan

Some other notes

smarter commented Mar 28, 2023

ckipp01 commented Apr 11, 2022 •

edited

Loading