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

problemMatcher squiggles should track edits #102724

Closed
ljw1004 opened this issue Jul 16, 2020 · 9 comments
Closed

problemMatcher squiggles should track edits #102724

ljw1004 opened this issue Jul 16, 2020 · 9 comments
Assignees

Comments

@ljw1004
Copy link

ljw1004 commented Jul 16, 2020

I've written a problemMatcher. It works great and produces problems (error squiggles) in the document.

  1. Now I edit the document to insert a newline above the squiggle.
    • I would like the squiggle to automatically shift down one line.
    • (instead, the squiggle currently stays at the exact same file+line+column).
  2. Now I edit the document to completely delete the text that contained the squiggle.
    • I would like the squiggle to automatically be deleted.
    • (instead, the squiggle currently stays at the exact same file+line+column).
  3. If I edit the document to delete part of the text range of the squiggle, I'd like the squiggle's range to be modified accordingly.

Here's a video which shows how squiggles aren't currently auto-moved by file edits.
https://imgur.com/a/yWfq97Z

REPRO

Create a folder with three files:

[a.txt]
hello
there
world

[a.ts]
const x = 15;
f(x);

[.vscode/tasks.json]
{
  // See https://go.microsoft.com/fwlink/?LinkId=733558
  // for the documentation about the tasks.json format
  "version": "2.0.0",
  "tasks": [
    {
      "label": "ts problemMatcher",
      "type": "shell",
      "command": "printf 'a.ts'; echo ':2:1 - error TS2304: my walk has become rather sillier recently'",
      "group": "build",
      "problemMatcher": "$tsc"
    },
    {
      "label": "absolute problemMatcher at line 2",
      "type": "shell",
      "command": "echo 'e /Users/ljw/code/a.txt:2:1: warning: absolute warning in /Users/ljw/code/a.txt:2'",
      "group": "build",
      "problemMatcher": {
        "owner": "moose",
        "fileLocation": "absolute",
        "pattern": {
          "regexp": "^e (.*):(\\d+):(\\d+):\\s+(warning|error):\\s+(.*)$",
          "file": 1,
          "line": 2,
          "column": 3,
          "severity": 4,
          "message": 5
        }
      }
    },
    {
      "label": "relative problemMatcher at line 3",
      "type": "shell",
      "command": "echo 'e a.txt:3:3: error: relative warning in {workspace}/a.txt:3'",
      "group": "build",
      "problemMatcher": {
        "owner": "moose",
        "fileLocation": ["relative", "${workspaceFolder}"],
        "pattern": {
          "regexp": "^e (.*):(\\d+):(\\d+):\\s+(warning|error):\\s+(.*)$",
          "file": 1,
          "line": 2,
          "column": 3,
          "severity": 4,
          "message": 5
        }
      }
    }
  ]
}
  1. Run the "relative problemMatcher" build task.
  2. Observe that it produces a problem on line 3 of a.txt.
  3. Click on that problem to go to a.txt.
  4. Insert a newline at the start of the file

WHAT I EXPECT: the problem should be shifted down one line, tracking the text
WHAT I GET: the problem remains on line 3, new on a different span of text

