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

Transform v2 -> v1 stack, stackFrame, region #911

Merged
merged 2 commits into from
Jun 1, 2018

Conversation

easyrhino-gh
Copy link
Contributor

Please check the logicalLocations (aka the cold trail from hell), I'm not confident there.

@easyrhino-gh easyrhino-gh requested a review from a user May 31, 2018 23:57
},
""region"": {
""startLine"": 15,
""startColumn"": 9
Copy link

Choose a reason for hiding this comment

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

@michaelcfanning @EasyRhin0

Chris, you have no action for now, but be aware:

At TC #18 on May 30th, we approved changes to the default values of missing region properties. In particular, if endColumn is missing, in v1 that meant endColumn == startColumn (an insertion point). But in v2, endColumn defaults to “one character past EOL (excluding the newline sequence”).

The reason for this change is that one of the TC members argued persuasively that the most natural interpretation of the region { "startLine": 3 } is "The whole of line 3" and not "An insertion point at the beginning of line 3."

So when you get around to making the schema changes for oasis-tcs/sarif-spec#93, you'll need to adjust the transformations as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks

""name"": ""CodeScanner"",
""semanticVersion"": ""2.1.0""
},
""invocation"": {},
Copy link

Choose a reason for hiding this comment

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

Don't emit empty object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens when there are notifications but no other properties set on the invocation.

Copy link

Choose a reason for hiding this comment

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

Can you stop if from happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. I'd have to check all of the strings for null/whitespace, and the processId == 0, and the datetimes == datetime.minvalue. Would that be deterministic?

Copy link

Choose a reason for hiding this comment

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

Yes, but now that you spell it out, not worth the trouble. After all, if we're copying an entire run to the property bag, I've got now reason to gripe about this. Let it go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

""uri"": ""file:///C:/src/main.cs"",
""module"": ""RuleLibrary"",
""threadId"": 52,
""fullyQualifiedLogicalName"": ""Rules.SecureHashAlgorithmRule.Register-0"",
Copy link

@ghost ghost Jun 1, 2018

Choose a reason for hiding this comment

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

In v2, the stackFrame.fullyQualifiedLogicalName property is always the key into the files dictionary (if the files dictionary exists). So you have to do this:

IF the files dictionary exists
    AND it has a key that matches v2 stackFrame.fullyQualifiedLogicalName
    AND the file object specified by that key has a fullyQualifiedName property
THEN
    set v1 stackFrame.fullyQualifiedLogicalName to v2 file.fullyQualifiedName
    set v1 stackFrame.logicalLocationKey to v2 stackFrame.fullyQualifiedLogicalName
ELSE
    set v1 stackFrame.fullyQualifiedLogicalName to v2 stackFrame.fullyQualifiedLogicalName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. This actually clarifies it quite a bit, thanks

return region;
}

internal RegionVersionOne CreateRegion(int startColumn, int startLine, int endColumn = 0, int endLine = 0, int length = 0, int offset = 0)
Copy link

Choose a reason for hiding this comment

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

Why do you need this overload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from the other transformer, where I do need it. Not needed here yet, will remove.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Logical location handling bug; possible unnecessary method overload.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Ship it!

@easyrhino-gh easyrhino-gh merged commit 3c58e00 into sarif-v2 Jun 1, 2018
@easyrhino-gh easyrhino-gh deleted the v2-to-v1-stack-stackFrame branch June 1, 2018 18:36
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.

1 participant