Updating SARIF2004 #1995
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -393,13 +393,25 @@ Similarly, most 'result' objects contain at least one 'artifactLocation' object. | |
|
|
||
| #### Messages | ||
|
|
||
| ##### `AvoidDuplicativeAnalysisTarget`: warning | ||
|
|
||
| The 'analysisTarget' property '{1}' at '{0}' is unnecessary because it is the same as the result location. Remove the 'analysisTarget' property. | ||
|
There was a problem hiding this comment.
Please add something like: 'The 'analysisTarget' property is used to distinguish cases when a result fires in a file (such as an included header) that is different than the file that was scanned (such as a .cpp file that included the header). This C++ example example is the canonical scenario that led to this design pattern. Also, though it is implied, the reason we recommend removing it is to optimize file size. The 'analysisTarget' property '{1}' at '{0}' can be removed because it is the same as the result location. This unnecessarily increases log file size. 'The 'analysisTarget' property is used to distinguish cases when a result fires in a file (such as an included header) that is different than the file that was scanned (such as a .cpp file that included the header). There was a problem hiding this comment. ... except that instead of "when a result fires" I suggest "when a tool detects a result". In reply to: 455907085 [](ancestors = 455907085,455904829) There was a problem hiding this comment. I didn't intend to take over reauthoring the words, just point you in the right direction. :) feel free to send Eddy updates after it goes through your wordsmithing process. Every string needs a pass in my estimation. I need the team to send me the logs you generated in testing so that I can see what your new output looks like in bulk. If we aren't yet doing that testing, it needs to be included in the formalized process. In reply to: 455907934 [](ancestors = 455907934,455907085,455904829) There was a problem hiding this comment. just updated this as well! In reply to: 455913218 [](ancestors = 455913218,455907934,455907085,455904829) |
||
|
|
||
| #### `AvoidDuplicativeResultRuleInformation`: warning | ||
|
|
||
| The result at '{0}' uses the 'rule' property to specify the violated rule, so it is not necessary also to specify 'ruleId' or 'ruleIndex'. Remove the 'ruleId' and 'ruleIndex' properties. | ||
|
There was a problem hiding this comment. You have a tendency to introduce descriptive prefixes in rules that I find distracting. The '{0}' value tends to be sufficiently descriptive to illuminate what's under discussion. This is most easily seen when actually reviewing output of the tool (which I encourage you to do), particularly real world quantities of output, not just a small # of test messages. In this second example. we see that we're trying to evolve to a common output format string for duplicative information (and we should converge on a standard). '{0}' uses the 'rule' property to specify the violated rule, so it is not necessary also to specify 'ruleId' or 'ruleIndex'. This unnecessarily increases log file size. Remove the 'ruleId' and 'ruleIndex' properties. #Closed There was a problem hiding this comment. I'm open to this but I ask you to consider the potential value of the descriptive prefixes. The problem is that the JSON paths can be very long, and without the prefix, the first thing that meets your eye is this long string. Consider an example where it's really long: I think this provides the reader with an easier entry to the message: ... because it says "I'm about to tell you something about a URI". Without the prefix, you have to scan to the end of the path string to learn that it's about a URI. In reply to: 455907068 [](ancestors = 455907068) There was a problem hiding this comment. Your example doesn't correspond to my feedback, which notes that describing the SARIF JSON type is generally not required because the type information tends to be encoded in the JSON path. The term 'URI' isn't typically encoded in the URI and so it is useful to identify the string literal as one. In reply to: 455936290 [](ancestors = 455936290,455907068) There was a problem hiding this comment. Ok, I understand the distinction. In reply to: 455941928 [](ancestors = 455941928,455936290,455907068) There was a problem hiding this comment. |
||
|
|
||
| ##### `EliminateLocationOnlyArtifacts`: warning | ||
|
|
||
| The 'artifacts' array at '{0}' contains no information beyond the locations of the artifacts. Removing this array might reduce the log file size without losing information. In some scenarios (for example, when assessing compliance with policy), the 'artifacts' array might be used to record the full set of artifacts that were analyzed. In such a scenario, the 'artifacts' array should be retained even if it contains only location information. | ||
|
|
||
| ##### `EliminateIdOnlyRules`: warning | ||
|
|
||
| The 'rules' array at '{0}' contains no information beyond the ids of the rules. Removing this array might reduce the log file size without losing information. In some scenarios (for example, when assessing compliance with policy), the 'rules' array might be used to record the full set of rules that were evaluated. In such a scenario, the 'rules' array should be retained even if it contains only id information. | ||
|
There was a problem hiding this comment.
I'd like to see actual instances of this rule output before approving. Can you provide some from your testing? Note that you have some language here 'Removing this array might reduce log file size' that should converge on a standard message. Also, why do we have the 'might'? This analysis s/be very clear whether our suggestion can be taken without implying information loss. If not, let's keep working on the analysis. #Resolved There was a problem hiding this comment. I've just sent you the output from running the validator on a SARIF file produced from scanning a significant repo. In reply to: 455908031 [](ancestors = 455908031) |
||
|
|
||
| #### `PreferRuleId`: warning | ||
|
|
||
| The result at '{0}' uses the 'rule' property to specify the violated rule, but this is not necessary because the rule is defined by 'tool.driver'. Use the 'ruleId' and 'ruleIndex' instead, because they are shorter and just as clear. | ||
|
|
||
| --- | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| using System; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Sarif.Multitool | ||
| { | ||
| public static class Extensions | ||
| { | ||
| public static bool RefersToDriver(this ToolComponentReference toolComponent, string driverGuid) | ||
| { | ||
| if (toolComponent.Index == -1) | ||
| { | ||
| if (toolComponent.Guid == null) | ||
| { | ||
| return true; | ||
| } | ||
| else | ||
| { | ||
| return toolComponent.Guid.Equals(driverGuid, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| } | ||
|
|
||
| return 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.
We should have code that auto-emits this content, rather than maintaining it in three places (resource strings, as comments in rule source, and here). Action item for later. #Pending