Different notifications for the same syntax check error

[bmeneg]

Hi @richterger,

While working on a bug for coc-perl (bmeneg/coc-perl#19) I noticed that the server is sending two different syntax check notifications for the same piece of code to the client:

1. reports problems on the code

   LS: <--- Notification: {
   "jsonrpc" : "2.0",
   "params" : {
     "uri" : "file:///home/bmeneg/test/perl/test.pl",
     "diagnostics" : [
        {
           "message" : "syntax error at /home/bmeneg/test/perl/test.pl line 9, near \"my \"",
           "severity" : 1,
           "source" : "perl syntax",
           "range" : {
              "start" : {
                 "line" : 8,
                 "character" : 0
              },
              "end" : {
                 "line" : 9,
                 "character" : 0
              }
           }
        },
        {
           "range" : {
              "end" : {
                 "line" : 15,
                 "character" : 0
              },
              "start" : {
                 "line" : 14,
                 "character" : 0
              }
           },
           "message" : "Bareword \"hello\" not allowed while \"strict subs\" in use at /home/bmeneg/test/perl/test.pl line 15.",
           "severity" : 1,
           "source" : "perl syntax"
        }
     ]
   },
   "method" : "textDocument/publishDiagnostics"
   }

2. reports no problem at all

   LS: <--- Notification: {
   "method" : "textDocument/publishDiagnostics",
   "params" : {
     "uri" : "file:///home/bmeneg/test/perl/test.pl",
     "diagnostics" : []
   },
   "jsonrpc" : "2.0"
   }

   Since the structure holding these notifications is a hash, their publishing (to the client) ordering might change on each server check. With that, whenever the notification reporting no issues is sent last, it overrides the correct notification where there are problems. The culprit is the presence of a filename - present inside the %diag hash on add_diagnostic_messages() subroutine, which I still didn't understand why it's being used. So, when the following code runs, the - filename changes to $uri on $fnuri (which is used as the actual filename) but the diagnostic message, $diags{$filename}, is empty because the $filename is still -.

https://github.com/richterger/Perl-LanguageServer/blob/68efd7b70070e9dd704d53727e99b5af7ddbb4b0/lib/Perl/LanguageServer/Workspace.pm#L349-L358

But, besides the above issue, in this very same code there is another bug (less impactful though): on commit f23ad050c253e, the diagnostic creation was moved from background_checker() to its own subroutine add_diagnostic_messages(), and the for-loop sending the notification got nested inside a second, and exact the same, loop, sending multiple times the same notifications:

https://github.com/richterger/Perl-LanguageServer/blob/68efd7b70070e9dd704d53727e99b5af7ddbb4b0/lib/Perl/LanguageServer/Workspace.pm#L345-L363

I'll send a pull-request fixing both issues, but I would like to understand why - is even used as a valid $filename.

[richterger]

The '-' filename comes when stdin is used. 5ddf9c66e0cfaa07e89107bece5078cd9a8e2e4c should fix this and also the double nested loop

[bmeneg]

@richterger oh, you already merged 5ddf9c66e0cfaa07e89107bece5078cd9a8e2e4c ? It didn't solve the problem. The issue of receiving one notification for the actual file and another, right after, for - is still happening:

LS: <--- Notification: {
   "jsonrpc" : "2.0",
   "method" : "textDocument/publishDiagnostics",
   "params" : {
      "uri" : "file:///home/bmeneg/test/perl/test.pl",
      "diagnostics" : [
         {
            "source" : "perl syntax",
            "message" : "Semicolon seems to be missing at /home/bmeneg/test/perl/test.pl line 7.",
            "range" : {
               "end" : {
                  "line" : 7,
                  "character" : 0
               },
               "start" : {
                  "line" : 6,
                  "character" : 0
               }
            },
            "severity" : 1
         },
         {
            "range" : {
               "end" : {
                  "character" : 0,
                  "line" : 8
               },
               "start" : {
                  "line" : 7,
                  "character" : 0
               }
            },
            "message" : "syntax error at /home/bmeneg/test/perl/test.pl line 8, near \"dosomething\"",
            "source" : "perl syntax",
            "severity" : 1
         },
         {
            "source" : "perl syntax",
            "message" : "Global symbol \"$somevar\" requires explicit package name (did you forget to declare \"my $somevar\"?) at /home/bmeneg/test/perl/test.pl line 11.",
            "range" : {
               "start" : {
                  "character" : 0,
                  "line" : 10
               },
               "end" : {
                  "line" : 11,
                  "character" : 0
               }
            },
            "severity" : 1
         }
      ]
   }
}

LS: <--- Notification: {
   "method" : "textDocument/publishDiagnostics",
   "params" : {
      "uri" : "file:///home/bmeneg/test/perl/test.pl",
      "diagnostics" : []
   },
   "jsonrpc" : "2.0"
}

diagnostics => $diags{$filename} is still getting $diags{-}, which is empty.

[richterger]

You are right, the commit fixes it only partly. I have made one additional change (27c71b9c0e24de1a37925614b5648b695ee1959c), that hopefully now fixes it completly

[bmeneg]

@richterger , the issue is now solved. Would it be possible to release a new version/release (minor) to include it? Considering this bug directly impacts users experience quite badly.

[richterger]

This was the last issue I was waiting for, for the 2.6 release. The release is on the way...