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

feat: update Problem to account for related information and code #6874

Merged
merged 1 commit into from
Apr 17, 2022

Conversation

ckipp01
Copy link
Contributor

@ckipp01 ckipp01 commented Apr 12, 2022

Follow up to #6871 (sorry had issues with rebasing and this was faster)
This PR makes changes to the existing xsbti.Problem to account for an
optional diagnostic code that the compiler can return for a given
diagnostic and also related information.

Given a piece of code like:

try {}

You'll receive the following:

-- [E002] Syntax Warning: /Users/ckipp/Documents/scala-workspace/dotty-error-index/examples/002_EmptyCatchAndFinallyBlockID.scala:3:2
3 |  try {}
  |  ^^^^^^
  |  A try without catch or finally is equivalent to putting
  |  its body in a block; no exceptions are handled.

The E002 here is the actual code. Right now there would be no
description.

Some diagnostics have multiple positions that they need to represent.
You can see an example of this
here in Dotty with the
use of inlining. Instead of needing to rely on including all of that
information in the diagnostic message it can now be extracted out into
a DiagnosticRelatedInformation.

These changes reference the conversation in #6868

This PR makes changes to the existing `xsbti.Problem` to account for an
optional diagnostic code that the compiler can return for a given
diagnostic and also related information.

Given a piece of code like:

```scala
try {}
```

You'll receive the following:

```
-- [E002] Syntax Warning: /Users/ckipp/Documents/scala-workspace/dotty-error-index/examples/002_EmptyCatchAndFinallyBlockID.scala:3:2
3 |  try {}
  |  ^^^^^^
  |  A try without catch or finally is equivalent to putting
  |  its body in a block; no exceptions are handled.
```

The `E002` here is the actual code. Right now there would be no
description.

Some diagnostics have multiple positions that they need to represent.
You can see an example of this
[here](scala/scala3#14002) in Dotty with the
use of inlining. Instead of needing to rely on including all of that
information in the diagnostic message it can now be extracted out into
a `DiagnosticRelatedInformation`.

These changes reference the conversation in sbt#6868
@ckipp01
Copy link
Contributor Author

ckipp01 commented Apr 14, 2022

Looks like the single run that failed sort of flaked out completely.

Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the contribution

@eed3si9n eed3si9n merged commit 543e831 into sbt:1.7.x Apr 17, 2022
@ckipp01 ckipp01 deleted the problem branch April 18, 2022 08:09
@deusaquilus
Copy link

Thanks so much @ckipp01 and @eed3si9n!!! Having per-inline-site info will be a really, really big bonus for diagnostics in Quill and everything that takes advantage of Scala 3 meta-programming!

ckipp01 added a commit to ckipp01/mill that referenced this pull request 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/mill that referenced this pull request 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/mill that referenced this pull request 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 pull request 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
eed3si9n pushed a commit to eed3si9n/sbt that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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
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.

4 participants