Merge pull request #14020 from tamasvajk/fix/dependency-fetching-1

C#: Fix lazy evaluation of not yet downloaded packages

Author: michaelnebel

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

@@ -60,7 +60,7 @@ public DependencyManager(string srcDir, IDependencyOptions options, ILogger logg
 
             packageDirectory = new TemporaryDirectory(ComputeTempDirectory(sourceDir.FullName));
 
-            this.fileContent = new FileContent(packageDirectory, progressMonitor, () => GetFiles("*.*"));
+            this.fileContent = new FileContent(progressMonitor, () => GetFiles("*.*"));
             this.allSources = GetFiles("*.cs").ToList();
             var allProjects = GetFiles("*.csproj");
             var solutions = options.SolutionFile is not null
@@ -388,7 +388,11 @@ private void DownloadMissingPackages()
                 nugetConfig = nugetConfigs.FirstOrDefault();
             }
 
-            foreach (var package in fileContent.NotYetDownloadedPackages)
+            var alreadyDownloadedPackages = Directory.GetDirectories(packageDirectory.DirInfo.FullName)
+                .Select(d => Path.GetFileName(d)
+                .ToLowerInvariant());
+            var notYetDownloadedPackages = fileContent.AllPackages.Except(alreadyDownloadedPackages);
+            foreach (var package in notYetDownloadedPackages)
             {
                 progressMonitor.NugetInstall(package);
                 using var tempDir = new TemporaryDirectory(ComputeTempDirectory(package));

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/FileContent.cs

@@ -12,23 +12,22 @@ namespace Semmle.Extraction.CSharp.DependencyFetching
     // This class is used to read a set of files and decide different properties about the
     // content (by reading the content of the files only once).
     // The implementation is lazy, so the properties are only calculated when
-    // the first property is accessed. 
+    // the first property is accessed.
     // </summary>
     internal partial class FileContent
     {
         private readonly ProgressMonitor progressMonitor;
         private readonly IUnsafeFileReader unsafeFileReader;
         private readonly Func<IEnumerable<string>> getFiles;
-        private readonly Func<HashSet<string>> getAlreadyDownloadedPackages;
-        private readonly HashSet<string> notYetDownloadedPackages = new HashSet<string>();
+        private readonly HashSet<string> allPackages = new HashSet<string>();
         private readonly Initializer initialize;
 
-        public HashSet<string> NotYetDownloadedPackages
+        public HashSet<string> AllPackages
         {
             get
             {
                 initialize.Run();
-                return notYetDownloadedPackages;
+                return allPackages;
             }
         }
 
@@ -50,23 +49,18 @@ public bool UseAspNetDlls
             }
         }
 
-        internal FileContent(Func<HashSet<string>> getAlreadyDownloadedPackages,
-            ProgressMonitor progressMonitor,
+        internal FileContent(ProgressMonitor progressMonitor,
             Func<IEnumerable<string>> getFiles,
             IUnsafeFileReader unsafeFileReader)
         {
-            this.getAlreadyDownloadedPackages = getAlreadyDownloadedPackages;
             this.progressMonitor = progressMonitor;
             this.getFiles = getFiles;
             this.unsafeFileReader = unsafeFileReader;
             this.initialize = new Initializer(DoInitialize);
         }
 
 
-        public FileContent(TemporaryDirectory packageDirectory, ProgressMonitor progressMonitor, Func<IEnumerable<string>> getFiles) : this(() => Directory.GetDirectories(packageDirectory.DirInfo.FullName)
-                .Select(d => Path.GetFileName(d)
-                .ToLowerInvariant())
-                .ToHashSet(), progressMonitor, getFiles, new UnsafeFileReader())
+        public FileContent(ProgressMonitor progressMonitor, Func<IEnumerable<string>> getFiles) : this(progressMonitor, getFiles, new UnsafeFileReader())
         { }
 
         private static string GetGroup(ReadOnlySpan<char> input, ValueMatch valueMatch, string groupPrefix)
@@ -101,22 +95,21 @@ private static bool IsGroupMatch(ReadOnlySpan<char> line, Regex regex, string gr
 
         private void DoInitialize()
         {
-            var alreadyDownloadedPackages = getAlreadyDownloadedPackages();
             foreach (var file in getFiles())
             {
                 try
                 {
                     foreach (ReadOnlySpan<char> line in unsafeFileReader.ReadLines(file))
                     {
 
-                        // Find the not yet downloaded packages.
+                        // Find all the packages.
                         foreach (var valueMatch in PackageReference().EnumerateMatches(line))
                         {
                             // We can't get the group from the ValueMatch, so doing it manually:
                             var packageName = GetGroup(line, valueMatch, "Include");
-                            if (!string.IsNullOrEmpty(packageName) && !alreadyDownloadedPackages.Contains(packageName))
+                            if (!string.IsNullOrEmpty(packageName))
                             {
-                                notYetDownloadedPackages.Add(packageName);
+                                allPackages.Add(packageName);
                             }
                         }
 

csharp/extractor/Semmle.Extraction.Tests/FileContent.cs

@@ -1,5 +1,6 @@
 using Xunit;
 using System.Collections.Generic;
+using System.Linq;
 using Semmle.Util.Logging;
 using Semmle.Extraction.CSharp.DependencyFetching;
 
@@ -33,8 +34,7 @@ public IEnumerable<string> ReadLines(string file)
 
     internal class TestFileContent : FileContent
     {
-        public TestFileContent(List<string> lines) : base(() => new HashSet<string>(),
-            new ProgressMonitor(new LoggerStub()),
+        public TestFileContent(List<string> lines) : base(new ProgressMonitor(new LoggerStub()),
             () => new List<string>() { "test1.cs" },
             new UnsafeFileReaderStub(lines))
         { }
@@ -57,15 +57,15 @@ public void TestFileContent1()
             var fileContent = new TestFileContent(lines);
 
             // Execute
-            var notYetDownloadedPackages = fileContent.NotYetDownloadedPackages;
+            var allPackages = fileContent.AllPackages;
             var useAspNetDlls = fileContent.UseAspNetDlls;
 
             // Verify
             Assert.False(useAspNetDlls);
-            Assert.Equal(3, notYetDownloadedPackages.Count);
-            Assert.Contains("DotNetAnalyzers.DocumentationAnalyzers".ToLowerInvariant(), notYetDownloadedPackages);
-            Assert.Contains("Microsoft.CodeAnalysis.NetAnalyzers".ToLowerInvariant(), notYetDownloadedPackages);
-            Assert.Contains("StyleCop.Analyzers".ToLowerInvariant(), notYetDownloadedPackages);
+            Assert.Equal(3, allPackages.Count);
+            Assert.Contains("DotNetAnalyzers.DocumentationAnalyzers".ToLowerInvariant(), allPackages);
+            Assert.Contains("Microsoft.CodeAnalysis.NetAnalyzers".ToLowerInvariant(), allPackages);
+            Assert.Contains("StyleCop.Analyzers".ToLowerInvariant(), allPackages);
         }
 
         [Fact]
@@ -83,13 +83,13 @@ public void TestFileContent2()
 
             // Execute
             var useAspNetDlls = fileContent.UseAspNetDlls;
-            var notYetDownloadedPackages = fileContent.NotYetDownloadedPackages;
+            var allPackages = fileContent.AllPackages;
 
             // Verify
             Assert.True(useAspNetDlls);
-            Assert.Equal(2, notYetDownloadedPackages.Count);
-            Assert.Contains("Microsoft.CodeAnalysis.NetAnalyzers".ToLowerInvariant(), notYetDownloadedPackages);
-            Assert.Contains("StyleCop.Analyzers".ToLowerInvariant(), notYetDownloadedPackages);
+            Assert.Equal(2, allPackages.Count);
+            Assert.Contains("Microsoft.CodeAnalysis.NetAnalyzers".ToLowerInvariant(), allPackages);
+            Assert.Contains("StyleCop.Analyzers".ToLowerInvariant(), allPackages);
         }
     }
 }