Fix error list unification #3071

dsyme · 2017-05-17T18:05:47Z

This fixes error list unification #3047.

See discussion on dotnet/roslyn#19561. This even appears to work for multi-line error messages (which I had expected to be problematic)

The technique is to hack the old FSharp.ProjectSystem.Base to use the Roslyn error-reporting mechanism rather than the hard-wired one there. Quite a lot of old project system code can be deleted as a result (we should probably keep deleting unused features from that code to help with the migration to http://github.com/dotnet/project-system)

This requires that Roslyn fix, so we have to see when that makes it in - hopefully soon! It is important to note that we will require the Roslyn fix to be in place, if not Visual Studio will crash when it hits the call to FailFast mentioned in that Roslyn thread.

Separately this makes the Symbol API's EnclosingEntity return an option, I may factor that out into a separate PR

To consider (I don't think these are blocking)

manual stress testing with many errors etc (though the fix would always be with Roslyn, and we can probably assume that this is working ok)
consider re-enabling the automated tests in vsintegration/tests/unittests/Tests.TaskReporter.fs, which were effectively disabled (they were testing unused functionality) and are new fully disabled (since part of that functionality has now been replaced in ProjectSystem.Base), though this would effectively be re-testing Roslyn's logic in a somewhat unconvincing way.

dsyme · 2017-05-17T18:07:52Z

I wanted to add @saul and @srivatsn as reviewers here but for some reason they aren't selectable - @KevinRansom do we have to add them as project collaborators?

saul

Looks good - I love seeing huge swathes of code deleted! 😄 Just got a few comments.

saul · 2017-05-17T19:38:08Z

vsintegration/Vsix/VisualFSharpFull/VisualFSharpFull.csproj

@@ -43,7 +43,7 @@
    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
    <OutputType>Library</OutputType>
    <AppDesignerFolder>Properties</AppDesignerFolder>
-    <VSRootSuffix>FSharpDev</VSRootSuffix>
+    <VSRootSuffix>RoslynDev</VSRootSuffix>


Whoa this is wrong! Or we could merge FSharpDev with RoslynDev? I'm not massively fussed - it will help with cross-project dev.

saul · 2017-05-17T19:38:13Z

vsintegration/Vsix/VisualFSharpOpenSource/VisualFSharpOpenSource.csproj

@@ -43,7 +43,7 @@
    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
    <OutputType>Library</OutputType>
    <AppDesignerFolder>Properties</AppDesignerFolder>
-    <VSRootSuffix>FSharpDev</VSRootSuffix>
+    <VSRootSuffix>RoslynDev</VSRootSuffix>


saul · 2017-05-17T19:39:00Z

vsintegration/src/FSharp.Editor/LanguageService/FSharpCheckerExtensions.fs

@@ -112,16 +112,16 @@ type FSharpChecker with
                            if func.IsProperty then
                                let fullNames =
                                    [|  if func.HasGetterMethod then
-                                            yield func.GetterMethod.EnclosingEntity.TryGetFullName()
+                                            yield func.GetterMethod.EnclosingEntity.Value.TryGetFullName()


Are we sure this isn't going to blow up?

saul · 2017-05-17T19:41:05Z

vsintegration/src/FSharp.ProjectSystem.Base/Project/IDEBuildLogger.cs

-                // remove all Project System tasks from the task list, add everything else to the task set
-                taskReporter.ClearAllTasks();
+                if (errorReporter != null) { 
+                    //UIThread.Run(delegate ()


Does this need to run on the UI thread? Can this comment be removed?

saul · 2017-05-17T19:41:32Z

vsintegration/src/FSharp.ProjectSystem.Base/Project/IDEBuildLogger.cs

                {
-                    this.taskProvider.Tasks.Add(task);
+                    errorReporter.ReportError2(taskText, errorCode, (VSTASKPRIORITY) priority, span.iStartLine, span.iStartIndex, span.iEndLine, span.iEndIndex, file);


How are warnings handled? I'm not even sure that warnings show in the Error List anyway - just something to consider.

The "priority" indicates information/warning/error. I'll add a note to double check once the roslyn fix is in

saul · 2017-05-17T19:43:32Z

vsintegration/src/FSharp.ProjectSystem.FSharp/Project.fs

@@ -382,16 +399,16 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
                member this.GetMostRecentLines(n:int) : string[] =
                    GetToolWindowAsITestVFSI().GetMostRecentLines(n)

+            // Can we get rid of this now?


If we're not calling anything here, then yes, this can go. Same with the registration for this IOleComponent

saul · 2017-05-17T19:44:37Z

vsintegration/src/FSharp.LanguageService.Base/DocumentTask.cs

@@ -182,807 +182,5 @@ internal static void DoOnUIThread(Action callback)
        }
    }

-    // DocumentTask is associated with an IVsTextLineMarker in a specified document and 


Can we rename this file to UIThread.cs now that it only contains the UIThread class?

I don't touch the old project system code except to delete stuff

…o eld

dsyme · 2017-05-18T00:21:46Z

OK, all reviewed. Now this just sits around until the Roslyn fix is available :)

dsyme · 2017-05-18T00:50:39Z

An odd error in CI, seems unrelated

1) Failed : Tests.ProjectSystem.Project.RenameFile.BuildActionIsResetBasedOnFilenameExtension
no exception expected, but got System.InvalidOperationException: The service 'Microsoft.Internal.VisualStudio.Shell.Interop.SVsUIThreadInvokerPrivate' must be installed for this feature to work.  Ensure that this service is available. HRESULT = 0x8000FFFF
   at Microsoft.VisualStudio.Shell.ThreadHelper.GetInvoker()
   at Microsoft.VisualStudio.Shell.ThreadHelper.InvokeOnUIThread(InvokableBase invokable)
   at Microsoft.VisualStudio.Shell.ThreadHelper.Invoke(Action action)
   at Microsoft.VisualStudio.FSharp.ProjectSystem.FileNode.PromptYesNoWithYesSelected(String message, String title, OLEMSGICON icon, IVsUIShell uiShell) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\src\FSharp.ProjectSystem.Base\Project\FileNode.cs:line 351
   at Microsoft.VisualStudio.FSharp.ProjectSystem.FileNode.SetEditLabel(String label) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\src\FSharp.ProjectSystem.Base\Project\FileNode.cs:line 342
   at <StartupCode$VisualFSharp-Unittests>.$Tests.ProjectSystem.Project.RenameFile-BuildActionIsResetBasedOnFilenameExtension@802.Invoke(UnitTestingFSharpProjectNode project) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\Tests.ProjectSystem.Project.fs:line 812
at <StartupCode$VisualFSharp-Unittests>.$Tests.ProjectSystem.Project.RenameFile-BuildActionIsResetBasedOnFilenameExtension@802.Invoke(UnitTestingFSharpProjectNode project) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\Tests.ProjectSystem.Project.fs:line 814
at <StartupCode$VisualFSharp-Unittests>.$TestLib.ProjectSystem.MakeProjectAndDoWithProjectFileAndConfigChangeNotifierDispose@311.Invoke(String file) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\TestLib.ProjectSystem.fs:line 323
at UnitTests.TestLib.Utils.FilesystemHelpers.DoWithTempFile[a](String filename, FSharpFunc`2 f) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\TestLib.Utils.fs:line 73
at UnitTests.TestLib.ProjectSystem.TheTests.MakeProjectAndDoWithProjectFileAndConfigChangeNotifierDispose[d](Boolean dispose, FSharpList`1 compileItems, FSharpList`1 references, String other, String targetFramework, FSharpFunc`2 f) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\TestLib.ProjectSystem.fs:line 311
at UnitTests.TestLib.ProjectSystem.TheTests.MakeProjectAndDoWithProjectFile(FSharpList`1 compileItems, FSharpList`1 references, String other, String targetFramework, FSharpFunc`2 f) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\TestLib.ProjectSystem.fs:line 340
at UnitTests.TestLib.ProjectSystem.TheTests.MakeProjectAndDo(FSharpList`1 compileItems, FSharpList`1 references, String other, String targetFramework, FSharpFunc`2 f) in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\TestLib.ProjectSystem.fs:line 351
at Tests.ProjectSystem.Project.RenameFile.BuildActionIsResetBasedOnFilenameExtension() in D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\Tests.ProjectSystem.Project.fs:line 802

saul · 2017-06-14T09:45:37Z

@dsyme any update on this? This should be available in Roslyn binaries

dsyme · 2017-06-15T18:20:51Z

@saul I will get back to it. I've been away at a conference this week and I'll get back into engineering mode over the next few working days

If you would like to finish it off please send an additional PR based on Pilchie's option #2 here: #3047 (comment)

dsyme · 2017-06-22T20:36:54Z

@saul I will leave it up to @brettfo as to whether we integrate in 15.3 branch or not. I'd like to but only very few fixes are being taken in that branch right now

If it doesn't go into that branch then as soon as 15.3 is out we will update master to use the right 15.3 Roslyn binaries. Then we can go ahead and pull this.

dsyme · 2017-06-22T22:10:05Z

@dotnet-bot test Windows_NT Release_ci_part2 Build please

brettfo · 2017-06-22T22:20:46Z

@saul and @dsyme: Due to changing APIs, master has to target Roslyn 2.0 until 15.3 ships (as RTW, not a preview), but vs2017-rtm is currently targeting 15.3. Once 15.3 ships then vs2017-rtm will be merged back into master, so this PR can either be retargeted to vs2017-rtm and merged today-ish or we can wait until 15.3 ships (and vs2017-rtm merges back into master).

We try to not get into tight spots like this, but due to VS 15.0, 15.1, and 15.2 all reporting assembly versions of 15.0 and 15.3 reporting 15.3, we're in this mess.

dsyme · 2017-06-22T22:36:57Z

@brettfo It your call with @cartermp in the end (and may depend on whether we need to take it to VS ship room to get permission).

Ideally this fix would go into 15.3 but the problem is that I can't validate the fix as I don't have a 15.3 on any machine (and I'm not sure getting 15.3.preview2 would help - I really need 15.3 preview 3)

dsyme · 2017-07-07T13:46:30Z

This is ready once we can assume 15.3

cartermp · 2017-07-10T18:04:02Z

If we get this into 15.3, we'll have to roll it up with changes to the language service as a high-priority bug fix. Otherwise, this would have to go out via nightlies and later, 15.4. @Pilchie your thoughts?

KevinRansom · 2017-07-10T18:13:11Z

@cartermp the bar for 15.3 is "astronomical", which I take to mean as I can't even imagine what would qualify :-)

But @Pilchie tells me that we can get this into 15.4 without too much of a tussle.

dsyme · 2017-08-17T19:27:32Z

Now that 15.3 is out and master is using the fixed Roslyn binaries I've removed the WIP tag and I believe this is ready for commit

However we should re-check that it fixes the error list problem and that no new problems have been introduced

cartermp · 2017-08-17T20:53:13Z

@dsyme I get this assertion when opening VisualFSharp.sln with your PR after a build vs:

This is for FSharp.Compiler.Interactive.Settings, as that was the only project which failed to load after ignoring assertions

dsyme · 2017-08-17T22:20:03Z

That will be related to #3393 . What happens if you ignore the assert?

cartermp · 2017-08-17T22:35:59Z

I see. It's fine and the PR will build without that project. I cannot reproduce #3047 with this PR. I'm psuedo-randomly screwing around with code in Paket to see how it reacts. Nice and clean!

dsyme · 2017-08-17T22:38:10Z

@cartermp Thank you so much for doing that validation.

@KevinRansom This is good to merge now

dsyme · 2017-08-18T12:44:22Z

Since @cartermp has validated this, I'm going to pull it.

* try use roslyn build error reporter * undo changes * undo changes * undo changes * try use roslyn build error reporter * more work on unifying diagnostics * minimize diff * bang bang * code review feedback * remove old test

dsyme and others added 9 commits May 15, 2017 16:06

try use roslyn build error reporter

9dfc7bf

undo changes

677d974

undo changes

ec55275

undo changes

db97b23

try use roslyn build error reporter

79168b1

more work on unifying diagnostics

2badeb8

minimize diff

15e374a

bang bang

f504fd2

descriptor numbers needed

f3fd756

msftclas added the cla-not-required label May 17, 2017

saul suggested changes May 17, 2017

View reviewed changes

dsyme added 2 commits May 18, 2017 01:07

Merge branch 'master' of http://github.com/Microsoft/visualfsharp int…

c209447

…o eld

code review feedback

584d083

dsyme and others added 3 commits May 18, 2017 02:39

remove old test

bb879e0

integrate with master

20ec3f0

Merge branch 'eld' of https://github.com/dsyme/visualfsharp into eld

344b205

dsyme force-pushed the eld branch from 56e94fb to 344b205 Compare May 19, 2017 23:06

dsyme mentioned this pull request Jun 6, 2017

[WIP] Don't try to access if thread is not running #3174

Closed

saul mentioned this pull request Jun 19, 2017

Duplicate entries in error list and Intellisense - VS2017 #3211

Closed

dsyme mentioned this pull request Jun 20, 2017

FSharpCompletionProvider throws MissingMethodException in Visual Studio 2017 Update 3 Preview #3067

Closed

integrate

65e5558

NatElkins mentioned this pull request Jul 10, 2017

Duplicated warnings #3314

Closed

dsyme changed the title ~~[WIP] Fix error list unification (requires roslyn fix)~~ Fix error list unification Aug 17, 2017

Merge branch 'master' into eld

14343c9

cartermp approved these changes Aug 17, 2017

View reviewed changes

dsyme merged commit 8c44f0c into dotnet:master Aug 18, 2017

This was referenced Aug 18, 2017

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

Closed

FS0039 does not go away, even after rebuilding, commenting out, while compilation is successful #3472

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error list unification #3071

Fix error list unification #3071

dsyme commented May 17, 2017 •

edited

Loading

dsyme commented May 17, 2017

saul left a comment

saul May 17, 2017

saul May 17, 2017

saul May 17, 2017

saul May 17, 2017

saul May 17, 2017

dsyme May 18, 2017

saul May 17, 2017

saul May 17, 2017

dsyme May 18, 2017

dsyme commented May 18, 2017

dsyme commented May 18, 2017

saul commented Jun 14, 2017

dsyme commented Jun 15, 2017

dsyme commented Jun 22, 2017

dsyme commented Jun 22, 2017

brettfo commented Jun 22, 2017

dsyme commented Jun 22, 2017 •

edited

Loading

dsyme commented Jul 7, 2017

cartermp commented Jul 10, 2017

KevinRansom commented Jul 10, 2017

dsyme commented Aug 17, 2017

cartermp commented Aug 17, 2017

dsyme commented Aug 17, 2017

cartermp commented Aug 17, 2017

dsyme commented Aug 17, 2017

dsyme commented Aug 18, 2017

Fix error list unification #3071

Fix error list unification #3071

Conversation

dsyme commented May 17, 2017 • edited Loading

dsyme commented May 17, 2017

saul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented May 18, 2017

dsyme commented May 18, 2017

saul commented Jun 14, 2017

dsyme commented Jun 15, 2017

dsyme commented Jun 22, 2017

dsyme commented Jun 22, 2017

brettfo commented Jun 22, 2017

dsyme commented Jun 22, 2017 • edited Loading

dsyme commented Jul 7, 2017

cartermp commented Jul 10, 2017

KevinRansom commented Jul 10, 2017

dsyme commented Aug 17, 2017

cartermp commented Aug 17, 2017

dsyme commented Aug 17, 2017

cartermp commented Aug 17, 2017

dsyme commented Aug 17, 2017

dsyme commented Aug 18, 2017

dsyme commented May 17, 2017 •

edited

Loading

dsyme commented Jun 22, 2017 •

edited

Loading