Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing context regions for single-line #2616

Merged
merged 16 commits into from
Feb 14, 2023
Merged

Conversation

michaelcfanning
Copy link
Member

@michaelcfanning michaelcfanning commented Feb 13, 2023

Comment on lines 466 to 564
foreach (string po in new string[] { null, Environment.NewLine })
{
string[] tests =
{
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 3)}",
$"{new string(padding, 3)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 3)}{pr}{sentinel}{po}{new string(padding, 3)}",
$"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, ssl)}",
$"{new string(padding, 1)}{pr}{sentinel}{po}{new string(padding, ssl)}",
$"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, 1)}",
$"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, ssl)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, bsl)}",
$"{new string(padding, 10)}{pr}{sentinel}{po}{new string(padding, bsl)}",
$"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, 10)}",
$"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, bsl)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 3)}",
$"{new string(padding, 3)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 3)}{pr}{sentinel}{po}{new string(padding, 3)}",
$"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, ssl)}",
$"{new string(padding, 1)}{pr}{sentinel}{po}{new string(padding, ssl)}",
$"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, 1)}",
$"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, ssl)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, bsl)}",
$"{new string(padding, 10)}{pr}{sentinel}{po}{new string(padding, bsl)}",
$"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, 10)}",
$"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, 0)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, bsl)}",
$"{new string(padding, 0)}{pr}{sentinel}{po}{new string(padding, 0)}",
};

var context = new StringBuilder();
int iteration = 0;

// DEBUGGING THESE TESTS: these tests do not accumulate all outputs and report
// them, instead they break on the first failure. A failure will report a
// message like so:
//
// Expected contextRegion.Snippet not to be <null> because 'baz' snippet exists
// (iteration 12, while processing char-based region type for value 'abaza').
//
// Observe the iteration value (in this case 12). Set a conditional breakpoint
// below when the iteration variable equals this value and you can debug the
// relevant failure.

foreach (string test in tests)
{
var cache = new FileRegionsCache();
var uri = new Uri(@"c:\temp\DoesNotExist\" + Guid.NewGuid().ToString() + ".cpp");

int index = test.IndexOf(sentinel);

// The FileRegions code takes two discrete code paths depending on whether
// the input variable is char-offset based or uses the start line convention.
var regions = new Region[]
{
new Region
{
CharOffset = index,
CharLength = sentinel.Length,
},
new Region
{
StartLine = 1,
StartColumn = index + 1,
EndColumn = index + sentinel.Length + 1,
}
};

string charBased = "char-based";
string lineBased = "line-based";

foreach (Region region in regions)
{
context.Clear();
context.Append($"(iteration {iteration}, while processing {(region.StartLine == 1 ? lineBased : charBased)} region type for value '{test}')");

// First, we populate the region and text snippet for the actual test finding.
Region actual = cache.PopulateTextRegionProperties(region, uri, populateSnippet: true, test);

actual.Snippet.Should().NotBeNull($"'{sentinel}' snippet exists {context}");
actual.Snippet.Text?.Should().Be($"{sentinel}", $"region snippet did not match {context}");

// Now, we attempt to produce a context region.
Region contextRegion = cache.ConstructMultilineContextSnippet(actual, uri, test);
contextRegion.Snippet.Should().NotBeNull($"'{sentinel}' snippet exists {context}");
contextRegion.Snippet.Text.Contains(sentinel).Should().BeTrue($"context region should encapsulate finding {context}");
}

iteration++;
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Select

This foreach loop immediately [maps its iteration variable to another variable](1) - consider mapping the sequence explicitly using '.Select(...)'.
@michaelcfanning michaelcfanning marked this pull request as ready for review February 13, 2023 17:17
@@ -142,6 +142,9 @@ private Region PopulateTextRegionProperties(NewLineIndex lineIndex, Region input
return region;
}

internal const int BIGSNIPPETLENGTH = 512;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BIGSNIPPETLENGTH

Note that I've lifted out these magic values to make them more explicit in test use, and to allow product changes to flow more naturally to test validation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we prefer put at the top of the file with other definitions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will do this in a follow-on change, thanks for the review!!


// Generating full inputRegion to prevent issues.
Region originalRegion = this.PopulateTextRegionProperties(inputRegion, uri, populateSnippet: true);
Region originalRegion = this.PopulateTextRegionProperties(inputRegion, uri, populateSnippet: true, fileText);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fileText

We were not consistent around passing this optional argument around. This likely would break certain in-memory analysis operations that aren't highly exercised/well-tested today.

region.CharOffset = originalRegion.CharOffset < smallSnippetLength
? 0
: originalRegion.CharOffset - smallSnippetLength;
region.CharOffset = Math.Max(0, originalRegion.CharOffset - SMALLSNIPPETLENGTH);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CharOffset

Any prepended context region must start at the beginning of the file, any value that precedes this is invalid.

region.CharLength = originalRegion.CharLength + region.CharOffset + smallSnippetLength < newLineIndex.Text.Length
? originalRegion.CharLength + smallSnippetLength + Math.Abs(region.CharOffset - originalRegion.CharOffset)
: newLineIndex.Text.Length - region.CharOffset;
region.CharLength =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CharLength

Similarly, any context region that blows past the end of the actual file is invalid.

@michaelcfanning
Copy link
Member Author

    public void FileRegionsCache_IncreasingToLeftAndRight()

This test really should have been built out more. It tests one idiosyncratic case in our idiosyncratic system.

If the case when have a context region that itself exceeds the large snippet size (of 512 chars), our solution will add up to 128 chars of additional text to it, starting with the prepended side. This test shows that only the overflow from the unused 128 chars is appended to the region.

All of this behavior should first be reexamined and then tested in directed tests. This change is a hammer to ensure that all cases, in fact, produce context regions. We should ship this and then later fix this:

#2617


Refers to: src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs:707 in 520eb13. [](commit_id = 520eb13, deletion_comment = False)

Copy link
Collaborator

@shaopeng-gh shaopeng-gh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@michaelcfanning michaelcfanning merged commit 254ab10 into main Feb 14, 2023
@michaelcfanning michaelcfanning deleted the missing-context-regions branch February 14, 2023 00:54
@shaopeng-gh shaopeng-gh mentioned this pull request Feb 21, 2023
@shaopeng-gh shaopeng-gh mentioned this pull request Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants