Add new visitor to get deterministic SARIF log by sorting results #2422
Conversation
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Linq; |
There was a problem hiding this comment.
I see several instances where using System.Linq is greyed out in VS, can you double check where this is actually needed and remove any unnecessary? #Resolved
There was a problem hiding this comment.
It is required in this file, checked other files and removed the unnecessary using.
| { | ||
| internal static readonly ReportingDescriptorSortingComparer Instance = new ReportingDescriptorSortingComparer(); | ||
|
|
||
| public int Compare(ReportingDescriptor left, ReportingDescriptor right) |
There was a problem hiding this comment.
This solution is a short term fix to help resolving issues some tools (E.g. PREfast) generate non-deterministic results for same targets run-over-run asap.
It mainly focus on relevant types cause the non-determinisms we observed in the sample/test SARIF files.
To compare all the fields we need to manually implement IComparer for all SARIF types. E.g. if compare Relationships, RelationshipComparer need to be implemented.
In planned comprehensive solution, we will update JSchema to automatically generates IComparer code for all SARIF types instead of these hand-written code, at that time we will make sure all types are used for comparing. Created issue #2424 for tracking.
There was a problem hiding this comment.
thanks for the detail explain.
| { | ||
| new CodeFlow | ||
| { | ||
| ThreadFlows = new [] |
There was a problem hiding this comment.
which tool do you use? I ran dotnet format before commit, it didn't catch all of these additional spaces.
There was a problem hiding this comment.
in VS right click file -> code clean up with the default profile.
| { | ||
| internal class ArtifactContentSortingComparer : IComparer<ArtifactContent> | ||
| { | ||
| internal static readonly ArtifactContentSortingComparer Instance = new ArtifactContentSortingComparer(); |
There was a problem hiding this comment.
it's a public (internal) member. It should match naming convention: "All visible static member names are pascal cased."
src/Sarif/Visitors/SortingVisitor.cs
Outdated
| // save old indexes | ||
| for (int i = 0; i < node.Artifacts.Count; i++) | ||
| { | ||
| if (!oldIndexes.TryGetValue(node.Artifacts[i], out int index)) |
src/Sarif/Visitors/SortingVisitor.cs
Outdated
| // before sort the rules, save old indexes | ||
| for (int i = 0; i < current.Rules.Count; i++) | ||
| { | ||
| if (!oldIndexes.TryGetValue(current.Rules[i], out int index)) |
fix dotnet format
…microsoft/sarif-sdk into users/yongyan-gh/sortingvisitor
| public void SortingVisitor_ShuffleTest() | ||
| { | ||
| bool areEqual; | ||
| // create a test sarif log |
There was a problem hiding this comment.
Yes, this note applies. This is not a helpful comment in any case.
| return compareResult; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you define a helper for this? #Resolved
There was a problem hiding this comment.
Moved to the common method in ComparerHelper
| return compareResult; | ||
| } | ||
|
|
||
| compareResult = string.Compare(left.Hashes.ElementAt(i).Value, right.Hashes.ElementAt(i).Value); |
There was a problem hiding this comment.
fixed. new implementation in ComparerHelper.CompareDictionary(...)
| return compareResult; | ||
| } | ||
|
|
||
| for (int index_1 = 0; index_1 < left.ThreadFlows.Count; ++index_1) |
| /// </summary> | ||
| namespace Microsoft.CodeAnalysis.Sarif | ||
| { | ||
| internal class ReportingConfigurationSortingComparer : IComparer<ReportingConfiguration> |
There was a problem hiding this comment.
Updated all comparers class name to {typename}Comparer
src/Sarif/Visitors/SortingVisitor.cs
Outdated
|
|
||
| public SortingVisitor() | ||
| { | ||
| ruleIndexMap = new ConcurrentDictionary<int, int>(); |
There was a problem hiding this comment.
Visitor traverses entire Sarif log by properties in certain order. I could not think multi threads scenario for this. If its true I will change it to normal dictionary.
There was a problem hiding this comment.
changed to Dictioanry<>
| { | ||
| public class SortingVisitor : SarifRewritingVisitor | ||
| { | ||
| private readonly IDictionary<int, int> ruleIndexMap; |
There was a problem hiding this comment.
changed to Dictionary
|
Add code coverage data: |
| internal class ComparerHelper | ||
| { | ||
| /// <summary> | ||
| /// Compare 2 object refences. Return value false presents need to |
| internal class ComparerHelper | ||
| { | ||
| /// <summary> | ||
| /// Compare 2 object refences. Return value false presents need to |
There was a problem hiding this comment.
Worked with Courtney got all user facing strings reviewed
| /// -1 if the first object is null and the second object is not null. | ||
| /// 1 if the first object is not null and the second object is null. | ||
| /// </param> | ||
| /// <returns>Return true if can get a definite compare result, otherwise return false.</returns> |
There was a problem hiding this comment.
how is this getting compiled? where? who can see your comment outside this source file?
otherwise convert this comment to a regular comment and make sure it's useful. #Resolved
There was a problem hiding this comment.
IDE intellisense shows this comment for people wants to call it. I d like to keep the comment for this method since it has some behaviors cannot tell by just signatures.
Removed comments for other methods in this file.
There was a problem hiding this comment.
changed to normal comments since its internal class its no visible outside of Sarif.Sdk
| /// 1 if the first object is not null and the second object is null. | ||
| /// </param> | ||
| /// <returns>Return true if can get a definite compare result, otherwise return false.</returns> | ||
| public static bool CompareReference(object left, object right, out int result) |
There was a problem hiding this comment.
renamed.
to be consistent with other compares method i d like to return int instead of nuallable int. what do you think?
|
|
||
| namespace Microsoft.CodeAnalysis.Sarif.Comparers | ||
| { | ||
| internal class ComparerHelper |
There was a problem hiding this comment.
Changed from helper methods to extension methods.
Renamed this method to 'TryReferenceCompares', following .net "ReferenceEquals" naming
|
|
||
| if (object.ReferenceEquals(left, right)) | ||
| { | ||
| result = 0; |
There was a problem hiding this comment.
unnecessary assignment here. #Resolved
|
|
||
| private static int CompareDictionaryHelper<T>(IDictionary<string, T> left, IDictionary<string, T> right, Func<T, T, int> compareFunc) | ||
| { | ||
| if (compareFunc == null) |
| { | ||
| return compareResult; | ||
| } | ||
|
|
There was a problem hiding this comment.
if (left.TryReferenceCompare(right, out result))
{
return result;
}
if ((object)left.TryReferenceCompare((object)right, out result))
{
return result;
}
#Resolved
| /// </summary> | ||
| namespace Microsoft.CodeAnalysis.Sarif.Comparers | ||
| { | ||
| internal class CodeFlowComparer : IComparer<CodeFlow> |
There was a problem hiding this comment.
was following the same pattern of Sarif SDK's EqualityComparers. The EqualityComparer class is internal and have a public reference in Sarif types, e.g.
public static IEqualityComparer ValueComparer => ArtifactLocationEqualityComparer.Instance;
| } | ||
|
|
||
| [Fact] | ||
| public void CompareList__Shuffle_Tests() |
| } | ||
|
|
||
| [Fact] | ||
| public void CompareList__Shuffle_Tests() |
There was a problem hiding this comment.
Random object created with RandomSarifLogGenerator.GenerateRandomAndLog logs the seed in test output. Below is an example:
[xUnit.net 00:00:25.02] Microsoft.CodeAnalysis.Test.UnitTests.Sarif.Comparers.ComparersTests.CompareList_Shuffle_Tests [FAIL]
Failed Microsoft.CodeAnalysis.Test.UnitTests.Sarif.Comparers.ComparersTests.CompareList_Shuffle_Tests [203 ms]
Error Message:
Expected result to be 0, but found -1.
Stack Trace:
at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message)
at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args)
at FluentAssertions.Numeric.NumericAssertions`1.Be(T expected, String because, Object[] becauseArgs)
at Microsoft.CodeAnalysis.Test.UnitTests.Sarif.Comparers.ComparersTests.CompareList_Shuffle_Tests() in C:\repo\github.com\sarif-sdk\src\Test.UnitTests.Sarif\Comparers\ComparersTests.cs:line 40
Standard Output Messages:
TestName: CompareList_Shuffle_Tests has seed 727824218
Also updated the RandomSarifLogGenerator.GenerateRandomAndLog to accept a specific seed so dev can easily reuse the seed in the failed case:
Random random = RandomSarifLogGenerator.GenerateRandomAndLog(this.output, seed: 727824218);
|
CodeQL reported 1 error and 1 warning both are false-positive.
|
|
This pull request introduces 1 alert when merging f18e005 into bedc46e - view on LGTM.com new alerts:
|
Description
Some static analysis tools generate non-deterministic SARIF results, means if scan the same targets multiple times, the same issues are detected, but in each output SARIF file the entities in collection (e.g. rules/results/locations) are arranged in random order. It causes the issues for the users want to check if 2 SARIF results present the same set of issues by comparing them using bitwise diff tools.
Fixes
IComparerinterface for relevant SARIF entities.Can get sorted results by adding
--sort-resultsto rewrite command.This is a short term fix so that the customers can have their issue resolved asap. The comprehensive solution will be auto-generated Comparer implementation code by JSchema for all the SARIF entities instead of handwritten Comparer code.