Skip to content
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.

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

[Progress] Expose diagnostic code and related information #14904

Open
12 of 16 tasks
ckipp01 opened this issue Apr 11, 2022 · 1 comment
Open
12 of 16 tasks

[Progress] Expose diagnostic code and related information #14904

ckipp01 opened this issue Apr 11, 2022 · 1 comment

Comments

@ckipp01
Copy link
Member

ckipp01 commented Apr 11, 2022

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.

@ckipp01 ckipp01 added the stat:needs triage Every issue needs to have an "area" and "itype" label label Apr 11, 2022
ckipp01 added a commit to ckipp01/dotty that referenced this issue Apr 18, 2022
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
@romanowski romanowski added area:tooling and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Apr 19, 2022
smarter added a commit to dotty-staging/dotty that referenced this issue Apr 20, 2022
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.
ckipp01 added a commit to ckipp01/dotty that referenced this issue May 6, 2022
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
ckipp01 added a commit to ckipp01/dotty that referenced this issue May 11, 2022
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
ckipp01 added a commit to ckipp01/dotty that referenced this issue Jul 1, 2022
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
ckipp01 added a commit to ckipp01/mill that referenced this issue Jul 1, 2022
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.
ckipp01 added a commit to ckipp01/dotty that referenced this issue Jul 1, 2022
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
ckipp01 added a commit to ckipp01/mill that referenced this issue Jul 22, 2022
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.
ckipp01 added a commit to ckipp01/dotty that referenced this issue Jul 22, 2022
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
ckipp01 added a commit to ckipp01/dotty that referenced this issue Jul 28, 2022
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
ckipp01 added a commit to ckipp01/dotty that referenced this issue Jul 28, 2022
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
dwijnand pushed a commit to dwijnand/scala3 that referenced this issue Aug 1, 2022
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
ckipp01 added a commit to ckipp01/mill that referenced this issue Aug 7, 2022
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.
ckipp01 added a commit to ckipp01/sbt that referenced this issue Aug 9, 2022
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 <adrien.piquerez@gmail.com>
Refs: scala/scala3#14904
ckipp01 added a commit to ckipp01/metals that referenced this issue Aug 9, 2022
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
eed3si9n pushed a commit to eed3si9n/sbt that referenced this issue Aug 10, 2022
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 <adrien.piquerez@gmail.com>
Refs: scala/scala3#14904
ckipp01 added a commit to ckipp01/sbt that referenced this issue Aug 21, 2022
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
ckipp01 added a commit to ckipp01/sbt that referenced this issue Aug 21, 2022
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
ckipp01 added a commit to ckipp01/sbt that referenced this issue Aug 22, 2022
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
eed3si9n pushed a commit to eed3si9n/sbt that referenced this issue Oct 2, 2022
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 <adrien.piquerez@gmail.com>
Refs: scala/scala3#14904
eed3si9n pushed a commit to eed3si9n/sbt that referenced this issue Oct 2, 2022
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
eed3si9n pushed a commit to eed3si9n/sbt that referenced this issue Oct 2, 2022
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
ckipp01 added a commit to ckipp01/mill that referenced this issue Oct 3, 2022
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.
ckipp01 added a commit to ckipp01/mill that referenced this issue Oct 8, 2022
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.
lefou pushed a commit to com-lihaoyi/mill that referenced this issue Oct 8, 2022
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
bishabosha pushed a commit to dotty-staging/dotty that referenced this issue Oct 18, 2022
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
bishabosha pushed a commit to dotty-staging/dotty that referenced this issue Oct 18, 2022
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
ckipp01 added a commit to ckipp01/metals that referenced this issue Oct 24, 2022
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
tgodzik pushed a commit to scalameta/metals that referenced this issue Oct 25, 2022
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
Copy link
Member

smarter commented Mar 28, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants