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

Add actionable item to PatternMatchExhaustivity diagnostic #18314

Merged
merged 8 commits into from
Aug 9, 2023

Conversation

ghostbuster91
Copy link
Contributor

@ghostbuster91 ghostbuster91 commented Jul 30, 2023

The purpose of this PR is to add an actionable item for non-exhaustive pattern match diagnostic, so that people can auto insert missing cases.

Relates to scalameta/metals-feature-requests#350

@ghostbuster91
Copy link
Contributor Author

@ckipp01 could you give it a look?

@ghostbuster91 ghostbuster91 force-pushed the code-action-missing-cases branch from 91bdc53 to bf29c1f Compare July 31, 2023 12:32
@ckipp01
Copy link
Member

ckipp01 commented Aug 1, 2023

Just a note about something we need to figure out. Testing this I now see our e2e with Dotty/sbt/Metals isn't working as expected. I know there was some recent changes in sbt, but the behavior I'm seeing now is that the diagnostic is published with the actions, but then published again without them with reset to true. For example:

[Trace - 00:47:25 pm] Received notification 'build/publishDiagnostics'
Params: {
  "textDocument": {
    "uri": "file:///Users/ckipp/Documents/scala-workspace/actionable-diagnostic/src/main/scala/example/Hello.scala"
  },
  "buildTarget": {
    "uri": "file:/Users/ckipp/Documents/scala-workspace/actionable-diagnostic/#root/Compile"
  },
  "diagnostics": [
    {
      "range": {
        "start": {
          "line": 17,
          "character": 26
        },
        "end": {
          "line": 17,
          "character": 30
        }
      },
      "severity": 2,
      "code": "29",
      "source": "sbt",
      "message": "\u001b[33mmatch\u001b[0m may not be exhaustive.\n\nIt would fail on pattern case: Tree.Leaf(_)\n",
      "relatedInformation": [],
      "dataKind": "scala",
      "data": {
        "actions": [
          {
            "title": "Insert missing cases (1)",
            "edit": {
              "changes": [
                {
                  "range": {
                    "start": {
                      "line": 18,
                      "character": 33
                    },
                    "end": {
                      "line": 18,
                      "character": 33
                    }
                  },
                  "newText": "\n      case Tree.Leaf(_) \u003d\u003e ???"
                }
              ]
            }
          }
        ]
      }
    }
  ],
  "reset": false
}


[Trace - 00:47:25 pm] Received notification 'build/logMessage'
Params: {
  "type": 2,
  "message": "\u001b[33m\u001b[33m-- [E029] Pattern Match Exhaustivity Warning: /Users/ckipp/Documents/scala-workspace/actionable-diagnostic/src/main/scala/example/Hello.scala:18:26 \u001b[0m\u001b[0m\n\u001b[33m18 |\u001b[0m    \u001b[33mdef\u001b[0m \u001b[36mfoo\u001b[0m(\u001b[36mtree\u001b[0m: \u001b[35mTree\u001b[0m) \u003d tree \u001b[33mmatch\u001b[0m {\n\u001b[33m\u001b[33m   |\u001b[0m                          ^^^^\u001b[0m\n\u001b[33m   |\u001b[0m                          \u001b[33mmatch\u001b[0m may not be exhaustive.\n\u001b[33m   |\u001b[0m\n\u001b[33m   |\u001b[0m                          It would fail on pattern case: Tree.Leaf(_)\n\u001b[33m   |\u001b[0m\n\u001b[33m   |\u001b[0m longer explanation available when compiling with `-explain`"
}


[Trace - 00:47:25 pm] Received notification 'build/logMessage'
Params: {
  "type": 2,
  "message": "one warning found"
}


[Trace - 00:47:25 pm] Received notification 'build/logMessage'
Params: {
  "type": 3,
  "message": "done compiling"
}


