dotnet / project-system

The .NET Project System for Visual Studio
MIT License
968 stars 387 forks source link

Design-time warnings/errors can get stale and stick around #1426

Closed davkean closed 7 years ago

davkean commented 7 years ago

I've noticed as I start playing around with projects and introducing warnings/errors during design-time - that these can be out-of-date and stale, here's an example:

  1. Create a new Console App
  2. Create a new Class Library
  3. Add a reference from Console App -> Class Library
  4. Edit Console App
  5. Add a reference to a non-existent project

Expected: A single warning Actual: Many warnings and as you make changes to the projects it grows:

Severity    Code    Description Project File    Line    Suppression State
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    
Warning     The referenced project 'D.csproj' does not exist.   ConsoleApp1 C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1742    

image

davkean commented 7 years ago

Ultimately, I think this is the cause of the majority of the warnings/errors we get from retargeting projects - basically, CPS kicks off a design-build at the wrong time, get an error, and then we never get rid of it.

davkean commented 7 years ago

In the above situation, you can never get rid of the design-time warnings.

davkean commented 7 years ago

The way this is supposed to happen is that design-time errors/warnings are supposed to go away when the same target is run again, this means two things:

1) This logic appears broken - so debugging this. 2) This means that we're going to have stale warnings when targets change underneath the project (ie when a target is removed from a project or when a target changes when it it scheduled to run, when we do remove the warnings?)

Oh and this logic is all in CPS.

davkean commented 7 years ago

Note this only occurs if there's another package reference.

davkean commented 7 years ago

Looks like these warnings are coming in without a target specified - when it should be ResolveProjectReferences.

davkean commented 7 years ago

~I suspect a regression when the TargetGeneratedError was changed to remove it's dependency on Shell.~ This is wrong.

davkean commented 7 years ago

The PR description: https://mseng.visualstudio.com/VSIDEProj/_git/VSIDEProj.CPS/pullrequest/184608?_a=overview#

When a target calls another target, such as in:



 <Target Name="Foo">
   <CallTarget Targets="Bar" />

   <Warning Text="This is foo" />

 </Target>

 <Target Name="Bar">
   <Warning Text="This is bar" />

 </Target>

> The VsErrorListLogger was currupting it’s state because it’s was assuming that only a single target could execute at once. This curruption was causing it to log the errors under the `<NOTARGET>` name, and never getting around to removing previously logged errors/warnings, causing them to grow continuously, such as in https://github.com/dotnet/roslyn-project-system/issues/1426.

> Due to the similar issue above of overlapping targets, it also has incorrect logic around filtering out warnings/errors that came from other projects. Tightened this so that we never respond to these events at all coming from other projects.
davkean commented 7 years ago

This has been fixed, and merged into RTM.