Merge pull request #16938 from tamasvajk/feature/extract-files-multiple

tamasvajk · web-flow · commit 42be9e98c8f7 · 2024-08-07T11:23:48.000+02:00
C#: Do not skip extraction of already seen source files
diff --git a/csharp/downgrades/15b989afd2bfc4743536fdb0958c1d8177a32600/old.dbscheme b/csharp/downgrades/15b989afd2bfc4743536fdb0958c1d8177a32600/old.dbscheme
diff --git a/csharp/downgrades/15b989afd2bfc4743536fdb0958c1d8177a32600/semmlecode.csharp.dbscheme b/csharp/downgrades/15b989afd2bfc4743536fdb0958c1d8177a32600/semmlecode.csharp.dbscheme
diff --git a/csharp/downgrades/15b989afd2bfc4743536fdb0958c1d8177a32600/upgrade.properties b/csharp/downgrades/15b989afd2bfc4743536fdb0958c1d8177a32600/upgrade.properties
@@ -0,0 +1,2 @@
+description: Add unique constraint on preprocessor directive and compilation pairs
+compatibility: backwards
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/PreprocessorDirectives/ElifDirective.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/PreprocessorDirectives/ElifDirective.cs
@@ -20,6 +20,7 @@ private ElifDirective(Context cx, ElifDirectiveTriviaSyntax trivia, IfDirective
         public override void WriteId(EscapingTextWriter trapFile)
         {
             trapFile.WriteSubId(Context.CreateLocation(ReportingLocation));
+            trapFile.WriteSubId(start);
             trapFile.Write(Symbol.IsActive);
             trapFile.Write(',');
             trapFile.Write(Symbol.BranchTaken);
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/PreprocessorDirectives/ElseDirective.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/PreprocessorDirectives/ElseDirective.cs
@@ -18,6 +18,7 @@ private ElseDirective(Context cx, ElseDirectiveTriviaSyntax trivia, IfDirective
         public override void WriteId(EscapingTextWriter trapFile)
         {
             trapFile.WriteSubId(Context.CreateLocation(ReportingLocation));
+            trapFile.WriteSubId(start);
             trapFile.Write(Symbol.IsActive);
             trapFile.Write(',');
             trapFile.Write(Symbol.BranchTaken);
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/PreprocessorDirectives/EndIfDirective.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/PreprocessorDirectives/EndIfDirective.cs
@@ -13,6 +13,14 @@ private EndIfDirective(Context cx, EndIfDirectiveTriviaSyntax trivia, IfDirectiv
             this.start = start;
         }
 
+        public override void WriteId(EscapingTextWriter trapFile)
+        {
+            trapFile.WriteSubId(Context.CreateLocation(ReportingLocation));
+            trapFile.WriteSubId(start);
+            trapFile.Write(Symbol.IsActive);
+            trapFile.Write(";trivia");
+        }
+
         protected override void PopulatePreprocessor(TextWriter trapFile)
         {
             trapFile.directive_endifs(this, start);
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Extractor/Analyser.cs b/csharp/extractor/Semmle.Extraction.CSharp/Extractor/Analyser.cs
@@ -190,37 +190,29 @@ private void DoExtractTree(SyntaxTree tree)
                 var transformedSourcePath = PathTransformer.Transform(sourcePath);
 
                 var trapPath = transformedSourcePath.GetTrapPath(Logger, options.TrapCompression);
-                var upToDate = false;
-
-                // compilation.Clone() is used to allow symbols to be garbage collected.
                 using var trapWriter = transformedSourcePath.CreateTrapWriter(Logger, options.TrapCompression, discardDuplicates: false);
 
-                upToDate = FileIsUpToDate(sourcePath, trapWriter.TrapFile);
-
                 var currentTaskId = IncrementTaskCount();
                 ReportProgressTaskStarted(currentTaskId, sourcePath);
 
-                if (!upToDate)
+                var cx = new Context(ExtractionContext, compilation, trapWriter, new SourceScope(tree), addAssemblyTrapPrefix);
+                // Ensure that the file itself is populated in case the source file is totally empty
+                var root = tree.GetRoot();
+                Entities.File.Create(cx, root.SyntaxTree.FilePath);
+
+                var csNode = (CSharpSyntaxNode)root;
+                var directiveVisitor = new DirectiveVisitor(cx);
+                csNode.Accept(directiveVisitor);
+                foreach (var branch in directiveVisitor.BranchesTaken)
                 {
-                    var cx = new Context(ExtractionContext, compilation, trapWriter, new SourceScope(tree), addAssemblyTrapPrefix);
-                    // Ensure that the file itself is populated in case the source file is totally empty
-                    var root = tree.GetRoot();
-                    Entities.File.Create(cx, root.SyntaxTree.FilePath);
-
-                    var csNode = (CSharpSyntaxNode)root;
-                    var directiveVisitor = new DirectiveVisitor(cx);
-                    csNode.Accept(directiveVisitor);
-                    foreach (var branch in directiveVisitor.BranchesTaken)
-                    {
-                        cx.TrapStackSuffix.Add(branch);
-                    }
-                    csNode.Accept(new CompilationUnitVisitor(cx));
-                    cx.PopulateAll();
-                    CommentPopulator.ExtractCommentBlocks(cx, cx.CommentGenerator);
-                    cx.PopulateAll();
+                    cx.TrapStackSuffix.Add(branch);
                 }
+                csNode.Accept(new CompilationUnitVisitor(cx));
+                cx.PopulateAll();
+                CommentPopulator.ExtractCommentBlocks(cx, cx.CommentGenerator);
+                cx.PopulateAll();
 
-                ReportProgressTaskDone(currentTaskId, sourcePath, trapPath, stopwatch.Elapsed, upToDate ? AnalysisAction.UpToDate : AnalysisAction.Extracted);
+                ReportProgressTaskDone(currentTaskId, sourcePath, trapPath, stopwatch.Elapsed, AnalysisAction.Extracted);
             }
             catch (Exception ex)  // lgtm[cs/catch-of-all-exceptions]
             {
@@ -268,12 +260,6 @@ public void AnalyseCompilation()
             extractionTasks.Add(() => DoAnalyseCompilation());
         }
 
-        private static bool FileIsUpToDate(string src, string dest)
-        {
-            return File.Exists(dest) &&
-                File.GetLastWriteTime(dest) >= File.GetLastWriteTime(src);
-        }
-
         private static void AnalyseNamespace(Context cx, INamespaceSymbol ns)
         {
             foreach (var memberNamespace in ns.GetNamespaceMembers())
diff --git a/csharp/extractor/Semmle.Extraction/TrapWriter.cs b/csharp/extractor/Semmle.Extraction/TrapWriter.cs
@@ -194,8 +194,11 @@ public void Dispose()
                     var hash = FileUtils.ComputeFileHash(tmpFile);
                     if (existingHash != hash)
                     {
-                        var root = TrapFile.Substring(0, TrapFile.Length - 8); // Remove trailing ".trap.gz"
-                        if (TryMove(tmpFile, $"{root}-{hash}.trap{TrapExtension(trapCompression)}"))
+                        var extension = TrapExtension(trapCompression);
+                        var root = TrapFile[..^extension.Length]; // Remove trailing ".trap", ".trap.gz", or ".trap.br"
+                        var newTrapName = $"{root}-{hash}{extension}";
+                        logger.LogInfo($"Identical trap file for {TrapFile} already exists, renaming to {newTrapName}");
+                        if (TryMove(tmpFile, $"{newTrapName}"))
                             return;
                     }
                     logger.LogInfo($"Identical trap file for {TrapFile} already exists");
@@ -217,16 +220,16 @@ private static string TrapExtension(CompressionMode compression)
         {
             switch (compression)
             {
-                case CompressionMode.None: return "";
-                case CompressionMode.Gzip: return ".gz";
-                case CompressionMode.Brotli: return ".br";
+                case CompressionMode.None: return ".trap";
+                case CompressionMode.Gzip: return ".trap.gz";
+                case CompressionMode.Brotli: return ".trap.br";
                 default: throw new ArgumentOutOfRangeException(nameof(compression), compression, "Unsupported compression type");
             }
         }
 
         public static string TrapPath(ILogger logger, string? folder, PathTransformer.ITransformedPath path, TrapWriter.CompressionMode trapCompression)
         {
-            var filename = $"{path.Value}.trap{TrapExtension(trapCompression)}";
+            var filename = $"{path.Value}{TrapExtension(trapCompression)}";
             if (string.IsNullOrEmpty(folder))
                 folder = Directory.GetCurrentDirectory();
 
diff --git a/csharp/ql/integration-tests/all-platforms/conditional_compilation/Methods.expected b/csharp/ql/integration-tests/all-platforms/conditional_compilation/Methods.expected
@@ -0,0 +1,4 @@
+| Program.cs:3:24:3:27 | Main | 2 |
+| Program.cs:10:17:10:33 | GreetConditional1 | 2 |
+| Program.cs:19:17:19:21 | Greet | 2 |
+| Program.cs:25:17:25:33 | GreetConditional2 | 2 |
diff --git a/csharp/ql/integration-tests/all-platforms/conditional_compilation/Methods.ql b/csharp/ql/integration-tests/all-platforms/conditional_compilation/Methods.ql
@@ -0,0 +1,5 @@
+import csharp
+
+from Method m
+where m.fromSource()
+select m, count(m.getBody())
diff --git a/csharp/ql/integration-tests/all-platforms/conditional_compilation/Program.cs b/csharp/ql/integration-tests/all-platforms/conditional_compilation/Program.cs
@@ -0,0 +1,40 @@
+﻿public class Test
+{
+    public static void Main()
+    {
+        Greet();
+        GreetConditional1();
+        GreetConditional2();
+    }
+
+    static void GreetConditional1()
+    {
+#if A
+    Console.WriteLine("Hello, A!");
+#elif C
+    Console.WriteLine("Hello, C!");
+#endif
+    }
+
+    static void Greet()
+    {
+        Console.WriteLine("Hello, World!");
+    }
+
+#if A
+    static void GreetConditional2()
+    {
+        Console.WriteLine("Hello, A!");
+    }
+#elif B
+    static void GreetConditional2()
+    {
+        Console.WriteLine("Hello, B!");
+    }
+#else
+    static void GreetConditional2()
+    {
+        Console.WriteLine("Hello, Others!");
+    }
+#endif
+}
diff --git a/csharp/ql/integration-tests/all-platforms/conditional_compilation/global.json b/csharp/ql/integration-tests/all-platforms/conditional_compilation/global.json
@@ -0,0 +1,5 @@
+{
+    "sdk": {
+        "version": "8.0.101"
+    }
+}
diff --git a/csharp/ql/integration-tests/all-platforms/conditional_compilation/test.csproj b/csharp/ql/integration-tests/all-platforms/conditional_compilation/test.csproj
@@ -0,0 +1,15 @@
+﻿<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <TargetFramework>net8.0</TargetFramework>
+    <ImplicitUsings>enable</ImplicitUsings>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <Compile Remove="**/*.cs" />
+    <Compile Include="Program.cs" />
+  </ItemGroup>
+
+</Project>
diff --git a/csharp/ql/integration-tests/all-platforms/conditional_compilation/test.py b/csharp/ql/integration-tests/all-platforms/conditional_compilation/test.py
@@ -0,0 +1,6 @@
+from create_database_utils import *
+import os
+
+os.environ["CODEQL_EXTRACTOR_CSHARP_OPTION_TRAP_COMPRESSION"] = "none"
+
+run_codeql_database_create(['dotnet build /p:DefineConstants=A', 'dotnet build /p:DefineConstants=B'], lang="csharp")
diff --git a/csharp/ql/lib/change-notes/2024-07-10-conditional-compilation.md b/csharp/ql/lib/change-notes/2024-07-10-conditional-compilation.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The extractor has been changed to not skip source files that have already been seen. This has an impact on source files that are compiled multiple times in the build process. Source files with conditional compilation preprocessor directives (such as `#if`) are now extracted for each set of preprocessor symbols that are used during the build process.
diff --git a/csharp/ql/lib/semmlecode.csharp.dbscheme b/csharp/ql/lib/semmlecode.csharp.dbscheme
@@ -421,7 +421,7 @@ preprocessor_directive_location(
   int loc: @location ref);
 
 preprocessor_directive_compilation(
-  unique int id: @preprocessor_directive ref,
+  int id: @preprocessor_directive ref,
   int compilation: @compilation ref);
 
 preprocessor_directive_active(
diff --git a/csharp/ql/lib/upgrades/fd04e45710e1988076801608abffdfa013b680fc/old.dbscheme b/csharp/ql/lib/upgrades/fd04e45710e1988076801608abffdfa013b680fc/old.dbscheme
diff --git a/csharp/ql/lib/upgrades/fd04e45710e1988076801608abffdfa013b680fc/semmlecode.csharp.dbscheme b/csharp/ql/lib/upgrades/fd04e45710e1988076801608abffdfa013b680fc/semmlecode.csharp.dbscheme
diff --git a/csharp/ql/lib/upgrades/fd04e45710e1988076801608abffdfa013b680fc/upgrade.properties b/csharp/ql/lib/upgrades/fd04e45710e1988076801608abffdfa013b680fc/upgrade.properties

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+description: Add unique constraint on preprocessor directive and compilation pairs`
	`2`	`+compatibility: backwards`
Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,7 @@ private ElifDirective(Context cx, ElifDirectiveTriviaSyntax trivia, IfDirective`
`20`	`20`	`public override void WriteId(EscapingTextWriter trapFile)`
`21`	`21`	`{`
`22`	`22`	`trapFile.WriteSubId(Context.CreateLocation(ReportingLocation));`
	`23`	`+ trapFile.WriteSubId(start);`
`23`	`24`	`trapFile.Write(Symbol.IsActive);`
`24`	`25`	`trapFile.Write(',');`
`25`	`26`	`trapFile.Write(Symbol.BranchTaken);`
Original file line number	Diff line number	Diff line change
`@@ -18,6 +18,7 @@ private ElseDirective(Context cx, ElseDirectiveTriviaSyntax trivia, IfDirective`
`18`	`18`	`public override void WriteId(EscapingTextWriter trapFile)`
`19`	`19`	`{`
`20`	`20`	`trapFile.WriteSubId(Context.CreateLocation(ReportingLocation));`
	`21`	`+ trapFile.WriteSubId(start);`
`21`	`22`	`trapFile.Write(Symbol.IsActive);`
`22`	`23`	`trapFile.Write(',');`
`23`	`24`	`trapFile.Write(Symbol.BranchTaken);`
Original file line number	Diff line number	Diff line change
`@@ -13,6 +13,14 @@ private EndIfDirective(Context cx, EndIfDirectiveTriviaSyntax trivia, IfDirectiv`
`13`	`13`	`this.start = start;`
`14`	`14`	`}`
`15`	`15`
	`16`	`+ public override void WriteId(EscapingTextWriter trapFile)`
	`17`	`+ {`
	`18`	`+ trapFile.WriteSubId(Context.CreateLocation(ReportingLocation));`
	`19`	`+ trapFile.WriteSubId(start);`
	`20`	`+ trapFile.Write(Symbol.IsActive);`
	`21`	`+ trapFile.Write(";trivia");`
	`22`	`+ }`
	`23`	`+`
`16`	`24`	`protected override void PopulatePreprocessor(TextWriter trapFile)`
`17`	`25`	`{`
`18`	`26`	`trapFile.directive_endifs(this, start);`
Original file line number	Diff line number	Diff line change
`@@ -194,8 +194,11 @@ public void Dispose()`
`194`	`194`	`var hash = FileUtils.ComputeFileHash(tmpFile);`
`195`	`195`	`if (existingHash != hash)`
`196`	`196`	`{`
`197`		`- var root = TrapFile.Substring(0, TrapFile.Length - 8); // Remove trailing ".trap.gz"`
`198`		`- if (TryMove(tmpFile, $"{root}-{hash}.trap{TrapExtension(trapCompression)}"))`
	`197`	`+ var extension = TrapExtension(trapCompression);`
	`198`	`+ var root = TrapFile[..^extension.Length]; // Remove trailing ".trap", ".trap.gz", or ".trap.br"`
	`199`	`+ var newTrapName = $"{root}-{hash}{extension}";`
	`200`	`+ logger.LogInfo($"Identical trap file for {TrapFile} already exists, renaming to {newTrapName}");`
	`201`	`+ if (TryMove(tmpFile, $"{newTrapName}"))`
`199`	`202`	`return;`
`200`	`203`	`}`
`201`	`204`	`logger.LogInfo($"Identical trap file for {TrapFile} already exists");`
`@@ -217,16 +220,16 @@ private static string TrapExtension(CompressionMode compression)`
`217`	`220`	`{`
`218`	`221`	`switch (compression)`
`219`	`222`	`{`
`220`		`- case CompressionMode.None: return "";`
`221`		`- case CompressionMode.Gzip: return ".gz";`
`222`		`- case CompressionMode.Brotli: return ".br";`
	`223`	`+ case CompressionMode.None: return ".trap";`
	`224`	`+ case CompressionMode.Gzip: return ".trap.gz";`
	`225`	`+ case CompressionMode.Brotli: return ".trap.br";`
`223`	`226`	`default: throw new ArgumentOutOfRangeException(nameof(compression), compression, "Unsupported compression type");`
`224`	`227`	`}`
`225`	`228`	`}`
`226`	`229`
`227`	`230`	`public static string TrapPath(ILogger logger, string? folder, PathTransformer.ITransformedPath path, TrapWriter.CompressionMode trapCompression)`
`228`	`231`	`{`
`229`		`- var filename = $"{path.Value}.trap{TrapExtension(trapCompression)}";`
	`232`	`+ var filename = $"{path.Value}{TrapExtension(trapCompression)}";`
`230`	`233`	`if (string.IsNullOrEmpty(folder))`
`231`	`234`	`folder = Directory.GetCurrentDirectory();`
`232`	`235`