Merge pull request #19226 from CyrusNajmabadi/poisonProject

Do not allow 'add nuget reference' for projects that we fail calling into Nuget for.

src/EditorFeatures/CSharpTest/AddUsing/AddUsingTests_NuGet.cs

@@ -54,7 +54,7 @@ public async Task TestSearchPackageSingleName()
                 // Make a loose mock for the installer service.  We don't care what this test
                 // calls on it.
                 var installerServiceMock = new Mock<IPackageInstallerService>(MockBehavior.Loose);
-                installerServiceMock.SetupGet(i => i.IsEnabled).Returns(true);
+                installerServiceMock.Setup(i => i.IsEnabled(It.IsAny<ProjectId>())).Returns(true);
                 installerServiceMock.SetupGet(i => i.PackageSources).Returns(NugetPackageSources);
                 installerServiceMock.Setup(s => s.TryInstallPackage(It.IsAny<Workspace>(), It.IsAny<DocumentId>(), It.IsAny<string>(), "NuGetPackage", It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<CancellationToken>()))
                                     .Returns(true);
@@ -83,7 +83,7 @@ public async Task TestSearchPackageMultipleNames()
                 // Make a loose mock for the installer service.  We don't care what this test
                 // calls on it.
                 var installerServiceMock = new Mock<IPackageInstallerService>(MockBehavior.Loose);
-                installerServiceMock.SetupGet(i => i.IsEnabled).Returns(true);
+                installerServiceMock.Setup(i => i.IsEnabled(It.IsAny<ProjectId>())).Returns(true);
                 installerServiceMock.SetupGet(i => i.PackageSources).Returns(NugetPackageSources);
                 installerServiceMock.Setup(s => s.TryInstallPackage(It.IsAny<Workspace>(), It.IsAny<DocumentId>(), It.IsAny<string>(), "NuGetPackage", It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<CancellationToken>()))
                                     .Returns(true);
@@ -112,7 +112,7 @@ public async Task TestMissingIfPackageAlreadyInstalled()
                 // Make a loose mock for the installer service.  We don't care what this test
                 // calls on it.
                 var installerServiceMock = new Mock<IPackageInstallerService>(MockBehavior.Loose);
-                installerServiceMock.SetupGet(i => i.IsEnabled).Returns(true);
+                installerServiceMock.Setup(i => i.IsEnabled(It.IsAny<ProjectId>())).Returns(true);
                 installerServiceMock.SetupGet(i => i.PackageSources).Returns(NugetPackageSources);
                 installerServiceMock.Setup(s => s.IsInstalled(It.IsAny<Workspace>(), It.IsAny<ProjectId>(), "NuGetPackage"))
                     .Returns(true);
@@ -135,7 +135,7 @@ public async Task TestOptionsOffered()
                 // Make a loose mock for the installer service.  We don't care what this test
                 // calls on it.
                 var installerServiceMock = new Mock<IPackageInstallerService>(MockBehavior.Loose);
-                installerServiceMock.SetupGet(i => i.IsEnabled).Returns(true);
+                installerServiceMock.Setup(i => i.IsEnabled(It.IsAny<ProjectId>())).Returns(true);
                 installerServiceMock.SetupGet(i => i.PackageSources).Returns(NugetPackageSources);
                 installerServiceMock.Setup(s => s.GetInstalledVersions("NuGetPackage"))
                     .Returns(ImmutableArray.Create("1.0", "2.0"));
@@ -177,7 +177,7 @@ public async Task TestOptionsOffered()
             public async Task TestInstallGetsCalledNoVersion()
             {
                 var installerServiceMock = new Mock<IPackageInstallerService>(MockBehavior.Loose);
-                installerServiceMock.SetupGet(i => i.IsEnabled).Returns(true);
+                installerServiceMock.Setup(i => i.IsEnabled(It.IsAny<ProjectId>())).Returns(true);
                 installerServiceMock.SetupGet(i => i.PackageSources).Returns(NugetPackageSources);
                 installerServiceMock.Setup(s => s.TryInstallPackage(It.IsAny<Workspace>(), It.IsAny<DocumentId>(), It.IsAny<string>(), "NuGetPackage", /*versionOpt*/ null, It.IsAny<bool>(), It.IsAny<CancellationToken>()))
                                     .Returns(true);
@@ -205,7 +205,7 @@ class C
             public async Task TestInstallGetsCalledWithVersion()
             {
                 var installerServiceMock = new Mock<IPackageInstallerService>(MockBehavior.Loose);
-                installerServiceMock.SetupGet(i => i.IsEnabled).Returns(true);
+                installerServiceMock.Setup(i => i.IsEnabled(It.IsAny<ProjectId>())).Returns(true);
                 installerServiceMock.SetupGet(i => i.PackageSources).Returns(NugetPackageSources);
                 installerServiceMock.Setup(s => s.GetInstalledVersions("NuGetPackage"))
                     .Returns(ImmutableArray.Create("1.0"));
@@ -235,7 +235,7 @@ class C
             public async Task TestFailedInstallRollsBackFile()
             {
                 var installerServiceMock = new Mock<IPackageInstallerService>(MockBehavior.Loose);
-                installerServiceMock.SetupGet(i => i.IsEnabled).Returns(true);
+                installerServiceMock.Setup(i => i.IsEnabled(It.IsAny<ProjectId>())).Returns(true);
                 installerServiceMock.SetupGet(i => i.PackageSources).Returns(NugetPackageSources);
                 installerServiceMock.Setup(s => s.GetInstalledVersions("NuGetPackage"))
                     .Returns(ImmutableArray.Create("1.0"));

src/EditorFeatures/VisualBasicTest/Diagnostics/AddImport/AddImportTests_NuGet.vb

@@ -44,7 +44,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.CodeActions.AddImp
                 ' Make a loose mock for the installer service.  We don't care what this test
                 ' calls on it.
                 Dim installerServiceMock = New Mock(Of IPackageInstallerService)(MockBehavior.Loose)
-                installerServiceMock.SetupGet(Function(i) i.IsEnabled).Returns(True)
+                installerServiceMock.Setup(Function(i) i.IsEnabled(It.IsAny(Of ProjectId))).Returns(True)
                 installerServiceMock.SetupGet(Function(i) i.PackageSources).Returns(NugetPackageSources)
                 installerServiceMock.Setup(Function(s) s.TryInstallPackage(It.IsAny(Of Workspace), It.IsAny(Of DocumentId), It.IsAny(Of String), "NuGetPackage", It.IsAny(Of String), It.IsAny(Of Boolean), It.IsAny(Of CancellationToken))).
                                      Returns(True)
@@ -71,7 +71,7 @@ End Class", fixProviderData:=New ProviderData(installerServiceMock.Object, packa
                 ' Make a loose mock for the installer service.  We don't care what this test
                 ' calls on it.
                 Dim installerServiceMock = New Mock(Of IPackageInstallerService)(MockBehavior.Loose)
-                installerServiceMock.SetupGet(Function(i) i.IsEnabled).Returns(True)
+                installerServiceMock.Setup(Function(i) i.IsEnabled(It.IsAny(Of ProjectId))).Returns(True)
                 installerServiceMock.SetupGet(Function(i) i.PackageSources).Returns(NugetPackageSources)
                 installerServiceMock.Setup(Function(s) s.TryInstallPackage(It.IsAny(Of Workspace), It.IsAny(Of DocumentId), It.IsAny(Of String), "NuGetPackage", It.IsAny(Of String), It.IsAny(Of Boolean), It.IsAny(Of CancellationToken))).
                                      Returns(True)
@@ -98,7 +98,7 @@ End Class", fixProviderData:=New ProviderData(installerServiceMock.Object, packa
                 ' Make a loose mock for the installer service.  We don't care what this test
                 ' calls on it.
                 Dim installerServiceMock = New Mock(Of IPackageInstallerService)(MockBehavior.Loose)
-                installerServiceMock.SetupGet(Function(i) i.IsEnabled).Returns(True)
+                installerServiceMock.Setup(Function(i) i.IsEnabled(It.IsAny(Of ProjectId))).Returns(True)
                 installerServiceMock.SetupGet(Function(i) i.PackageSources).Returns(NugetPackageSources)
                 installerServiceMock.Setup(Function(s) s.TryInstallPackage(It.IsAny(Of Workspace), It.IsAny(Of DocumentId), It.IsAny(Of String), "NuGetPackage", It.IsAny(Of String), It.IsAny(Of Boolean), It.IsAny(Of CancellationToken))).
                                      Returns(False)
@@ -123,7 +123,7 @@ End Class", fixProviderData:=New ProviderData(installerServiceMock.Object, packa
                 ' Make a loose mock for the installer service.  We don't care what this test
                 ' calls on it.
                 Dim installerServiceMock = New Mock(Of IPackageInstallerService)(MockBehavior.Loose)
-                installerServiceMock.SetupGet(Function(i) i.IsEnabled).Returns(True)
+                installerServiceMock.Setup(Function(i) i.IsEnabled(It.IsAny(Of ProjectId))).Returns(True)
                 installerServiceMock.SetupGet(Function(i) i.PackageSources).Returns(NugetPackageSources)
                 installerServiceMock.Setup(Function(s) s.IsInstalled(It.IsAny(Of Workspace)(), It.IsAny(Of ProjectId)(), "NuGetPackage")).
                     Returns(True)
@@ -145,7 +145,7 @@ New TestParameters(fixProviderData:=New ProviderData(installerServiceMock.Object
                 ' Make a loose mock for the installer service.  We don't care what this test
                 ' calls on it.
                 Dim installerServiceMock = New Mock(Of IPackageInstallerService)(MockBehavior.Loose)
-                installerServiceMock.SetupGet(Function(i) i.IsEnabled).Returns(True)
+                installerServiceMock.Setup(Function(i) i.IsEnabled(It.IsAny(Of ProjectId))).Returns(True)
                 installerServiceMock.SetupGet(Function(i) i.PackageSources).Returns(NugetPackageSources)
                 installerServiceMock.Setup(Function(s) s.GetInstalledVersions("NuGetPackage")).
                     Returns(ImmutableArray.Create("1.0", "2.0"))
@@ -185,7 +185,7 @@ parameters:=New TestParameters(fixProviderData:=data))
             <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddImport)>
             Public Async Function TestInstallGetsCalledNoVersion() As Task
                 Dim installerServiceMock = New Mock(Of IPackageInstallerService)(MockBehavior.Loose)
-                installerServiceMock.SetupGet(Function(i) i.IsEnabled).Returns(True)
+                installerServiceMock.Setup(Function(i) i.IsEnabled(It.IsAny(Of ProjectId))).Returns(True)
                 installerServiceMock.SetupGet(Function(i) i.PackageSources).Returns(NugetPackageSources)
                 installerServiceMock.Setup(Function(s) s.TryInstallPackage(It.IsAny(Of Workspace), It.IsAny(Of DocumentId), It.IsAny(Of String), "NuGetPackage", Nothing, It.IsAny(Of Boolean), It.IsAny(Of CancellationToken))).
                                      Returns(True)
@@ -211,7 +211,7 @@ End Class", fixProviderData:=New ProviderData(installerServiceMock.Object, packa
             <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddImport)>
             Public Async Function TestInstallGetsCalledWithVersion() As Task
                 Dim installerServiceMock = New Mock(Of IPackageInstallerService)(MockBehavior.Loose)
-                installerServiceMock.SetupGet(Function(i) i.IsEnabled).Returns(True)
+                installerServiceMock.Setup(Function(i) i.IsEnabled(It.IsAny(Of ProjectId))).Returns(True)
                 installerServiceMock.SetupGet(Function(i) i.PackageSources).Returns(NugetPackageSources)
                 installerServiceMock.Setup(Function(s) s.GetInstalledVersions("NuGetPackage")).
                     Returns(ImmutableArray.Create("1.0"))

src/Features/Core/Portable/AddImport/SymbolReferenceFinder_PackageAssemblySearch.cs

@@ -84,7 +84,7 @@ private partial class SymbolReferenceFinder
                 if (symbolSearchService != null &&
                     installerService != null &&
                     searchNugetPackages && 
-                    installerService.IsEnabled)
+                    installerService.IsEnabled(_document.Project.Id))
                 {
                     foreach (var packageSource in installerService.PackageSources)
                     {

src/Features/Core/Portable/AddPackage/AbstractAddPackageCodeFixProvider.cs

@@ -51,7 +51,7 @@ internal abstract partial class AbstractAddPackageCodeFixProvider : CodeFixProvi
             if (symbolSearchService != null &&
                 installerService != null &&
                 searchNugetPackages &&
-                installerService.IsEnabled)
+                installerService.IsEnabled(document.Project.Id))
             {
                 foreach (var packageSource in installerService.PackageSources)
                 {

src/VisualStudio/Core/Def/Packaging/PackageInstallerService.ProjectState.cs

+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System.Collections.Generic;
+
+namespace Microsoft.VisualStudio.LanguageServices.Packaging
+{
+    internal partial class PackageInstallerService
+    {
+        private struct ProjectState
+        {
+            public readonly bool IsEnabled;
+            public readonly Dictionary<string, string> InstalledPackageToVersion;
+
+            public ProjectState(bool isEnabled, Dictionary<string, string> installedPackageToVersion)
+            {
+                IsEnabled = isEnabled;
+                InstalledPackageToVersion = installedPackageToVersion;
+            }
+        }
+    }
+}

src/VisualStudio/Core/Def/Packaging/PackageInstallerServiceFactory.cs

@@ -56,8 +56,8 @@ internal partial class PackageInstallerService : AbstractDelayStartedService, IP
         private bool _solutionChanged;
         private HashSet<ProjectId> _changedProjects = new HashSet<ProjectId>();
 
-        private readonly ConcurrentDictionary<ProjectId, Dictionary<string, string>> _projectToInstalledPackageAndVersion =
-            new ConcurrentDictionary<ProjectId, Dictionary<string, string>>();
+        private readonly ConcurrentDictionary<ProjectId, ProjectState> _projectToInstalledPackageAndVersion =
+            new ConcurrentDictionary<ProjectId, ProjectState>();
 
         [ImportingConstructor]
         public PackageInstallerService(
@@ -77,7 +77,23 @@ internal partial class PackageInstallerService : AbstractDelayStartedService, IP
 
         public event EventHandler PackageSourcesChanged;
 
-        public bool IsEnabled => _packageServices != null;
+        private bool IsEnabled => _packageServices != null;
+
+        bool IPackageInstallerService.IsEnabled(ProjectId projectId)
+        {
+            if (_packageServices == null)
+            {
+                return false;
+            }
+
+            if (_projectToInstalledPackageAndVersion.TryGetValue(projectId, out var state))
+            {
+                return state.IsEnabled;
+            }
+
+            // If we haven't scanned the project yet, assume that we're available for it.
+            return true;
+        }
 
         protected override void EnableService()
         {
@@ -270,10 +286,25 @@ private string GetInstalledVersion(string packageName, EnvDTE.Project dteProject
                 var metadata = installedPackages.FirstOrDefault(m => m.Id == packageName);
                 return metadata?.VersionString;
             }
+            catch (ArgumentException e) when (IsKnownNugetIssue(e))
+            {
+                // Nuget may throw an ArgumentException when there is something about the project 
+                // they do not like/support.
+            }
             catch (Exception e) when (FatalError.ReportWithoutCrash(e))
             {
-                return null;
             }
+
+            return null;
+        }
+
+        private bool IsKnownNugetIssue(ArgumentException exception)
+        {
+            // See https://github.com/NuGet/Home/issues/4706
+            // Nuget throws on legal projects.  We do not want to report this exception
+            // as it is known (and NFWs are expensive), but we do want to report if we 
+            // run into anything else.
+            return exception.Message.Contains("is not a valid version string");
         }
 
         private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs e)
@@ -388,8 +419,9 @@ private ProjectId DequeueNextProject(Solution solution)
         private void ProcessProjectChange(Solution solution, ProjectId projectId)
         {
             this.AssertIsForeground();
+
             // Remove anything we have associated with this project.
-            _projectToInstalledPackageAndVersion.TryRemove(projectId, out var installedPackages);
+            _projectToInstalledPackageAndVersion.TryRemove(projectId, out var projectState);
 
             var project = solution.GetProject(projectId);
             if (project == null)
@@ -416,26 +448,35 @@ private void ProcessProjectChange(Solution solution, ProjectId projectId)
                 return;
             }
 
-            installedPackages = new Dictionary<string, string>();
+            var installedPackages = new Dictionary<string, string>();
+            var isEnabled = false;
 
             // Calling into NuGet.  Assume they may fail for any reason.
             try
             {
                 var installedPackageMetadata = _packageServices.GetInstalledPackages(dteProject);
                 installedPackages.AddRange(installedPackageMetadata.Select(m => new KeyValuePair<string, string>(m.Id, m.VersionString)));
+                isEnabled = true;
+            }
+            catch (ArgumentException e) when (IsKnownNugetIssue(e))
+            {
+                // Nuget may throw an ArgumentException when there is something about the project 
+                // they do not like/support.
             }
             catch (Exception e) when (FatalError.ReportWithoutCrash(e))
             {
             }
 
-            _projectToInstalledPackageAndVersion.AddOrUpdate(projectId, installedPackages, (_1, _2) => installedPackages);
+            var state = new ProjectState(isEnabled, installedPackages);
+            _projectToInstalledPackageAndVersion.AddOrUpdate(
+                projectId, state, (_1, _2) => state);
         }
 
         public bool IsInstalled(Workspace workspace, ProjectId projectId, string packageName)
         {
             ThisCanBeCalledOnAnyThread();
             return _projectToInstalledPackageAndVersion.TryGetValue(projectId, out var installedPackages) &&
-                installedPackages.ContainsKey(packageName);
+                installedPackages.InstalledPackageToVersion.ContainsKey(packageName);
         }
 
         public ImmutableArray<string> GetInstalledVersions(string packageName)
@@ -443,10 +484,10 @@ public ImmutableArray<string> GetInstalledVersions(string packageName)
             ThisCanBeCalledOnAnyThread();
 
             var installedVersions = new HashSet<string>();
-            foreach (var installedPackages in _projectToInstalledPackageAndVersion.Values)
+            foreach (var state in _projectToInstalledPackageAndVersion.Values)
             {
                 string version = null;
-                if (installedPackages?.TryGetValue(packageName, out version) == true && version != null)
+                if (state.InstalledPackageToVersion.TryGetValue(packageName, out version) && version != null)
                 {
                     installedVersions.Add(version);
                 }
@@ -494,16 +535,13 @@ public IEnumerable<Project> GetProjectsWithInstalledPackage(Solution solution, s
 
             foreach (var kvp in this._projectToInstalledPackageAndVersion)
             {
-                var installedPackageAndVersion = kvp.Value;
-                if (installedPackageAndVersion != null)
+                var state = kvp.Value;
+                if (state.InstalledPackageToVersion.TryGetValue(packageName, out var installedVersion) && installedVersion == version)
                 {
-                    if (installedPackageAndVersion.TryGetValue(packageName, out var installedVersion) && installedVersion == version)
+                    var project = solution.GetProject(kvp.Key);
+                    if (project != null)
                     {
-                        var project = solution.GetProject(kvp.Key);
-                        if (project != null)
-                        {
-                            result.Add(project);
-                        }
+                        result.Add(project);
                     }
                 }
             }
@@ -631,4 +669,4 @@ public void UninstallPackage(EnvDTE.Project project, string packageId, bool remo
                 => _packageUninstaller.UninstallPackage(project, packageId, removeDependencies);
         }
     }
-}
+}
\ No newline at end of file

src/VisualStudio/Core/Def/ServicesVisualStudio.csproj

@@ -174,6 +174,7 @@
     <Compile Include="Implementation\Workspace\VisualStudioTaskSchedulerFactory.cs" />
     <Compile Include="Packaging\Interop\SVsRemoteControlService.cs" />
     <Compile Include="Packaging\IPackageServicesProxy.cs" />
+    <Compile Include="Packaging\PackageInstallerService.ProjectState.cs" />
     <Compile Include="Storage\VisualStudioPersistentStorageLocationService.cs" />
     <Compile Include="SymbolSearch\AbstractDelayStartedService.cs" />
     <Compile Include="Packaging\PackageInstallerServiceFactory.cs" />

src/Workspaces/Core/Portable/Packaging/IPackageInstallerService.cs

@@ -11,7 +11,7 @@ namespace Microsoft.CodeAnalysis.Packaging
 {
     internal interface IPackageInstallerService : IWorkspaceService
     {
-        bool IsEnabled { get; }
+        bool IsEnabled(ProjectId projectId);
 
         bool IsInstalled(Workspace workspace, ProjectId projectId, string packageName);