For the repro, I also did it with an "absolute problemMatcher" and "$tsc" problem matcher (this one's easiest to see if you disable the built-in typescript language extension). They all have the same phenomenon: problems fail to track edits to the file.

@alexr00
Copy link
Member

alexr00 commented Jul 17, 2020

This has been discussed here

If you want the problem to update, you should run a a background task that uses a watch problem matcher. Otherwise, the problems will not update.

@alexr00 alexr00 closed this as completed Jul 17, 2020
@ljw1004
Copy link
Author

ljw1004 commented Jul 17, 2020

@alexr00 Thanks for the suggestion to use background-task and watch-problem-matcher. But I don't think that will work in two very typical scenarios: (1) if the task has a side-effect, e.g. "publish to azure", so you can't have it run in watch mode, (2) if the task is long-running e.g. 90s - imagine it produces three errors in a file, and you addressed the first one, and now you have to wait an unpredictable length of time before you can get the squiggles you need to work on the next. The Roslyn project for instance takes ~5mins to build. Developers commonly kick off a build, spend 20 minutes fixing issues, and only then do they deliberately and intentionally kick off a next build.

To recap from #90890

  1. @pkruk2 observed that squiggles were being moved around in response to edits, but the line+column shown in the Problems pane was being left as-is; @pkruk2 therefore requested that the Problems pane be updated also in response to edits.
  2. @sandy081 "fixed it" on April 6th 2020 404f66c by making squiggles no longer move around. I put "fixed it" in quotes because this was the opposite of what @pkruk2 and I hope for. @dbaeumer also observed that this fix is unexpected.

I understand the motive behind @sandy081's fix: the motive is that the task produced some output, and we scraped that output, and the output is sacrosanct and should never be edited, and it's weird for squiggles to disagree with what's shown in the problems pane, so therefore the squiggles shouldn't move in response to edits.

But although that conclusion is intellectually self-consistent, I can't imagine any scenarios or user-experiences where that behavior is actually helpful! So I think it's valuable to figure out a different answer. Indeed, I suspect the pre-April behavior was easier on users than the current.

Just to note: the full Visual Studio product has the pre-April behavior, where (1) it populates its Problems pane with the output of its problemMatcher but they remain unchanged, (2) however, squiggle locations do move in response to edits. Here's a video: https://imgur.com/a/fYZsAEI. [note1: in visual studio the squiggles from a build task are actually invisible, so they only way to tell where they are is by double-clicking on the error in the problems pane]. [note2: in visual studio the live-tracking of squiggles doesn't persist after a file has been closed, and indeed it can't].

Further context: I'd chatted with @dbaeumer two years ago about how we should reconcile the problems that come out of LSP, with those that come out of a built task. That's because not all LSPs can be as fast as typescript, and many LSPs only produce problems for the currently-open-file rather than for all files. The way Visual Studio resolves this is that the problemMatcher and build task are used to produce whole-project "blue" squiggles, and the Visual Studio LSPs only produce single-file "red" squiggles, and whenever you click on a blue error in the problem pane then the blue error gets silently and automatically surplanted by the red squiggle once the file is open.

With the current VSCode behavior, I guess my only hope is (1) use a problem-matcher, (2) write my own extension which moves problems upon text edits. I don't know if that's possible. I'll look.

For the VisualStudio repro: (1) create a new C# console application, (2) create a file "moose.bat" with contents

echo Program.cs(3,5-11): error CS1234: There's a moose loose about this hoose

and (3) edit ConsoleApp1.csproj to include these lines immediately before the final closing tag:

  <Target Name="AfterBuild">
    <Exec Command="moose.bat" IgnoreExitCode="true"/>
  </Target>

I also uploaded a complete self-contained VS project with the repro: VSbuildtask.zip

@dbaeumer
Copy link
Member

See also #74524

@alexr00
Copy link
Member

alexr00 commented Jul 30, 2020

@sandy081 "fixed it" on April 6th 2020 404f66c by making squiggles no longer move around. I put "fixed it" in quotes because this was the opposite of what @pkruk2 and I hope for.

@sandy081 would you consider changing this behavior?

@sandy081
Copy link
Member

sandy081 commented Aug 4, 2020

@alexr00 That would inconsistencies across workbench. Diagnostics will not represent same range. Right fix would be is to let that source/owner update diagnostics.

@pkrukp
Copy link

pkrukp commented Aug 4, 2020

I'm not sure I understand your comment - what are the disadvantages / problems of updating the diagnostics (maybe optionally/as opt-in for extension)? Could you give an example?

@sandy081
Copy link
Member

sandy081 commented Aug 4, 2020

I meant Diagnostics should be updated by their owners when document content changes, for eg., TS extension shall update TS diagnostics. VS Code cannot update them because it does not know the semantics and it was never updating them. Previously Squiggles were not updated when content changed which caused they inconsistencies between them and diagnostics as latter were not updated.

@pkrukp
Copy link

pkrukp commented Aug 4, 2020

In case extension can't simply re-generate diagnostics (for example due to side-effects or long generation time):

  • right now diagnostics are not very usable, they point to wrong location
  • if updated based on file edits, they could point to correct location

My argument is that in this case not updating diagnostics is worse than updating them.

@sandy081
Copy link
Member

sandy081 commented Aug 4, 2020

I filed a separate issue #103953 for discussion

@github-actions github-actions bot locked and limited conversation to collaborators Aug 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants