Merge pull request github#16836 from michaelnebel/csharp/bestlocation

michaelnebel · web-flow · commit e1f65d1f8b65 · 2024-06-26T12:46:50.000+02:00
C#: Be more consistent when picking between locations.
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/CachedSymbol.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/CachedSymbol.cs
@@ -82,12 +82,12 @@ protected void ExtractCompilerGenerated(TextWriter trapFile)
         /// The location which is stored in the database and is used when highlighting source code.
         /// It's generally short, e.g. a method name.
         /// </summary>
-        public override Microsoft.CodeAnalysis.Location? ReportingLocation => Symbol.Locations.FirstOrDefault();
+        public override Microsoft.CodeAnalysis.Location? ReportingLocation => Symbol.Locations.BestOrDefault();
 
         /// <summary>
         /// The full text span of the entity, e.g. for binding comments.
         /// </summary>
-        public virtual Microsoft.CodeAnalysis.Location? FullLocation => Symbol.Locations.FirstOrDefault();
+        public virtual Microsoft.CodeAnalysis.Location? FullLocation => Symbol.Locations.BestOrDefault();
 
         public virtual IEnumerable<Extraction.Entities.Location> Locations
         {
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs
@@ -1,4 +1,3 @@
-using System;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Conversion.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Conversion.cs
@@ -24,7 +24,7 @@ public override Microsoft.CodeAnalysis.Location? ReportingLocation
                     .OfType<ConversionOperatorDeclarationSyntax>()
                     .Select(s => s.FixedLocation())
                     .Concat(Symbol.Locations)
-                    .FirstOrDefault();
+                    .BestOrDefault();
             }
         }
 
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs
@@ -93,7 +93,7 @@ public override Microsoft.CodeAnalysis.Location FullLocation
                     .OfType<IndexerDeclarationSyntax>()
                     .Select(s => s.GetLocation())
                     .Concat(Symbol.Locations)
-                    .First();
+                    .Best();
             }
         }
 
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs
@@ -111,7 +111,7 @@ public override Microsoft.CodeAnalysis.Location FullLocation
                     .OfType<PropertyDeclarationSyntax>()
                     .Select(s => s.GetLocation())
                     .Concat(Symbol.Locations)
-                    .First();
+                    .Best();
             }
         }
 
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/DynamicType.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/DynamicType.cs
@@ -11,7 +11,7 @@ private DynamicType(Context cx, IDynamicTypeSymbol init)
 
         public static DynamicType Create(Context cx, IDynamicTypeSymbol type) => DynamicTypeFactory.Instance.CreateEntityFromSymbol(cx, type);
 
-        public override Microsoft.CodeAnalysis.Location? ReportingLocation => Context.Compilation.ObjectType.Locations.FirstOrDefault();
+        public override Microsoft.CodeAnalysis.Location? ReportingLocation => Context.Compilation.ObjectType.Locations.BestOrDefault();
 
         public override void Populate(TextWriter trapFile)
         {
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs
@@ -124,7 +124,7 @@ public override IEnumerable<Extraction.Entities.Location> Locations
                 );
         }
 
-        public override Microsoft.CodeAnalysis.Location? ReportingLocation => GetLocations(Symbol).FirstOrDefault();
+        public override Microsoft.CodeAnalysis.Location? ReportingLocation => GetLocations(Symbol).BestOrDefault();
 
         private bool IsAnonymousType() => Symbol.IsAnonymousType || Symbol.Name.Contains("__AnonymousType");
 
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Extractor/TracingAnalyser.cs b/csharp/extractor/Semmle.Extraction.CSharp/Extractor/TracingAnalyser.cs
@@ -130,7 +130,7 @@ private static string GetOutputName(CSharpCompilation compilation,
                     return Path.ChangeExtension(entryPointFile, ".exe");
                 }
 
-                var entryPointFilename = entry.Locations.First().SourceTree!.FilePath;
+                var entryPointFilename = entry.Locations.Best().SourceTree!.FilePath;
                 return Path.ChangeExtension(entryPointFilename, ".exe");
             }
 
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Populators/Locations.cs b/csharp/extractor/Semmle.Extraction.CSharp/Populators/Locations.cs
@@ -107,7 +107,7 @@ public static Location GetSymbolLocation(this ISymbol symbol)
         {
             return symbol.DeclaringSyntaxReferences.Any() ?
                 symbol.DeclaringSyntaxReferences.First().GetSyntax().FixedLocation() :
-                symbol.Locations.First();
+                symbol.Locations.Best();
         }
     }
 }
diff --git a/csharp/extractor/Semmle.Extraction/Context.cs b/csharp/extractor/Semmle.Extraction/Context.cs
@@ -379,7 +379,7 @@ private void ExtractionError(string message, ISymbol? optionalSymbol, Entity opt
         {
             if (!(optionalSymbol is null))
             {
-                ExtractionError(message, optionalSymbol.ToDisplayString(), CreateLocation(optionalSymbol.Locations.FirstOrDefault()));
+                ExtractionError(message, optionalSymbol.ToDisplayString(), CreateLocation(optionalSymbol.Locations.BestOrDefault()));
             }
             else if (!(optionalEntity is null))
             {
diff --git a/csharp/extractor/Semmle.Extraction/InternalError.cs b/csharp/extractor/Semmle.Extraction/InternalError.cs
@@ -13,7 +13,7 @@ public InternalError(ISymbol symbol, string msg)
         {
             Text = msg;
             EntityText = symbol.ToString() ?? "";
-            Location = symbol.Locations.FirstOrDefault();
+            Location = symbol.Locations.BestOrDefault();
         }
 
         public InternalError(SyntaxNode node, string msg)
diff --git a/csharp/extractor/Semmle.Extraction/LocationExtensions.cs b/csharp/extractor/Semmle.Extraction/LocationExtensions.cs
@@ -1,4 +1,7 @@
-﻿using Microsoft.CodeAnalysis;
+﻿using System;
+using System.Collections.Generic;
+using System.Linq;
+using Microsoft.CodeAnalysis;
 
 namespace Semmle.Extraction
 {
@@ -36,5 +39,46 @@ public static bool Before(this Location before, Location after)
             var endsBefore = before.SourceSpan.End <= after.SourceSpan.Start;
             return sameFile && endsBefore;
         }
+
+        private static int GetLocationKindPriority(Location location) =>
+            location.IsInSource
+                ? 2
+                : location.IsInMetadata
+                    ? 1
+                    : 0;
+
+        /// <summary>
+        /// Returns true if l1 is better than l2.
+        /// Source locations are considered better than non source locations.
+        /// </summary>
+        private static bool BetterThan(Location l1, Location l2)
+        {
+            if (GetLocationKindPriority(l1) > GetLocationKindPriority(l2))
+            {
+                return true;
+            }
+
+            // For source locations we compare the filepath and span.
+            if (l1.IsInSource && l2.IsInSource)
+            {
+                var l1s = l1.SourceTree.FilePath + l1.SourceSpan;
+                var l2s = l2.SourceTree.FilePath + l2.SourceSpan;
+                return l1s.CompareTo(l2s) < 0;
+            }
+
+            return false;
+        }
+
+        /// <summary>
+        /// Returns the best location from the given list of locations.
+        /// Source locations are considered better than non-source locations.
+        /// In case of a (source location) tie, the location with the
+        /// lexicographically smaller filepath and span is considered better.
+        /// </summary>
+        public static Location? BestOrDefault(this IEnumerable<Location> locations) =>
+            locations.Any() ? locations.Aggregate((best, loc) => BetterThan(loc, best) ? loc : best) : null;
+
+        public static Location Best(this IEnumerable<Location> locations) =>
+            locations.BestOrDefault() ?? throw new ArgumentException("No location found.");
     }
 }
diff --git a/csharp/extractor/Semmle.Extraction/Message.cs b/csharp/extractor/Semmle.Extraction/Message.cs
@@ -27,7 +27,7 @@ public Message(string text, string? entityText, Entities.Location? location, str
 
         public static Message Create(Context cx, string text, ISymbol symbol, string? stackTrace = null, Severity severity = Severity.Error)
         {
-            return new Message(text, symbol.ToString(), cx.CreateLocation(symbol.Locations.FirstOrDefault()), stackTrace, severity);
+            return new Message(text, symbol.ToString(), cx.CreateLocation(symbol.Locations.BestOrDefault()), stackTrace, severity);
         }
 
         public static Message Create(Context cx, string text, SyntaxNode node, string? stackTrace = null, Severity severity = Severity.Error)
diff --git a/csharp/ql/test/library-tests/exprorstmtparent/Callable.expected b/csharp/ql/test/library-tests/exprorstmtparent/Callable.expected
@@ -29,7 +29,7 @@
 | A.cs:28:7:28:8 | C3 | B.cs:25:7:25:8 | {...} |
 | A.cs:30:21:30:23 | get_P3 | A.cs:30:28:30:37 | throw ... |
 | A.cs:34:15:34:16 | C4 | A.cs:34:15:34:16 | {...} |
-| A.cs:34:15:34:16 | C4 | C.cs:1:15:1:16 | {...} |
+| A.cs:34:15:34:16 | C4 | B.cs:30:15:30:16 | {...} |
 | A.cs:36:9:36:10 | M1 | A.cs:36:14:36:28 | {...} |
 | A.cs:36:9:36:10 | M1 | B.cs:32:17:32:17 | 0 |
 | A.cs:37:9:37:10 | M2 | A.cs:37:14:37:28 | {...} |

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,3 @@`
`1`		`-using System;`
`2`	`1`	`using System.Collections.Generic;`
`3`	`2`	`using System.Diagnostics.CodeAnalysis;`
`4`	`3`	`using System.IO;`
Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ public override Microsoft.CodeAnalysis.Location? ReportingLocation`
`24`	`24`	`.OfType<ConversionOperatorDeclarationSyntax>()`
`25`	`25`	`.Select(s => s.FixedLocation())`
`26`	`26`	`.Concat(Symbol.Locations)`
`27`		`- .FirstOrDefault();`
	`27`	`+ .BestOrDefault();`
`28`	`28`	`}`
`29`	`29`	`}`
`30`	`30`
Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,7 @@ public override Microsoft.CodeAnalysis.Location FullLocation`
`93`	`93`	`.OfType<IndexerDeclarationSyntax>()`
`94`	`94`	`.Select(s => s.GetLocation())`
`95`	`95`	`.Concat(Symbol.Locations)`
`96`		`- .First();`
	`96`	`+ .Best();`
`97`	`97`	`}`
`98`	`98`	`}`
`99`	`99`
Original file line number	Diff line number	Diff line change
`@@ -111,7 +111,7 @@ public override Microsoft.CodeAnalysis.Location FullLocation`
`111`	`111`	`.OfType<PropertyDeclarationSyntax>()`
`112`	`112`	`.Select(s => s.GetLocation())`
`113`	`113`	`.Concat(Symbol.Locations)`
`114`		`- .First();`
	`114`	`+ .Best();`
`115`	`115`	`}`
`116`	`116`	`}`
`117`	`117`
Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ private DynamicType(Context cx, IDynamicTypeSymbol init)`
`11`	`11`
`12`	`12`	`public static DynamicType Create(Context cx, IDynamicTypeSymbol type) => DynamicTypeFactory.Instance.CreateEntityFromSymbol(cx, type);`
`13`	`13`
`14`		`- public override Microsoft.CodeAnalysis.Location? ReportingLocation => Context.Compilation.ObjectType.Locations.FirstOrDefault();`
	`14`	`+ public override Microsoft.CodeAnalysis.Location? ReportingLocation => Context.Compilation.ObjectType.Locations.BestOrDefault();`
`15`	`15`
`16`	`16`	`public override void Populate(TextWriter trapFile)`
`17`	`17`	`{`
Original file line number	Diff line number	Diff line change
`@@ -124,7 +124,7 @@ public override IEnumerable<Extraction.Entities.Location> Locations`
`124`	`124`	`);`
`125`	`125`	`}`
`126`	`126`
`127`		`- public override Microsoft.CodeAnalysis.Location? ReportingLocation => GetLocations(Symbol).FirstOrDefault();`
	`127`	`+ public override Microsoft.CodeAnalysis.Location? ReportingLocation => GetLocations(Symbol).BestOrDefault();`
`128`	`128`
`129`	`129`	`private bool IsAnonymousType() => Symbol.IsAnonymousType \|\| Symbol.Name.Contains("__AnonymousType");`
`130`	`130`
Original file line number	Diff line number	Diff line change
`@@ -130,7 +130,7 @@ private static string GetOutputName(CSharpCompilation compilation,`
`130`	`130`	`return Path.ChangeExtension(entryPointFile, ".exe");`
`131`	`131`	`}`
`132`	`132`
`133`		`- var entryPointFilename = entry.Locations.First().SourceTree!.FilePath;`
	`133`	`+ var entryPointFilename = entry.Locations.Best().SourceTree!.FilePath;`
`134`	`134`	`return Path.ChangeExtension(entryPointFilename, ".exe");`
`135`	`135`	`}`
`136`	`136`
Original file line number	Diff line number	Diff line change
`@@ -107,7 +107,7 @@ public static Location GetSymbolLocation(this ISymbol symbol)`
`107`	`107`	`{`
`108`	`108`	`return symbol.DeclaringSyntaxReferences.Any() ?`
`109`	`109`	`symbol.DeclaringSyntaxReferences.First().GetSyntax().FixedLocation() :`
`110`		`- symbol.Locations.First();`
	`110`	`+ symbol.Locations.Best();`
`111`	`111`	`}`
`112`	`112`	`}`
`113`	`113`	`}`
Original file line number	Diff line number	Diff line change
`@@ -379,7 +379,7 @@ private void ExtractionError(string message, ISymbol? optionalSymbol, Entity opt`
`379`	`379`	`{`
`380`	`380`	`if (!(optionalSymbol is null))`
`381`	`381`	`{`
`382`		`- ExtractionError(message, optionalSymbol.ToDisplayString(), CreateLocation(optionalSymbol.Locations.FirstOrDefault()));`
	`382`	`+ ExtractionError(message, optionalSymbol.ToDisplayString(), CreateLocation(optionalSymbol.Locations.BestOrDefault()));`
`383`	`383`	`}`
`384`	`384`	`else if (!(optionalEntity is null))`
`385`	`385`	`{`