-
Notifications
You must be signed in to change notification settings - Fork 91
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
Fix #1530: CS converter doesn't produce threadFlowLocation.location. #1531
Conversation
@michaelcfanning is added to the review. #Closed |
@harleenkohli is added to the review. #Closed |
@rscrivens is added to the review. #Closed |
@@ -1121,14 +1121,6 @@ private PhysicalLocation CreatePhysicalLocation(string uri, Region region = null | |||
}; | |||
} | |||
|
|||
private static readonly Regex LogicalLocationRegex = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogicalLocationRegex [](start = 38, length = 20)
Moved down, right before the new method RemoveReturnTypeFrom
. #ByDesign
@@ -1138,14 +1130,9 @@ private static string GetUserCodeLocation(Stack stack) | |||
foreach (StackFrame frame in stack.Frames) | |||
{ | |||
string fullyQualifiedLogicalName = frame.Location.LogicalLocation.FullyQualifiedName; | |||
Match match = LogicalLocationRegex.Match(fullyQualifiedLogicalName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fullyQualifiedLogicalName [](start = 57, length = 25)
There's no need to strip off the return type here, in this special case, because now we do it on every stack frame when we create it (see below). #ByDesign
@@ -1617,6 +1625,8 @@ private static void ReadStack(SparseReader reader, object parent) | |||
Debug.Assert(context.CurrentThreadFlowLocation == null); | |||
context.CurrentThreadFlowLocation = new ThreadFlowLocation | |||
{ | |||
Location = context.Signature.Location, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Location [](start = 16, length = 8)
After all the fuss and bother, this one-liner is the fix for #1530. The rest was just making sure the fully qualified logical name was in the right form. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't like what we've done to FQN, we've elided the information that qualifies one method overload over the other. this isn't heplful and will compromise result matching.
In reply to: 293025496 [](ancestors = 293025496)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the other two comments -- I will restore the signature to the FQN.
In reply to: 293604799 [](ancestors = 293604799,293025496)
The result of the fix: every Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:134 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False) |
The first of hundreds of places where we stripped off the return type and the method signature from the fully qualified logical name, leaving just the namespace-qualified method name. #Resolved Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:113 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False) |
why is this a good change? Removing the parens suggests we aren't providing params data, are we not? #Resolved Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:3049 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = True) |
this isn't a fully qualified name, there are many string.concat overloads, so which one is this>? you need to populate this before doing the stack frame stripping, i think #Resolved Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:12246 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
It was the same change that stripped off the return type. I'll restore the signature (including empty parens when there are no arguments). In reply to: 501905176 [](ancestors = 501905176) Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:12246 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False) |
It's not. I got overaggressive when I stripped off the return type. Will fix. In reply to: 501904892 [](ancestors = 501904892) Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:3049 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = True) |
Per feedback, we now strip just the return type, and leave the argument signature. In reply to: 501370995 [](ancestors = 501370995) Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:113 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the CS converter translates a sequence of
propagation-event
elements into athreadFlow
, it populates eachthreadFlowLocation.stack
from thepropagation-event
'sstack
sub-element. The converter also takes thepropagation-event
'ssignature
sub-element and places it at the top of the stack.But the converter does not populate
threadFlowLocation.location
, which confuses the VS Code Viewer, which expectslocation
to be present. Sure, we could "fix" the viewer, but really,signature
does specify the location, and we should populate it.While looking at the code I found a couple more issues:
run.language
="en-US"
, which is wrong because it's the default. I removed it. I have a fix in another branch for the test infrastructure bug that failed to detect this.threadFlowLocation
object now haslocation
property in addition to thestack
property.