C#: Address review comments.

Author: michaelnebel

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

@@ -3,6 +3,7 @@
 using System.IO;
 using System.Linq;
 using Newtonsoft.Json.Linq;
+using Semmle.Util;
 
 namespace Semmle.Extraction.CSharp.DependencyFetching
 {
@@ -29,21 +30,15 @@ internal Assets(ProgressMonitor progressMonitor)
         /// <summary>
         /// Class needed for deserializing parts of an assets file.
         /// It holds information about a reference.
+        ///
+        /// Type carries the type of the reference.
+        /// We are only interested in package references.
+        ///
+        /// Compile holds information about the files needed for compilation.
+        /// However, if it is a .NET framework reference we assume that all files in the
+        /// package are needed for compilation.
         /// </summary>
-        private class ReferenceInfo
-        {
-            /// <summary>
-            /// This carries the type of the reference.
-            /// We are only interested in package references.
-            /// </summary>
-            public string Type { get; set; } = "";
-
-            /// <summary>
-            /// If not a .NET framework reference we assume that only the files mentioned
-            /// in the compile section are needed for compilation.
-            /// </summary>
-            public Dictionary<string, object> Compile { get; set; } = new();
-        }
+        private record class ReferenceInfo(string? Type, Dictionary<string, object>? Compile);
 
         /// <summary>
         /// Add the package dependencies from the assets file to dependencies.
@@ -74,7 +69,7 @@ private class ReferenceInfo
         /// }
         /// 
         /// Returns dependencies
-        ///   Required = {
+        ///   RequiredPaths = {
         ///     "castle.core/4.4.1/lib/netstandard1.5/Castle.Core.dll",
         ///     "json.net/1.0.33/lib/netstandard2.0/Json.Net.dll"
         ///   }
@@ -83,7 +78,7 @@ private class ReferenceInfo
         ///     "json.net"
         ///   }
         /// </summary>
-        private Dependencies AddPackageDependencies(JObject json, Dependencies dependencies)
+        private DependencyContainer AddPackageDependencies(JObject json, DependencyContainer dependencies)
         {
             // If there are more than one framework we need to pick just one.
             // To ensure stability we pick one based on the lexicographic order of
@@ -103,31 +98,37 @@ private Dependencies AddPackageDependencies(JObject json, Dependencies dependenc
 
             // Find all the compile dependencies for each reference and
             // create the relative path to the dependency.
-            return references
-                .Aggregate(dependencies, (deps, r) =>
+            references
+                .ForEach(r =>
                 {
                     var info = r.Value;
                     var name = r.Key.ToLowerInvariant();
                     if (info.Type != "package")
                     {
-                        return deps;
+                        return;
                     }
 
                     // If this is a .NET framework reference then include everything.
-                    return netFrameworks.Any(framework => name.StartsWith(framework))
-                        ? deps.Add(name, "")
-                        : info
-                            .Compile
-                            .Aggregate(deps, (d, p) => d.Add(name, p.Key));
+                    if (netFrameworks.Any(framework => name.StartsWith(framework)))
+                    {
+                        dependencies.Add(name);
+                    }
+                    else
+                    {
+                        info.Compile?
+                            .ForEach(r => dependencies.Add(name, r.Key));
+                    }
                 });
+
+            return dependencies;
         }
 
         /// <summary>
         /// Parse `json` as project.assets.json content and add relative paths to the dependencies
         /// (together with used package information) required for compilation.
         /// </summary>
         /// <returns>True if parsing succeeds, otherwise false.</returns>
-        public bool TryParse(string json, Dependencies dependencies)
+        public bool TryParse(string json, DependencyContainer dependencies)
         {
             try
             {
@@ -142,15 +143,16 @@ public bool TryParse(string json, Dependencies dependencies)
             }
         }
 
-        public static Dependencies GetCompilationDependencies(ProgressMonitor progressMonitor, IEnumerable<string> assets)
+        public static DependencyContainer GetCompilationDependencies(ProgressMonitor progressMonitor, IEnumerable<string> assets)
         {
             var parser = new Assets(progressMonitor);
-            return assets.Aggregate(new Dependencies(), (dependencies, asset) =>
+            var dependencies = new DependencyContainer();
+            assets.ForEach(asset =>
             {
                 var json = File.ReadAllText(asset);
                 parser.TryParse(json, dependencies);
-                return dependencies;
             });
+            return dependencies;
         }
     }
 

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Dependencies.cs renamed to csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyContainer.cs

@@ -7,9 +7,9 @@ namespace Semmle.Extraction.CSharp.DependencyFetching
     /// <summary>
     /// Container class for dependencies found in the assets file.
     /// </summary>
-    internal class Dependencies
+    internal class DependencyContainer
     {
-        private readonly List<string> required = new();
+        private readonly List<string> requiredPaths = new();
         private readonly HashSet<string> usedPackages = new();
 
         /// <summary>
@@ -33,9 +33,9 @@ private static string GetPackageName(string package) =>
                 .First();
 
         /// <summary>
-        /// Dependencies required for Compilation.
+        /// Paths to dependencies required for compilation.
         /// </summary>
-        public IEnumerable<string> Required => required;
+        public IEnumerable<string> RequiredPaths => requiredPaths;
 
         /// <summary>
         /// Packages that are used as a part of the required dependencies.
@@ -45,29 +45,25 @@ private static string GetPackageName(string package) =>
         /// <summary>
         /// Add a dependency inside a package.
         /// </summary>
-        public Dependencies Add(string package, string dependency)
+        public void Add(string package, string dependency)
         {
             var p = package.Replace('/', Path.DirectorySeparatorChar);
             var d = dependency.Replace('/', Path.DirectorySeparatorChar);
 
             var path = Path.Combine(p, ParseFilePath(d));
-            required.Add(path);
+            requiredPaths.Add(path);
             usedPackages.Add(GetPackageName(p));
-
-            return this;
         }
 
         /// <summary>
         /// Add a dependency to an entire package
         /// </summary>
-        public Dependencies Add(string package)
+        public void Add(string package)
         {
             var p = package.Replace('/', Path.DirectorySeparatorChar);
 
-            required.Add(p);
+            requiredPaths.Add(p);
             usedPackages.Add(GetPackageName(p));
-
-            return this;
         }
     }
 }

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

@@ -104,7 +104,7 @@ public DependencyManager(string srcDir, IDependencyOptions options, ILogger logg
                 var dependencies = Assets.GetCompilationDependencies(progressMonitor, assets1.Union(assets2));
 
                 var paths = dependencies
-                    .Required
+                    .RequiredPaths
                     .Select(d => Path.Combine(packageDirectory.DirInfo.FullName, d))
                     .ToList();
                 dllPaths.AddRange(paths);
@@ -303,7 +303,7 @@ private IEnumerable<string> GetAllPackageDirectories()
                 .Select(d => d.FullName);
         }
 
-        private void LogAllUnusedPackages(Dependencies dependencies) =>
+        private void LogAllUnusedPackages(DependencyContainer dependencies) =>
             GetAllPackageDirectories()
                 .Where(package => !dependencies.UsedPackages.Contains(package))
                 .ForEach(package => progressMonitor.LogInfo($"Unused package: {package}"));

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

@@ -14,17 +14,17 @@ public void TestAssets1()
             // Setup
             var assets = new Assets(new ProgressMonitor(new LoggerStub()));
             var json = assetsJson1;
-            var dependencies = new Dependencies();
+            var dependencies = new DependencyContainer();
 
             // Execute
             var success = assets.TryParse(json, dependencies);
 
             // Verify
             Assert.True(success);
-            Assert.Equal(5, dependencies.Required.Count());
+            Assert.Equal(5, dependencies.RequiredPaths.Count());
             Assert.Equal(4, dependencies.UsedPackages.Count());
 
-            var normalizedPaths = dependencies.Required.Select(FixExpectedPathOnWindows);
+            var normalizedPaths = dependencies.RequiredPaths.Select(FixExpectedPathOnWindows);
             // Required references
             Assert.Contains("castle.core/4.4.1/lib/netstandard1.5/Castle.Core.dll", normalizedPaths);
             Assert.Contains("castle.core/4.4.1/lib/netstandard1.5/Castle.Core2.dll", normalizedPaths);
@@ -44,16 +44,16 @@ public void TestAssets2()
             // Setup
             var assets = new Assets(new ProgressMonitor(new LoggerStub()));
             var json = assetsJson2;
-            var dependencies = new Dependencies();
+            var dependencies = new DependencyContainer();
 
             // Execute
             var success = assets.TryParse(json, dependencies);
 
             // Verify
             Assert.True(success);
-            Assert.Equal(2, dependencies.Required.Count());
+            Assert.Equal(2, dependencies.RequiredPaths.Count());
 
-            var normalizedPaths = dependencies.Required.Select(FixExpectedPathOnWindows);
+            var normalizedPaths = dependencies.RequiredPaths.Select(FixExpectedPathOnWindows);
             // Required references
             Assert.Contains("microsoft.netframework.referenceassemblies/1.0.3", normalizedPaths);
             Assert.Contains("microsoft.netframework.referenceassemblies.net48/1.0.3", normalizedPaths);
@@ -68,14 +68,14 @@ public void TestAssets3()
             // Setup
             var assets = new Assets(new ProgressMonitor(new LoggerStub()));
             var json = "garbage data";
-            var dependencies = new Dependencies();
+            var dependencies = new DependencyContainer();
 
             // Execute
             var success = assets.TryParse(json, dependencies);
 
             // Verify
             Assert.False(success);
-            Assert.Empty(dependencies.Required);
+            Assert.Empty(dependencies.RequiredPaths);
         }
 
         private readonly string assetsJson1 = """