check for duplicate diagnostics before adding new one

[nachiket87]

possible fix #275

Alternatively, I tried checking if service.refresh() was called multiple times unnecessarily but that was not the case.

[marcoroth]

I guess this only really fixes the symptoms, but I wonder why they show up multiple times in the first place. It seems like there's another issue somewhere deeper in the stack

[marcoroth]

This might still be worthwhile to add, nonetheless

[nachiket87]

This might still be worthwhile to add, nonetheless

Hmm okay if you think so.

You are right though, I hesitated before opening this PR because i realise it only fixes the sypmtom.

My initial theory was the service.refresh() was called multiple times but that is not the case. I can continue investigating.

[marcoroth]

The issue is here:

stimulus-lsp/server/src/diagnostics.ts

Line 324 in 93fdd53

sourceFile.importDeclarations.forEach((importDeclaration) => {

Since the linked issue is importing two exports we also have two "import declarations" in the array, one for each import, thus resulting in the 4 diagnostics.

import { Controller, Context } from "stimulus"

But I'm not sure why we see 4 in that case and not just two.

[nachiket87]

@marcoroth okay, so I may have found the bug.

There are two locations from where diagnostic#validateStimulusImports is run.

This causes two diagnostics to be added.

1. stimulus-lsp/server/src/server.ts

   Line 102 in 27a7283

   connection.onDidChangeWatchedFiles((_params) => {

2. stimulus-lsp/server/src/service.ts

   Line 59 in 27a7283

   this.documentService.onDidChangeContent((change) => {

Here is a similar discussion of the issue: microsoft/vscode-discussions#511

What do you think we should do? imo we remove the onDidChangeWatchedFiles as it handles an edge case where the file could be edited outside of VSCode.

EDIT - however, I'm not sure if this would work if the LSP is used outside of VScode e.g Neovim etc.

[nachiket87]

okay upon further investigation, the issue does not lie in what I previously thought^

I am quite certain It is this event and the usage that is causing the issue:

stimulus-lsp/server/src/server.ts

Lines 102 to 108 in 27a7283

	connection.onDidChangeWatchedFiles((_params) => {
	// params.changes.forEach((event) => {
	// service.refreshFile(event)
	// })
	service.refresh()
	})

the service.refresh() call is adding multiple diagnostics.

[marcoroth]

Oh interesting, thanks for the investigation @nachiket87!

After thinking about this a bit more, it feels like this diagnostic should anyway be part of the Stimulus Parser itself and not part of the LSP. So maybe if we move the analysis to the parser we could solve the issue that way, would that make sense in your eyes?

[nachiket87]

Oh interesting, thanks for the investigation @nachiket87!

After thinking about this a bit more, it feels like this diagnostic should anyway be part of the Stimulus Parser itself and not part of the LSP. So maybe if we move the analysis to the parser we could solve the issue that way, would that make sense in your eyes?

Sure! I've been looking at the stimulus parser for this and at the moment I have no idea how I would implement it. I will continue trying though and let you know how it goes.

[marcoroth]

Hey @nachiket87, FWIW, I still think this could be useful to add.

[nachiket87]

Hey @nachiket87, FWIW, I still think this could be useful to add.

Really @marcoroth ? okay, do you have any feedback on this? Sorry I didn't come back to this.

[marcoroth]

I don't have any feedback right now, I just wanted to try to find the root cause and fix it and then merge this one