[Trace - 00:47:25 pm] Received notification 'build/publishDiagnostics'
Params: {
  "textDocument": {
    "uri": "file:///Users/ckipp/Documents/scala-workspace/actionable-diagnostic/src/main/scala/example/Hello.scala"
  },
  "buildTarget": {
    "uri": "file:/Users/ckipp/Documents/scala-workspace/actionable-diagnostic/#root/Compile"
  },
  "diagnostics": [
    {
      "range": {
        "start": {
          "line": 17,
          "character": 26
        },
        "end": {
          "line": 17,
          "character": 30
        }
      },
      "severity": 2,
      "source": "sbt",
      "message": "\u001b[33mmatch\u001b[0m may not be exhaustive.\n\nIt would fail on pattern case: Tree.Leaf(_)\n",
      "relatedInformation": []
    }
  ],
  "reset": true
}

I'm not really sure why this is happening now.

@tgodzik
Copy link
Contributor

tgodzik commented Aug 1, 2023

I think this was already fixed in sbt/zinc#1225 ?

We had same issues when discussing sbt/zinc#1214

@ckipp01
Copy link
Member

ckipp01 commented Aug 1, 2023

I think this was already fixed in sbt/zinc#1225 ?

We had same issues when discussing sbt/zinc#1214

Yea, it's just odd that we didn't hit on this before with the other examples for Scala 3. I'm trying to update https://github.com/lampepfl/dotty/blob/10503b0345550259e9381f4fe59da80fb35b32b9/sbt-bridge/src/dotty/tools/xsbt/CompilerBridgeDriver.java#L139-L140 right now which I also believe might be problematic here because it's not using the new problem2.

@ckipp01 ckipp01 force-pushed the code-action-missing-cases branch from 6ddd045 to 8386b64 Compare August 3, 2023 07:02
@ckipp01
Copy link
Member

ckipp01 commented Aug 3, 2023

Alright I hope it's ok, but I just rebase this @ghostbuster91 because I want to test it now that #18322 is merged in.

@ghostbuster91
Copy link
Contributor Author

Alright I hope it's ok, but I just rebase this @ghostbuster91 because I want to test it now that #18322 is merged in.

Nice, thanks:) Could you also share how you are testing it together with these e2e tests with metals?

@ckipp01
Copy link
Member

ckipp01 commented Aug 3, 2023

Nice, thanks:) Could you also share how you are testing it together with these e2e tests with metals?

Sure, I know you're using nvim-metals as well, so the steps should be the same:

  1. Do a sbt publishLocal on this repo
  2. Create a minimal sbt project using 1.9.3
  3. Set the scalaVersion to the version you just published locally
  4. Switch your build server to use sbt instead of Bloop
  5. Then just go into your main file and drop the code from your test in there. You should see a diagnostic and be able to trigger a code action on it.
  6. If you don't you can start debugging by creating a .metals/bsp.trace.json file and look in the publishDiagnostics and see what they look like, or if they are there at all.

O and make sure you're on the latest release of Metals.

Very cool 😄

2023-08-03 09 25 31

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

This looks great @ghostbuster91! Thanks for doing this! Just one small nit on the test, let's make sure we also have one with significant whitespace being used just to be safe.

@ckipp01 ckipp01 assigned ghostbuster91 and unassigned ckipp01 Aug 3, 2023
@ghostbuster91
Copy link
Contributor Author

This looks great @ghostbuster91! Thanks for doing this! Just one small nit on the test, let's make sure we also have one with significant whitespace being used just to be safe.

Thanks for making it possible :)

@ghostbuster91 ghostbuster91 marked this pull request as ready for review August 5, 2023 09:43
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks a lot @ghostbuster91! LGTM

@ckipp01 ckipp01 requested a review from dwijnand August 9, 2023 12:09
@ckipp01
Copy link
Member

ckipp01 commented Aug 9, 2023

It's not actually letting me merge. @dwijnand I think since you requested changes it's waiting for your approval.

@dwijnand dwijnand removed their request for review August 9, 2023 12:27
@dwijnand dwijnand merged commit c5adafc into scala:main Aug 9, 2023
Kordyjan added a commit that referenced this pull request Dec 8, 2023
… to LTS (#19155)

Backports #18314 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan added this to the 3.3.2 milestone Dec 14, 2023
@ghostbuster91 ghostbuster91 deleted the code-action-missing-cases branch January 16, 2024 17:06
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.

6 participants