C#: Address review comments.

Author: calumgrant

csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs

@@ -1,9 +1,13 @@
-using System;
+using Semmle.Util;
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using Semmle.Extraction.CSharp.Standalone;
 using System.Threading.Tasks;
+using System.Collections.Concurrent;
+using System.Text;
+using System.Security.Cryptography;
 
 namespace Semmle.BuildAnalyser
 {
@@ -42,20 +46,18 @@ interface IBuildAnalysis
     /// <summary>
     /// Main implementation of the build analysis.
     /// </summary>
-    class BuildAnalysis : IBuildAnalysis
+    class BuildAnalysis : IBuildAnalysis, IDisposable
     {
         private readonly AssemblyCache assemblyCache;
         private readonly NugetPackages nuget;
         private readonly IProgressMonitor progressMonitor;
-        private HashSet<string> usedReferences = new HashSet<string>();
-        private readonly HashSet<string> usedSources = new HashSet<string>();
-        private readonly HashSet<string> missingSources = new HashSet<string>();
-        private readonly Dictionary<string, string> unresolvedReferences = new Dictionary<string, string>();
+        private readonly IDictionary<string, bool> usedReferences = new ConcurrentDictionary<string, bool>();
+        private readonly IDictionary<string, bool> sources = new ConcurrentDictionary<string, bool>();
+        private readonly IDictionary<string, string> unresolvedReferences = new ConcurrentDictionary<string, string>();
         private readonly DirectoryInfo sourceDir;
         private int failedProjects, succeededProjects;
         private readonly string[] allSources;
         private int conflictedReferences = 0;
-        private readonly object mutex = new object();
 
         /// <summary>
         /// Performs a C# build analysis.
@@ -77,7 +79,7 @@ public BuildAnalysis(Options options, IProgressMonitor progress)
                 ToArray();
 
             var dllDirNames = options.DllDirs.Select(Path.GetFullPath).ToList();
-            PackageDirectory = TemporaryDirectory.CreateTempDirectory(sourceDir.FullName);
+            PackageDirectory = new TemporaryDirectory(ComputeTempDirectory(sourceDir.FullName));
 
             if (options.UseNuGet)
             {
@@ -97,23 +99,22 @@ public BuildAnalysis(Options options, IProgressMonitor progress)
             {
                 dllDirNames.Add(Runtime.Runtimes.First());
             }
-            
+
+            // These files can sometimes prevent `dotnet restore` from working correctly.
+            using (new FileRenamer(sourceDir.GetFiles("global.json", SearchOption.AllDirectories)))
+            using (new FileRenamer(sourceDir.GetFiles("Directory.Build.props", SearchOption.AllDirectories)))
             {
-                // These files can sometimes prevent `dotnet restore` from working correctly.
-                using (new FileRenamer(sourceDir.GetFiles("global.json", SearchOption.AllDirectories)))
-                using (new FileRenamer(sourceDir.GetFiles("Directory.Build.props", SearchOption.AllDirectories)))
-                {
-                    var solutions = options.SolutionFile != null ?
-                            new[] { options.SolutionFile } :
-                            sourceDir.GetFiles("*.sln", SearchOption.AllDirectories).Select(d => d.FullName);
+                var solutions = options.SolutionFile != null ?
+                        new[] { options.SolutionFile } :
+                        sourceDir.GetFiles("*.sln", SearchOption.AllDirectories).Select(d => d.FullName);
 
-                    RestoreSolutions(solutions);
-                    dllDirNames.Add(PackageDirectory.DirInfo.FullName);
-                    assemblyCache = new BuildAnalyser.AssemblyCache(dllDirNames, progress);
-                    AnalyseSolutions(solutions);
+                RestoreSolutions(solutions);
+                dllDirNames.Add(PackageDirectory.DirInfo.FullName);
+                assemblyCache = new BuildAnalyser.AssemblyCache(dllDirNames, progress);
+                AnalyseSolutions(solutions);
 
-                    usedReferences = new HashSet<string>(assemblyCache.AllAssemblies.Select(a => a.Filename));
-                }
+                foreach (var filename in assemblyCache.AllAssemblies.Select(a => a.Filename))
+                    UseReference(filename);
             }
 
             ResolveConflicts();
@@ -124,7 +125,7 @@ public BuildAnalysis(Options options, IProgressMonitor progress)
             }
 
             // Output the findings
-            foreach (var r in usedReferences)
+            foreach (var r in usedReferences.Keys)
             {
                 progressMonitor.ResolvedReference(r);
             }
@@ -146,6 +147,25 @@ public BuildAnalysis(Options options, IProgressMonitor progress)
                 DateTime.Now - startTime);
         }
 
+        /// <summary>
+        /// Computes a unique temp directory for the packages associated
+        /// with this source tree. Use a SHA1 of the directory name.
+        /// </summary>
+        /// <param name="srcDir"></param>
+        /// <returns>The full path of the temp directory.</returns>
+        private static string ComputeTempDirectory(string srcDir)
+        {
+            var bytes = Encoding.Unicode.GetBytes(srcDir);
+
+            using var sha1 = new SHA1CryptoServiceProvider();
+            var sha = sha1.ComputeHash(bytes);
+            var sb = new StringBuilder();
+            foreach (var b in sha.Take(8))
+                sb.AppendFormat("{0:x2}", b);
+
+            return Path.Combine(Path.GetTempPath(), "GitHub", "packages", sb.ToString());
+        }
+
         /// <summary>
         /// Resolves conflicts between all of the resolved references.
         /// If the same assembly name is duplicated with different versions,
@@ -154,7 +174,7 @@ public BuildAnalysis(Options options, IProgressMonitor progress)
         void ResolveConflicts()
         {
             var sortedReferences = usedReferences.
-                Select(r => assemblyCache.GetAssemblyInfo(r)).
+                Select(r => assemblyCache.GetAssemblyInfo(r.Key)).
                 OrderBy(r => r.Version).
                 ToArray();
 
@@ -165,7 +185,9 @@ void ResolveConflicts()
                 finalAssemblyList[r.Name] = r;
 
             // Update the used references list
-            usedReferences = new HashSet<string>(finalAssemblyList.Select(r => r.Value.Filename));
+            usedReferences.Clear();
+            foreach (var r in finalAssemblyList.Select(r => r.Value.Filename))
+                UseReference(r);
 
             // Report the results
             foreach (var r in sortedReferences)
@@ -194,8 +216,7 @@ void ReadNugetFiles()
         /// <param name="reference">The filename of the reference.</param>
         void UseReference(string reference)
         {
-            lock (mutex)
-                usedReferences.Add(reference);
+            usedReferences[reference] = true;
         }
 
         /// <summary>
@@ -204,27 +225,18 @@ void UseReference(string reference)
         /// <param name="sourceFile">The source file.</param>
         void UseSource(FileInfo sourceFile)
         {
-            if (sourceFile.Exists)
-            {
-                lock(mutex)
-                    usedSources.Add(sourceFile.FullName);
-            }
-            else
-            {
-                lock(mutex)
-                    missingSources.Add(sourceFile.FullName);
-            }
+            sources[sourceFile.FullName] = sourceFile.Exists;
         }
 
         /// <summary>
         /// The list of resolved reference files.
         /// </summary>
-        public IEnumerable<string> ReferenceFiles => this.usedReferences;
+        public IEnumerable<string> ReferenceFiles => this.usedReferences.Keys;
 
         /// <summary>
         /// The list of source files used in projects.
         /// </summary>
-        public IEnumerable<string> ProjectSourceFiles => usedSources;
+        public IEnumerable<string> ProjectSourceFiles => sources.Where(s => s.Value).Select(s => s.Key);
 
         /// <summary>
         /// All of the source files in the source directory.
@@ -240,7 +252,7 @@ void UseSource(FileInfo sourceFile)
         /// List of source files which were mentioned in project files but
         /// do not exist on the file system.
         /// </summary>
-        public IEnumerable<string> MissingSourceFiles => missingSources;
+        public IEnumerable<string> MissingSourceFiles => sources.Where(s => !s.Value).Select(s => s.Key);
 
         /// <summary>
         /// Record that a particular reference couldn't be resolved.
@@ -250,8 +262,7 @@ void UseSource(FileInfo sourceFile)
         /// <param name="projectFile">The project file making the reference.</param>
         void UnresolvedReference(string id, string projectFile)
         {
-            lock(mutex)
-                unresolvedReferences[id] = projectFile;
+            unresolvedReferences[id] = projectFile;
         }
 
         readonly TemporaryDirectory PackageDirectory;
@@ -276,7 +287,7 @@ void AnalyseProject(FileInfo project)
 
             try
             {
-                IProjectFile csProj = new CsProjFile(project);
+                var csProj = new CsProjFile(project);
 
                 foreach (var @ref in csProj.References)
                 {
@@ -309,14 +320,6 @@ void AnalyseProject(FileInfo project)
 
         }
 
-        /// <summary>
-        /// Delete packages directory.
-        /// </summary>
-        public void Cleanup()
-        {
-            PackageDirectory?.Cleanup();
-        }
-
         void Restore(string projectOrSolution)
         {
             int exit = DotNet.RestoreToDirectory(projectOrSolution, PackageDirectory.DirInfo.FullName);
@@ -345,13 +348,18 @@ public void AnalyseSolutions(IEnumerable<string> solutions)
                 {
                     var sln = new SolutionFile(solutionFile);
                     progressMonitor.AnalysingSolution(solutionFile);
-                    AnalyseProjectFiles(sln.Projects.Select(p => new FileInfo(p)).Where(p => p.Exists).ToArray());
+                    AnalyseProjectFiles(sln.Projects.Select(p => new FileInfo(p)).Where(p => p.Exists));
                 }
                 catch (Microsoft.Build.Exceptions.InvalidProjectFileException ex)
                 {
                     progressMonitor.FailedProjectFile(solutionFile, ex.BaseMessage);
                 }
             });
         }
