C#: Reduce location tuples. #20525
C#: Reduce location tuples. #20525
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
michaelnebel
force-pushed
the
csharp/reducelocationtuples
branch
3 times, most recently
from
295307c to
01b789d
Compare
…lways create one such location.
… can be unfound) and don't extract empty locations.
michaelnebel
force-pushed
the
csharp/reducelocationtuples
branch
from
457e9b3 to
018ccb3
Compare
|
@hvitved : Could you review, even though still i draft? |
|
FYI: The most recent DCA run forgot the |
Ran it again, DCA reports the same alert changes. Performance looks good. |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes location extraction for C# entities by only extracting locations for unbound declarations to avoid duplication and improve performance. The changes ensure that bound generic entities reuse the location information from their unbound counterparts.
- Modified location extraction logic to use unbound declarations instead of extracting multiple times for bound generics
- Added utility methods to filter out empty locations during trap file writing
- Renamed
GeneratedLocationtoEmptyLocationfor clarity and ensured it's always extracted
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
csharp/ql/lib/semmle/code/csharp/*.qll |
Updated location retrieval methods to use unbound declarations |
csharp/extractor/Semmle.Extraction.CSharp/Entities/*.cs |
Added conditional location extraction and helper methods for filtering empty locations |
csharp/extractor/Semmle.Extraction.CSharp/Entities/Locations/EmptyLocation.cs |
Renamed from GeneratedLocation to EmptyLocation |
csharp/extractor/Semmle.Extraction.CSharp/Extractor/*.cs |
Updated context methods and ensured empty location creation |
csharp/ql/test/library-tests/locations/* |
Test files for verifying location extraction behavior |
| if (attributeSyntax is not null) | ||
| { | ||
| trapFile.attribute_location(this, Assembly.CreateOutputAssembly(Context)); | ||
| WriteLocationToTrap(trapFile.attribute_location, this, Assembly.CreateOutputAssembly(Context)); |
There was a problem hiding this comment.
This call to WriteLocationToTrap appears incorrect. The second parameter should be the location, but Assembly.CreateOutputAssembly(Context) returns an Assembly entity, not a Location. This will likely cause a compilation error or runtime issue.
| WriteLocationToTrap(trapFile.attribute_location, this, Assembly.CreateOutputAssembly(Context)); | |
| WriteLocationToTrap(trapFile.attribute_location, this, attributeSyntax.GetLocation()); |
Copilot uses AI. Check for mistakes.
| public override void Populate(TextWriter trapFile) | ||
| { | ||
| var typeKey = VarargsType.Create(Context); | ||
| // !! Maybe originaldefinition is wrong |
There was a problem hiding this comment.
The comment has a spelling error: 'originaldefinition' should be 'original definition' (two words).
| // !! Maybe originaldefinition is wrong | |
| // !! Maybe original definition is wrong |
Copilot uses AI. Check for mistakes.
|
DCA confirms a reduction in TRAP size, |
In this PR we change, how locations are extracted.
That is,
*IDs for extracting locations.With this change we do get a minor change to what locations are reported.
DCA shows "small" alerts discrepancies for
cs/dispose-not-called-on-throwcs/dereferenced-value-may-be-nullI investigated the discrepancy for
cs/dispose-not-called-on-throwfor ASP.NET Core and it appears to boil down to the following issue: When extracting methods, we now only extract locations for unbound declarations and then use these locations for the method. However, it appears that we may extract multiple locations for unbound callables - both a location in source (for the override) and an assembly location (for the prototype). This could affect that the "best location" of a method goes from assembly to source (which could affect logic that uses whether a callable is in source or library).For the cases I investigated for
cs/dispose-not-called-on-throwit appears that some results are removed, where it is assumed that library methods can throw any exception - as this is just an approximation based on location - I think we can accept the discrepancy.