Merge pull request #20617 from michaelnebel/csharp/unboundlocations

michaelnebel · web-flow · commit ab9f78fee25a · 2025-10-10T13:47:57.000+02:00
C#: Reduce location TRAP creation for Fields, Parameters, Constructors, Destructors and Operators.
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs
@@ -29,7 +29,10 @@ public override void Populate(TextWriter trapFile)
             ContainingType!.PopulateGenerics();
 
             trapFile.constructors(this, Symbol.ContainingType.Name, ContainingType, (Constructor)OriginalDefinition);
-            WriteLocationToTrap(trapFile.constructor_location, this, Location);
+            if (Context.ExtractLocation(Symbol) && (!IsDefault || IsBestSourceLocation))
+            {
+                WriteLocationToTrap(trapFile.constructor_location, this, Location);
+            }
 
             if (MakeSynthetic)
             {
@@ -168,7 +171,15 @@ Symbol.ContainingType.TypeKind is TypeKind.Class or TypeKind.Struct &&
             Symbol.ContainingType.IsSourceDeclaration() &&
             !Symbol.ContainingType.IsAnonymousType;
 
-        private bool MakeSynthetic => IsPrimary || IsDefault;
+        /// <summary>
+        /// Returns true if we consider the reporting location of this constructor entity the best
+        /// location of the constructor.
+        /// For partial classes with default constructors, Roslyn consider each partial class declaration
+        /// as the possible location for the implicit default constructor.
+        /// </summary>
+        private bool IsBestSourceLocation => ReportingLocation is not null && Context.IsLocationInContext(ReportingLocation);
+
+        private bool MakeSynthetic => IsPrimary || (IsDefault && IsBestSourceLocation);
 
         [return: NotNullIfNotNull(nameof(constructor))]
         public static new Constructor? Create(Context cx, IMethodSymbol? constructor)
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Destructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Destructor.cs
@@ -15,7 +15,10 @@ public override void Populate(TextWriter trapFile)
             ContainingType!.PopulateGenerics();
 
             trapFile.destructors(this, $"~{Symbol.ContainingType.Name}", ContainingType, OriginalDefinition(Context, this, Symbol));
-            WriteLocationToTrap(trapFile.destructor_location, this, Location);
+            if (Context.ExtractLocation(Symbol))
+            {
+                WriteLocationToTrap(trapFile.destructor_location, this, Location);
+            }
         }
 
         private static new Destructor OriginalDefinition(Context cx, Destructor original, IMethodSymbol symbol)
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Field.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Field.cs
@@ -49,7 +49,10 @@ public override void Populate(TextWriter trapFile)
                 }
             }
 
-            WriteLocationsToTrap(trapFile.field_location, this, Locations);
+            if (Context.ExtractLocation(Symbol))
+            {
+                WriteLocationsToTrap(trapFile.field_location, this, Locations);
+            }
 
             if (!IsSourceDeclaration || !Symbol.FromSource())
                 return;
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Parameter.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Parameter.cs
@@ -115,9 +115,10 @@ public override void Populate(TextWriter trapFile)
             var type = Type.Create(Context, Symbol.Type);
             trapFile.@params(this, Name, type.TypeRef, Ordinal, ParamKind, Parent!, Original);
 
-            foreach (var l in Symbol.Locations)
+            if (Context.ExtractLocation(Symbol))
             {
-                WriteLocationToTrap(trapFile.param_location, this, Context.CreateLocation(l));
+                var locations = Context.GetLocations(Symbol);
+                WriteLocationsToTrap(trapFile.param_location, this, locations);
             }
 
             if (!Symbol.Locations.Any() &&
diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/UserOperator.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/UserOperator.cs
@@ -26,7 +26,10 @@ public override void Populate(TextWriter trapFile)
                 returnType.TypeRef,
                 (UserOperator)OriginalDefinition);
 
-            WriteLocationsToTrap(trapFile.operator_location, this, Locations);
+            if (Context.ExtractLocation(Symbol))
+            {
+                WriteLocationsToTrap(trapFile.operator_location, this, Locations);
+            }
 
             if (IsSourceDeclaration)
             {
diff --git a/csharp/ql/lib/change-notes/2025-10-10-entity-locations.md b/csharp/ql/lib/change-notes/2025-10-10-entity-locations.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The extraction of location information for parameters, fields, constructors, destructors and user operators has been optimized. Previously, location information was extracted multiple times for each bound generic. Now, only the location of the unbound generic declaration is extracted during the extraction phase, and the QL library explicitly reuses this location for all bound instances of the same generic.
diff --git a/csharp/ql/lib/semmle/code/csharp/Callable.qll b/csharp/ql/lib/semmle/code/csharp/Callable.qll
@@ -357,7 +357,7 @@ class Constructor extends Callable, Member, Attributable, @constructor {
 
   override Constructor getUnboundDeclaration() { constructors(this, _, _, result) }
 
-  override Location getALocation() { constructor_location(this, result) }
+  override Location getALocation() { constructor_location(this.getUnboundDeclaration(), result) }
 
   override predicate fromSource() { Member.super.fromSource() and not this.isCompilerGenerated() }
 
@@ -450,7 +450,7 @@ class Destructor extends Callable, Member, Attributable, @destructor {
 
   override Destructor getUnboundDeclaration() { destructors(this, _, _, result) }
 
-  override Location getALocation() { destructor_location(this, result) }
+  override Location getALocation() { destructor_location(this.getUnboundDeclaration(), result) }
 
   override string toString() { result = Callable.super.toString() }
 
@@ -484,7 +484,7 @@ class Operator extends Callable, Member, Attributable, Overridable, @operator {
 
   override Operator getUnboundDeclaration() { operators(this, _, _, _, _, result) }
 
-  override Location getALocation() { operator_location(this, result) }
+  override Location getALocation() { operator_location(this.getUnboundDeclaration(), result) }
 
   override string toString() { result = Callable.super.toString() }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/Variable.qll b/csharp/ql/lib/semmle/code/csharp/Variable.qll
@@ -213,7 +213,7 @@ class Parameter extends LocalScopeVariable, Attributable, TopLevelExprParent, @p
     params(this, _, getTypeRef(result), _, _, _, _)
   }
 
-  override Location getALocation() { param_location(this, result) }
+  override Location getALocation() { param_location(this.getUnboundDeclaration(), result) }
 
   override string toString() { result = this.getName() }
 
@@ -449,7 +449,7 @@ class Field extends Variable, AssignableMember, Attributable, TopLevelExprParent
     fields(this, _, _, _, getTypeRef(result), _)
   }
 
-  override Location getALocation() { field_location(this, result) }
+  override Location getALocation() { field_location(this.getUnboundDeclaration(), result) }
 
   override string toString() { result = Variable.super.toString() }
 
diff --git a/csharp/ql/test/library-tests/assignables/Assignables.expected b/csharp/ql/test/library-tests/assignables/Assignables.expected
@@ -43,12 +43,6 @@
 | Assignables.cs:92:23:92:23 | b |
 | Assignables.cs:92:33:92:33 | s |
 | Assignables.cs:95:40:95:44 | tuple |
-| Assignables.cs:97:24:97:24 | Item1 |
-| Assignables.cs:97:27:97:36 | Item2 |
-| Assignables.cs:101:6:101:8 | Item1 |
-| Assignables.cs:101:11:101:24 | Item2 |
-| Assignables.cs:101:12:101:15 | Item1 |
-| Assignables.cs:101:18:101:23 | Item2 |
 | Assignables.cs:108:13:108:13 | i |
 | Assignables.cs:109:14:109:14 | p |
 | Assignables.cs:113:25:113:25 | i |
@@ -69,8 +63,6 @@
 | Assignables.cs:132:13:132:13 | x |
 | Assignables.cs:133:29:133:29 | s |
 | Assignables.cs:138:19:138:19 | x |
-| Discards.cs:5:6:5:8 | Item1 |
-| Discards.cs:5:11:5:16 | Item2 |
 | Discards.cs:5:30:5:30 | x |
 | Discards.cs:19:14:19:14 | x |
 | Discards.cs:20:17:20:17 | y |
diff --git a/csharp/ql/test/library-tests/csharp7/TupleTypes.expected b/csharp/ql/test/library-tests/csharp7/TupleTypes.expected
@@ -1,12 +1,8 @@
-| (Int32,(String,Int32)) | (int, (string, int)) | ValueTuple<int, (string, int)> | 2 | 0 | CSharp7.cs:96:19:96:19 | Item1 |
-| (Int32,(String,Int32)) | (int, (string, int)) | ValueTuple<int, (string, int)> | 2 | 1 | CSharp7.cs:102:22:102:46 | Item2 |
-| (Int32,Double) | (int, double) | ValueTuple<int, double> | 2 | 0 | CSharp7.cs:213:6:213:8 | Item1 |
-| (Int32,Double) | (int, double) | ValueTuple<int, double> | 2 | 1 | CSharp7.cs:213:11:213:16 | Item2 |
-| (Int32,Int32) | (int, int) | ValueTuple<int, int> | 2 | 0 | CSharp7.cs:62:10:62:10 | Item1 |
-| (Int32,Int32) | (int, int) | ValueTuple<int, int> | 2 | 1 | CSharp7.cs:62:17:62:17 | Item2 |
-| (Int32,String) | (int, string) | ValueTuple<int, string> | 2 | 0 | CSharp7.cs:95:19:95:19 | Item1 |
-| (Int32,String) | (int, string) | ValueTuple<int, string> | 2 | 1 | CSharp7.cs:95:22:95:37 | Item2 |
-| (String,Int32) | (string, int) | ValueTuple<string, int> | 2 | 0 | CSharp7.cs:82:17:82:17 | Item1 |
-| (String,Int32) | (string, int) | ValueTuple<string, int> | 2 | 1 | CSharp7.cs:82:23:82:23 | Item2 |
-| (String,String) | (string, string) | ValueTuple<string, string> | 2 | 0 | CSharp7.cs:87:19:87:27 | Item1 |
-| (String,String) | (string, string) | ValueTuple<string, string> | 2 | 1 | CSharp7.cs:87:30:87:33 | Item2 |
+| (Int32,(String,Int32)) | (int, (string, int)) | ValueTuple<int, (string, int)> | 2 | 0 | Item1 |
+| (Int32,(String,Int32)) | (int, (string, int)) | ValueTuple<int, (string, int)> | 2 | 1 | Item2 |
+| (Int32,Double) | (int, double) | ValueTuple<int, double> | 2 | 0 | Item1 |
+| (Int32,Double) | (int, double) | ValueTuple<int, double> | 2 | 1 | Item2 |
+| (Int32,String) | (int, string) | ValueTuple<int, string> | 2 | 0 | Item1 |
+| (Int32,String) | (int, string) | ValueTuple<int, string> | 2 | 1 | Item2 |
+| (String,String) | (string, string) | ValueTuple<string, string> | 2 | 0 | Item1 |
+| (String,String) | (string, string) | ValueTuple<string, string> | 2 | 1 | Item2 |
diff --git a/csharp/ql/test/library-tests/csharp7/TupleTypes.ql b/csharp/ql/test/library-tests/csharp7/TupleTypes.ql
@@ -1,6 +1,6 @@
 import csharp
 
 from TupleType tt, int i
-where tt.getAnElement().fromSource()
+where tt.fromSource()
 select tt.getName(), tt.toStringWithTypes(), tt.getUnderlyingType().toStringWithTypes(),
-  tt.getArity(), i, tt.getElement(i)
+  tt.getArity(), i, tt.getElement(i).getName()
diff --git a/csharp/ql/test/library-tests/locations/A.cs b/csharp/ql/test/library-tests/locations/A.cs
@@ -5,8 +5,13 @@ public abstract class A<T>
     public abstract T Prop { get; }
     public abstract T this[int index] { get; set; }
     public abstract event EventHandler Event;
-    public void Apply(T t) { }
-    public abstract object ToObject(T t);
+    public void Apply(T t1) { }
+    public abstract object ToObject(T t2);
+    public object Field;
+    public A() { }
+    public A(T t) { }
+    ~A() { }
+    public static A<T> operator +(A<T> a1, A<T> a2) { return a1; }
 }
 
 public class A2 : A<string>
diff --git a/csharp/ql/test/library-tests/locations/locations.expected b/csharp/ql/test/library-tests/locations/locations.expected
diff --git a/csharp/ql/test/library-tests/locations/locations.ql b/csharp/ql/test/library-tests/locations/locations.ql

Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,10 @@ public override void Populate(TextWriter trapFile)`
`49`	`49`	`}`
`50`	`50`	`}`
`51`	`51`
`52`		`- WriteLocationsToTrap(trapFile.field_location, this, Locations);`
	`52`	`+ if (Context.ExtractLocation(Symbol))`
	`53`	`+ {`
	`54`	`+ WriteLocationsToTrap(trapFile.field_location, this, Locations);`
	`55`	`+ }`
`53`	`56`
`54`	`57`	`if (!IsSourceDeclaration \|\| !Symbol.FromSource())`
`55`	`58`	`return;`
Original file line number	Diff line number	Diff line change
`@@ -115,9 +115,10 @@ public override void Populate(TextWriter trapFile)`
`115`	`115`	`var type = Type.Create(Context, Symbol.Type);`
`116`	`116`	`trapFile.@params(this, Name, type.TypeRef, Ordinal, ParamKind, Parent!, Original);`
`117`	`117`
`118`		`- foreach (var l in Symbol.Locations)`
	`118`	`+ if (Context.ExtractLocation(Symbol))`
`119`	`119`	`{`
`120`		`- WriteLocationToTrap(trapFile.param_location, this, Context.CreateLocation(l));`
	`120`	`+ var locations = Context.GetLocations(Symbol);`
	`121`	`+ WriteLocationsToTrap(trapFile.param_location, this, locations);`
`121`	`122`	`}`
`122`	`123`
`123`	`124`	`if (!Symbol.Locations.Any() &&`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: minorAnalysis
 +---
 +* The extraction of location information for parameters, fields, constructors, destructors and user operators has been optimized. Previously, location information was extracted multiple times for each bound generic. Now, only the location of the unbound generic declaration is extracted during the extraction phase, and the QL library explicitly reuses this location for all bound instances of the same generic.
Original file line number	Diff line number	Diff line change
`@@ -213,7 +213,7 @@ class Parameter extends LocalScopeVariable, Attributable, TopLevelExprParent, @p`
`213`	`213`	`params(this, _, getTypeRef(result), _, _, _, _)`
`214`	`214`	`}`
`215`	`215`
`216`		`- override Location getALocation() { param_location(this, result) }`
	`216`	`+ override Location getALocation() { param_location(this.getUnboundDeclaration(), result) }`
`217`	`217`
`218`	`218`	`override string toString() { result = this.getName() }`
`219`	`219`
`@@ -449,7 +449,7 @@ class Field extends Variable, AssignableMember, Attributable, TopLevelExprParent`
`449`	`449`	`fields(this, _, _, _, getTypeRef(result), _)`
`450`	`450`	`}`
`451`	`451`
`452`		`- override Location getALocation() { field_location(this, result) }`
	`452`	`+ override Location getALocation() { field_location(this.getUnboundDeclaration(), result) }`
`453`	`453`
`454`	`454`	`override string toString() { result = Variable.super.toString() }`
`455`	`455`