Missing syntax diagnostics in VS

[0101]

Repro steps

Create new Console App project with the following code:

type MyType() =

    member this.Foo(): MyType = {
        Field = 4
    }

    one two three

Expected behavior

We see errors in Error List in VS when we open the file.

Actual behavior

No errors shown when we open the file. But the errors do appear if the file is closed and project is built.

bugerrorpane.mp4

Related information

Appeared somewhere between 17.8 preview 1 and preview 3

[0101]

A current F# extension in VS 17.8 preview 1 works correctly, so it looks like some change in VS or Roslyn caused this issue. @CyrusNajmabadi can you have a look please?

It seems like results from IFSharpDocumentDiagnosticAnalyzer.AnalyzeSyntaxAsync are somehow getting lost. The semantic diagnostics work fine.

[0101]

@mavasani Did you get a chance to have a look at this?

[mavasani]

@0101 thanks for the reminder. Let me look tomorrow.

[mavasani]

@0101 I am unable to repro this on the latest dogfood build

[0101]

@mavasani that is most likely the workaround #15982

When I disable the workaround I can still reproduce the issue in VS Main Version 17.8.0 Preview 3.0 [34119.376.main]

With the workaround, the diagnostics that should be coming from IFSharpDocumentDiagnosticAnalyzer.AnalyzeSyntaxAsync
are also reported from AnalyzeSemanticsAsync.

When it's fixed the syntax diagnostics will mostly likely be doubled in the error list.

I can make an F# VSIX with the workaround removed if it would help you.

[mavasani]

Thanks @0101 - I was just skimming over your analyzer code and noticed you have below: https://github.com/dotnet/roslyn/blob/1421a4a8e7c48f4d7fda5e053ab95ffb9f4f35f5/src/Tools/ExternalAccess/FSharp/Internal/Diagnostics/FSharpDocumentDiagnosticAnalyzer.cs#L95-L98

        public DiagnosticAnalyzerCategory GetAnalyzerCategory()
        {
            return DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;
        }

The above category indicates the analyzer only supports semantic diagnostics, and the fact that it worked before might indicate some underlying bug that got fixed. You can read up the doc comments on each of the flags here: https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Diagnostics/DiagnosticAnalyzerCategory.cs

Are you able to locally make the below Roslyn change + remove the F# workaround and see if things work for you as expected with both these VSIXes installed? If not, I can investigate further.

        public DiagnosticAnalyzerCategory GetAnalyzerCategory()
        {
            return DiagnosticAnalyzerCategory.SyntaxTreeWithoutSemanticsAnalysis | DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;
        }

[0101]

I can try. But when debugging it, I saw that AnalyzeSyntaxAsync was still being called, just the results that we provided weren't shown.

If it was looking at capabilities then I'd expect it not to be called at all.

[mavasani]

I can try. But when debugging it, I saw that AnalyzeSyntaxAsync was still being called, just the results that we provided weren't shown.

If it was looking at capabilities then I'd expect it not to be called at all.

Indeed, that seems like another bug, filed #16008 to track fixing that on Roslyn side.

[0101]

So I made the change in GetAnalyzerCategory and it didn't help. I installed:

Roslyn.VisualStudio.Setup.Dependencies.vsix
Roslyn.VisualStudio.Setup.vsix
RoslynDeployment.vsix

[mavasani]

So I made the change in GetAnalyzerCategory and it didn't help. I installed:

Roslyn.VisualStudio.Setup.Dependencies.vsix
Roslyn.VisualStudio.Setup.vsix
RoslynDeployment.vsix

Alright, let me investigate

[mavasani]

@0101 I am still unable to reproduce this issue on Version 17.8.0 Preview 3.0 [34118.359.main] build. I have tried the following:

1. Tried the scenario without any local changes/patch and things work fine
2. Tried the scenario by commenting out the AnalyzeSemanticsAsync call below, which is the single place where this callback is made, and verified things work fine: https://github.com/dotnet/roslyn/blob/1421a4a8e7c48f4d7fda5e053ab95ffb9f4f35f5/src/Features/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor_Helpers.cs#L239-L244
3. Once I revert the change in 2 above, I see that no syntax errors are returned by AnalyzeSemanticsAsync call, so most likely I don't have the F# workaround you mentioned above.

I tried the above scenarios with both the below entire solution analysis checkbox being checked and unchecked and see the same behavior:

@0101 I am not sure how to proceed here, as everything seems to work fine for me on the above build without the F# workaround

[0101]

Ok I'll try to get that exact version and see what I see. Though I doubt it was fixed and then broken again by 34119.376.main. If it still doesn't work for me than it means it's probably affected by some settings?

[0101]

Hmm, when I installed INT preview I got 34119.384.main. Which includes the workaround and reproduces the issue (with VSIX without the workaround)

Can you maybe try to update? But still there is probably something external that made it work for you but not for us.

[mavasani]

@0101 I upgraded to the latest build, no local changes or VSIXes and still can't reproduce the issue. I see duplicate syntax errors:

still there is probably something external that made it work for you but not for us.

I believe this is the case. I also tried toggling the entire solution analysis checkbox, but that made no difference.

[mavasani]

Fixed with dotnet/roslyn#70068

[T-Gro]

Fixed with the roslyn fix above.

[0101]

Should we revert #15982 now?

[T-Gro]

IMO yes.

[vzarytovskii]

Should we revert #15982 now?

Probably. No rush though since p1 is m2 already.

[brianrourkeboll]

Should we revert #15982 now?

Does this still need to happen?

[vzarytovskii]

Should we revert #15982 now?

Does this still need to happen?

I guess it's not urgent, but probably need to revert it and test it. Might look at it once in the office. Let me make issue out of it.