+
+        public void Dispose()
+        {
+            PackageDirectory?.Dispose();
+        }
     }
 }

csharp/extractor/Semmle.Extraction.CSharp.Standalone/CsProjFile.cs

@@ -5,21 +5,14 @@
 
 namespace Semmle.BuildAnalyser
 {
-    interface IProjectFile
-    {
-        IEnumerable<string> References { get; }
-
-        IEnumerable<string> Sources { get; }
-    }
-
     /// <summary>
     /// Represents a .csproj file and reads information from it.
     /// </summary>
-    class CsProjFile : IProjectFile
+    class CsProjFile
     {
-        public string Filename { get; }
+        private string Filename { get; }
 
-        public string Directory => Path.GetDirectoryName(Filename);
+        private string Directory => Path.GetDirectoryName(Filename);
 
         /// <summary>
         /// Reads the .csproj file.
@@ -52,7 +45,7 @@ public CsProjFile(FileInfo filename)
         /// and there seems to be no way to make it succeed. Fails on Linux.
         /// </summary>
         /// <param name="filename">The file to read.</param>
-        public void ReadMsBuildProject(FileInfo filename)
+        private void ReadMsBuildProject(FileInfo filename)
         {
             var msbuildProject = new Microsoft.Build.Execution.ProjectInstance(filename.FullName);
 
@@ -75,7 +68,7 @@ public void ReadMsBuildProject(FileInfo filename)
         /// fallback if ReadMsBuildProject() fails.
         /// </summary>
         /// <param name="filename">The .csproj file.</param>
-        public void ReadProjectFileAsXml(FileInfo filename)
+        private void ReadProjectFileAsXml(FileInfo filename)
         {
             var projFile = new XmlDocument();
             var mgr = new XmlNamespaceManager(projFile.NameTable);
@@ -88,15 +81,15 @@ public void ReadProjectFileAsXml(FileInfo filename)
 
             bool netCoreProjectFile = root.GetAttribute("Sdk") == "Microsoft.NET.Sdk";
 
-            if(netCoreProjectFile)
+            if (netCoreProjectFile)
             {
-                var relativeCsIncludes2 =
+                var relativeCsIncludes =
                     root.SelectNodes("/Project/ItemGroup/Compile/@Include", mgr).
                     NodeList().
                     Select(node => node.Value).
                     ToArray();
 
-                var explicitCsFiles = relativeCsIncludes2.
+                var explicitCsFiles = relativeCsIncludes.
                     Select(cs => Path.DirectorySeparatorChar == '/' ? cs.Replace("\\", "/") : cs).
                     Select(f => Path.GetFullPath(Path.Combine(projDir.FullName, f)));
 
@@ -105,25 +98,27 @@ public void ReadProjectFileAsXml(FileInfo filename)
                 csFiles = explicitCsFiles.Concat(additionalCsFiles).ToArray();
 
                 references = new string[0];
-                return;
             }
+            else
+            {
 
-            references =
-                root.SelectNodes("/msbuild:Project/msbuild:ItemGroup/msbuild:Reference/@Include", mgr).
-                NodeList().
-                Select(node => node.Value).
-                ToArray();
+                references =
+                    root.SelectNodes("/msbuild:Project/msbuild:ItemGroup/msbuild:Reference/@Include", mgr).
+                    NodeList().
+                    Select(node => node.Value).
+                    ToArray();
 
-            var relativeCsIncludes =
-                root.SelectNodes("/msbuild:Project/msbuild:ItemGroup/msbuild:Compile/@Include", mgr).
-                NodeList().
-                Select(node => node.Value).
-                ToArray();
+                var relativeCsIncludes =
+                    root.SelectNodes("/msbuild:Project/msbuild:ItemGroup/msbuild:Compile/@Include", mgr).
+                    NodeList().
+                    Select(node => node.Value).
+                    ToArray();
 
-            csFiles = relativeCsIncludes.
-                Select(cs => Path.DirectorySeparatorChar == '/' ? cs.Replace("\\", "/") : cs).
-                Select(f => Path.GetFullPath(Path.Combine(projDir.FullName, f))).
-                ToArray();
+                csFiles = relativeCsIncludes.
+                    Select(cs => Path.DirectorySeparatorChar == '/' ? cs.Replace("\\", "/") : cs).
+                    Select(f => Path.GetFullPath(Path.Combine(projDir.FullName, f))).
+                    ToArray();
+            }
         }
 
         string[] references;

csharp/extractor/Semmle.Extraction.CSharp.Standalone/DotNet.cs

@@ -13,36 +13,9 @@ static class DotNet
     {
         public static int RestoreToDirectory(string projectOrSolutionFile, string packageDirectory)
         {
-            using var proc = Process.Start("dotnet", $"restore --no-dependencies \"{projectOrSolutionFile}\" --packages \"{packageDirectory}\"");
+            using var proc = Process.Start("dotnet", $"restore --no-dependencies \"{projectOrSolutionFile}\" --packages \"{packageDirectory}\" /p:DisableImplicitNuGetFallbackFolder=true");
             proc.WaitForExit();
             return proc.ExitCode;
         }
     }
-
-    /// <summary>
-    /// Utility to temporarily rename a set of files.
-    /// </summary>
-    sealed class FileRenamer : IDisposable
-    {
-        readonly string[] files;
-        const string suffix = ".codeqlhidden";
-
-        public FileRenamer(IEnumerable<FileInfo> oldFiles)
-        {
-            files = oldFiles.Select(f => f.FullName).ToArray();
-
-            foreach(var file in files)
-            {
-                File.Move(file, file + suffix);
-            }
-        }
-
-        public void Dispose()
-        {
-            foreach (var file in files)
-            {
-                File.Move(file + suffix, file);
-            }
-        }
-    }
 }