No unification of build and intellisense diagnostics in VS2017 (was in VS2015)

[dsyme]

There are likely multiple issues with the Visual Studio Error list.. I'm splitting out one known repro for a basic one, which is the lack of "Error Unification" between build and intellisense errors.

1. open any solution, e,g,

   devenv tests\projects\misc\TestProjectChanges\TestProjectChanges.sln

2,. add an error, watch intelliense error appear

3. do a build, wathc duplicate copy of "build" error appear

4. fix the error - the "intellisense" copy goes away, but the red suiglly stays

5. do a build - the "build" error goes away

Although logical, it is painful to have the red squiggly stay around even after you've fixed the error. In practice this means that once you do a build, and there are errors, you have to keep doing builds do clear all your red-squigglies, or else resort to showing Intellisense errors only.

This is a regression (or at least a change) from how F# worked in VS2015 and before, when build and intellisense errors were "unified" and fixing an error made it disappear from the error list - the user just was not particularly aware of the distinction between build and intellisense errors.

Now I'm not sure how deeply this distinction is built into Roslyn - it may not be something we control anymore - advice welcome please @Pilchie @DustinCampbell

Note that the diagnostic ID is also missing for the build error - this could be the underlying problem since some comments in the F# language service code imply that error de-duplication should be ocurring. It's possible this is a problem with whichever error parser is being used by default

Here is the output from the build, it contains FS0039

3>C:\GitHub\dsyme\visualfsharp\tests\projects\misc\TestProjectChanges\TestProjectChanges\Program.fs(7,58): error FS0039: The field, constructor or member 'PropertyA' is not defined. Maybe you want one of the following: PropertyAlwaysAvailable
3>Done building project "TestProjectChanges.fsproj" -- FAILED.

[dsyme]

(Note, I haven't yet compared to C# roslyn behavior)

[BentTranberg]

@dsyme, I compared C# and F# in VS 2015 and VS 2017, and the results is in a comment a bit down in #2548. I don't understand that much of this, but it looks to me there's pretty much the same issue in VS 2015, but just not that obvious to the user.

[dsyme]

@BentTranberg Ah yes, thanks. Good analysis. TBH I much prefer the "unified error list" model of F# in VS2015 (all errors are just one kind of error), I wonder why C#/roslyn didn't adopt this model.

[dsyme]

One of the odd things here is that this recently added code seems to indicate that we are trying to do error unification.

    // Normalize the error message into the same format that we will receive it from the compiler.
    // This ensures that IntelliSense and Compiler errors in the 'Error List' are de-duplicated.
    // (i.e the same error does not appear twice, where the only difference is the line endings.)

However the displayed error message in the error list above still has newline characters. Odd, will debug through this

[saul]

@dsyme we are trying but we aren't reporting build errors through Roslyn, so it doesn't see the build/intellisense errors as identical (as the former don't have error IDs amongst other data). I believe I put more info in my PR: #2839 (comment)

[dsyme]

we are trying but we aren't reporting build errors through Roslyn, so it doesn't see the build/intellisense errors as identical (as the former don't have error IDs amongst other data). I believe I put more info in my PR...

Thanks, that's very helpful! Here's a copy of the relevant comment:

F# build errors are passed to the IDE via the ErrorListProvider, which is wrapped by a Microsoft.VisualStudio.Shell.TaskProvider. See DocumentTask.cs@506 in the F# code.

I think the correct solution for this is to move over to Roslyn's ProjectExternalErrorReporter. We currently don't report any errors through this, but this class implements IVsLanguageServiceBuildErrorReporter2 so we can report build errors through the ReportError2 method. This is what the Roslyn project system does.

[dsyme]

More notes. The current F# project system has IDEBuildLogger which is added to MSBuild loggers in DoMSBuildSubmission

                loggerList.Add(buildLogger);

MSBuild then raises events and these get pushed to ErrorHandler and WarningHandler and thus to AddToErrorList. This creates a DefaultCompilerError (which does contain the error code) and a DocumentTask which doesn't. The task gets registered using taskReporter.AddTask. As @saul mentions, this last call should be removed and we should probably use ProjectExternalErrorReporter

[dsyme]

@Pilchie I'd appreciate some guidance here: does Roslyn come with any mechanism for listening to build output (e.g. from an MSBuild build logger) and creating matching tasks from the Error and Warning items that come from MSBuild? And matching those to the text being shown in the output window?

The context is that F# intellisense errors (via roslyn) and the F# build errors (via non-roslyn) are not being de-duplicated. This is because of subtle differences in the content of the error/task items being produced by these two paths (slightly different text in multi-line error messages, and a missing ID for the non-Roslyn errors). Ideally we want to produce all errors via roslyn to better ensure consistency. But we don't want to reinvent the wheel here.

[dsyme]

@saul @Pilchie I notice we do already create our own ProjectExternalErrorReporter, perhaps we just have to use that

Glancing around the Roslyn code I see GetExternalErrorReporter but also note there is no F# mapping here, which must be why we create our own. It would be helpful to start removing the C#/VB-specific things from Roslyn and just make it a multi-language framework.

[dsyme]

@saul @Pilchie

I took a look at what it would take to junk the existing F# code for build task creation and use ProjectExternalErrorReporter. A draft of what would be needed is here:
https://github.com/Microsoft/visualfsharp/compare/master...dsyme:eld?expand=1

@saul If you get the chance could you take a glance over that draft and see what you think? The idea is to literally delete the use of TasKReporter and TaskProvider in the ProjectSystem.Base and replace it with a propagated IVsLanguageServiceBuildErrorReporter2 (i.e. a ProjectExternalErrorReporter)

[Pilchie]

@heejaechang has pretty much done everything related to the Error List since Roslyn started, and I'll admit that I've lost track of it.

HeeJae - can you confirm my understanding is correct?

IIRC, Roslyn has no ability to insert msbuild loggers or anything, but the C# project system looks for error output and creates the "Build" tasks. Roslyn only reports the "IntelliSense" errors from it's in-memory model.