comments on draft

[James Kupsch <kupsch@cs.wisc.edu>]

Larry,

Attached are my comments on the draft.  You can use change tracking to 
view them in the document, or there is a txt files with essentially the 
same comments.

Jim

Attachment: sarif-v2.0-csd02-provisiona-kupsch-comments.docx
Description: sarif-v2.0-csd02-provisiona-kupsch-comments.docx

Kupsch comments on the sarif-v2.0-csd02-provisiona-kupsch-comments.docx
Apr 10, 2019

------------------------------------------------------------
p.16 1.2 artifact definition:
a database table should not be included; it is neither byte nor URI addressable 
----

------------------------------------------------------------
p.22 1.3 UNICODE10
Unicode 12.0.0 is the current version
----

------------------------------------------------------------
p.26 3.3.2 text property
The document should be readable if the reference is removed, and having the name present aids the reader:   "that [RFC8259]" -> "that JSON [RFC8259]"
----

------------------------------------------------------------
p.30 3.4.6 Guidance on the use of artifactLocation objects
"... the root of the source code enlistment." ->
"... the path to the root of the directory tree containing the analyzed source code."
----

------------------------------------------------------------
p.33 3.7.3 Array properties with unique values
"in [JSCHEMA01]" -> "in JSON Schema [JSCHEMA01]"
----

------------------------------------------------------------
p.33 3.8.2 Tags
Add a note that for results and rules the preferred mechanism is to use taxonomies.
----

------------------------------------------------------------
p.35 3.9 Date/time properties
typo: "thos" -> "this"
----

------------------------------------------------------------
p.35 3.9 Date/time properties
add note that producers SHOULD include resolution of at least whole seconds for timestamps that are not product release dates.
----

------------------------------------------------------------
p.35 3.10.1 General
'The ":" delimiter is omitted' ->
'The ":" delimiter is omitted in the authority"
----

------------------------------------------------------------
p.36 3.10.2 URIs that use the file scheme
Add:  Remove empty path segments ("//" -> "/")
Add:  If the path is a directory include a trailing "/" to match URI semantics [RFC3986]
Add:  When normalizing file scheme paths, consumers shall remove empty path segments ("//" -> "/" and trailing "/" -> "").
----

------------------------------------------------------------
p.38 3.11.3 Plain text messages
I think the language about sentences should removed:

1) many of the existing tools in the SWAMP do not produce messages that are complete sentences, so this is hard to enforce for converters
2) I think there are languages that do not have sentences, and
3) programmatically defining a sentence is hard for english language messages:  'The question is "How is Mr. X!?"' ends on the double quote character.
4) in general this algorithm needs to handle different languages which is likely difficult.

A better solution would be to have an abbrText and abbrMarkdown type of messages for single line, short (for some definition of short such as 50 characters).  If not present display the first number of character followed by an elipse that fit the display area.
----

------------------------------------------------------------
p.38 3.11.3 Plain text messages
iI think the SHOULD be a single paragraph and discussion of of line break sequences should be removed. Having this restrictions makes it very difficult to write a plain text version of the markdown version. There are many tools that output multiple paragraph plain text messages and plain text consumers can display them just fine.  Generally, a post-process should not modify line break sequences.  Viewers should start a line break when a line break sequence is encountered is possible.  If a consumer cannot display multiple lines then it MAY eliminate line breaks.
----

------------------------------------------------------------
p.45 3.13.5 example
The example is incorrect:
externalPropertyFileReferences should be an object not an array
----

------------------------------------------------------------
p.50 3.14.7 language
"by [RFC5646]" -> "by Tags for Identifying Languages [RFC5646]"
----

------------------------------------------------------------
p.51 3.14.14 originalUriBaseIds property
It would be useful for client UX if there was a description for each uriBaseId to aid users in determining the meaning of each uriBaseId.  3.4.4 mentions that producers and consumers need to agree on the meaning which is not possible for generic viewer. Adding an optional "description" MultiFormatMessageString object to artifactLocation object would allow the producer to communicate this information to users.
----

------------------------------------------------------------
p.57 3.15.2 location property
No need to to restrict uriBaseId usage.  Change "its uri property SHALL be an absolute URI using the sarif scheme (§3.10.3), and its uriBaseId property SHALL be absent." -> "the resolved URI SHALL be an absolute URI using the sarif scheme (§3.10.3)."
----

------------------------------------------------------------
p.66 3.18.5 guid property
Add: "The guid is a stable identifier for the tool and as such all version of the tool SHALL have the same value."
----

------------------------------------------------------------
p.66 3.18.6 name property
It would be nice to have an optional abbrName property that is the tool name restricted to 15 characters for use by viewers and reports.  This allows a narrow column that can display the tool's name.  I have seen viewers where the default column name for the tool and rule is too narrow (by default usually) to display this information.
----

------------------------------------------------------------
p.66 3.18.8 semanticVersion property
"and semantics specified by [SEMVER]." -> "and semantics of Semantic Versioning [SEMVER]."
"to SemVer" -> "to Semantic Versioning"
----

------------------------------------------------------------
p.67 3.18.15 product property
how does this relate to name and fullName?
----

------------------------------------------------------------
p.68 3.18.18 fullDescription property
Remove first sentence paragraph and use shortDescription as sentence is ill-defined.
----

------------------------------------------------------------
p.72 3.18.26 artifactIndices property
why not array of artifactLocation.  This seems to be the only place where artifacts must be in the artifacts array.
----

------------------------------------------------------------
p.94 3.25.5 ruleId property
I think that restriction on converters synthizing ruleIds should be eliminated.

As ruleId is the unique id of the rule, if converters are not allowed to synthesize them, then there needs to be another mechanism to identify the same rule.  This restriction on converters should be removed.  Converters SHOULD synthesize them if possible.  Consumers should be aware that results from different converters are not comparable.  The advice to give is that consumers should consistently use the same converter or understand that there may be differences.  These differences may extend beyond just the ruleId.  Using different converters for the same tool should be avoided.
----

------------------------------------------------------------
p.103 3.25.14 fingerprints property
Why the restriction "A direct SARIF producer SHOULD NOT populate".  It seems like a direct produce MAY produce these fingerprints, and very well produce better fingerprints than other mechanisms.
----

------------------------------------------------------------
p.105 3.25.20 relatedLocations
A related location should say what is related to with a relatedTo property that is a URI (a sarif scheme uri relative to thisResult) this is necessary as a tool may produce a relatedLocation that has its own related location for instance error an error in foo.h may have a related location to say it was included from bar.h which was included in baz.c.  Also since there may also be multiple locations, they may each have unique related locations.
----

------------------------------------------------------------
p.146 3.44.3 id property
For viewers, it would be really useful to have a short (20-30) character human readable id (abbrName property) that can be used to display in columnar tables or UI sidebars that can be localized and is not necessarily stable.

Id's are not supposed to be generated by converters, but result management system is supposed to use them for comparison.
----

------------------------------------------------------------
p.149 3.44.10 fullDescription
Remove discussion about first sentence.  Require shortDescription instead.
----

------------------------------------------------------------
p.158 3.48.3 kinds property
How about MAY contain and make the default be [ "relevant" ]?  This is a reasonable default if a tool developer doesn't want to think about relationships.
----

------------------------------------------------------------
p.175 Appendix D (Informative) Production of SARIF by converters
A converter does not introduce any more or less complexity if it synthesizes properties, than a tool would if it makes mistakes in its output.  The advice for converters is to not change the semantics of the output without changing the semantic version of the converter.

Since the ruleId is important for viewer and result management system, it SHOULD be synthesized if possible

If you have different converter for the same tool's native result output, then they can produce incompatible value.  I find it unlikely that there will be multiple converter implementations for a given tool.

Consumers of SARIF should be aware that conversion results from different converters are not comparable.  Consumers can compare results only if the convert is the same and the sematic version of the converter and tool are compatible. Consumers should use the same converter if they wish to compare results in different SARIF files.
----

------------------------------------------------------------
p.175 Appendix D (Informative) Production of SARIF by converters
A converter does have a version number now in the conversion property, and it SHOULD include a version.
----

------------------------------------------------------------
p.177 Appendix D (Normative) Producing deterministic SARIF log files
The algorithms used by many tools are inherently non-deterministic as the tool's algorithms may do random sampling or random traversals of the graphs that represent the code.  Generally, they produce mostly the same result set, but there may be small differences between runs.  This fact should also included in this appendix.
----

------------------------------------------------------------
p.183 Appendix H (Informative) Diagnosing results in generated files
I don't see a need for different uriBaseIds anymore, just have multiple entries that resolve to the same absolute uri, but have a different values for for properties such as artifactIndex, hashes and lastModifiedTimeUtc; and the role will include "modified".
----

------------------------------------------------------------
p.184 Appendix I (Informative) Sample sourceLanguage values
Add more languages
----

------------------------------------------------------------
p.187 Appendix J.2 (Informative) Examples
"list:add" -> "list::add"
----

------------------------------------------------------------
p.193-197 Appendix J.4 (Informative) Examples
"list:add" -> "list::add"
----

------------------------------------------------------------
p.190 Appendix J.4 (Informative) Examples
"/home/buildAgent/src" -> "file:///home/buildAgent/src"
----

------------------------------------------------------------
p.192-193 Appendix J.4 (Informative) Examples
multiple times:  '"mimeType": "text/x-c"' -> '"sourceLanguage": "c"'
----





NOT INCLUDED:
-------------

    ------------------------------------------------------------
    p.44 3.13.2 version
    why is the version 2.1.0 and not 2.0.0?
    ----


    ------------------------------------------------------------
    p.64 3.18.3 Translations
    If you have or want translations the guid is required.
    "To facilitate the identification of translations..."  ->
    "If a toolComponent has translations or may have translations in the future, the toolComponent SHALL populate its guid property and a translation for that component SHALL set its guid property to the same value"
    Require the guid property to be set?
    ----



    ------------------------------------------------------------
    p.99 3.25.10
    review algorithm
    ----