Query metadata style guide: add to ql/docs#581
Conversation
|
|
| * [QL language specification](https://help.semmle.com/QL/QLLanguageSpecification.html) | ||
| * [QL style Guide](https://github.com/Semmle/ql/blob/master/docs/ql-style-guide.md) | ||
|
|
||
| For examples of query files for the language supported by Semmle, see the following pages: |
|
|
||
| ## Metadata area | ||
|
|
||
| Query file metadata contains important information which defines the name and purpose of the query. In order to help others use your query, and to ensure that the query works correctly on LGTM.com, you should include all of the required information outlined below in the metadata, and as much of the optional information as as possible. For further information on query metadata see [Query file requirements](https://help.semmle.com/wiki/display/SD/Query+file+requirements). |
|
|
||
| ### Query descriptions `@description` | ||
|
|
||
| You must define an `@description` property for your query. This property defines a short help message. Query descriptions should be written as a sentence or short-paragraph of plain prose, with sentence capitalization and full stop. The preferred pattern for alert queries is `Syntax X causes behavior Y.` Any code elements included in the description should be enclosed in single quotes. For example: |
There was a problem hiding this comment.
the pattern "Syntax X causes behavior Y" should not be quoted with backticks. Double-quotes " or something else is probably more appropriate.
| * `@id cs/command-line-injection` | ||
| * `@id java/string-concatenation-in-loop` | ||
|
|
||
| Note, `@id` properties should be consistent for queries that highlight the same issue for different languages. For example, the following queries identify format strings which contain unsanitized input in Java and C++ code respectively: |
| * `@problem.severity`–defines the level of severity of the alert: | ||
| * `error`–an issue that is likely to cause incorrect program behavior, for example a crash or vulnerability. | ||
| * `warning`–an issue that indicates a potential problem in the code, or makes the code fragile if another (unrelated) part of code is changed. | ||
| * `recommendation`–an issue that results in code that is hard to read, but is otherwise correct. |
There was a problem hiding this comment.
It's probably not all recommendations that are code-is-hard-to-read issues - even though it might be the common case. So this definition should be loosened up a bit.
There was a problem hiding this comment.
Changed to: recommendation–an issue where the code behaves correctly, but it could be improved.
|
|
||
| The `@tags` property is used to define categories that the query relates to. Each query should belong to one (or more, if necessary) of the following four top-level categories: | ||
|
|
||
| * `@tags correctness`–for queries that detect common coding mistakes. |
There was a problem hiding this comment.
No that seems wrong - that's not what correctness is about. A definition along the lines of "for queries that detect wrong program behavior" is better, I think.
| The `@tags` property is used to define categories that the query relates to. Each query should belong to one (or more, if necessary) of the following four top-level categories: | ||
|
|
||
| * `@tags correctness`–for queries that detect common coding mistakes. | ||
| * `@tags maintainability`–for queries that detect opportunities for structural improvement. |
There was a problem hiding this comment.
I think "opportunities for structural improvement" is slightly off - what does it even mean. Something like ".. patterns that make it harder for developers to make correct changes to code" is better (stolen directly from the line below as it fits better here)
|
|
||
| * `@tags correctness`–for queries that detect common coding mistakes. | ||
| * `@tags maintainability`–for queries that detect opportunities for structural improvement. | ||
| * `@tags readability`–for queries that detect confusing or dangerous patterns that make it harder for developers to make correct changes to code. |
There was a problem hiding this comment.
Things that make it harder to change the code is probably more in the realms of maintainability even though the two categories are close. Also "dangerous" is probably a bit strong. So it should probably be something like "for queries that detect confusing patterns that make it harder for developers to understand the code" instead.
|
|
||
| ### Alert messages | ||
|
|
||
| You must define an message in the select clause of an alert query to display with your results. Alert messages are strings that concisely describe the problem that the alert is highlighting and, if possible, also provide some context. For consistency, alert messages should adhere to the following guidelines: |
| You must define an message in the select clause of an alert query to display with your results. Alert messages are strings that concisely describe the problem that the alert is highlighting and, if possible, also provide some context. For consistency, alert messages should adhere to the following guidelines: | ||
|
|
||
| * Each message should be a complete, standalone sentence. That is, it should be capitalized and have proper punctuation, including a full stop. | ||
| * The message should factually describe the problem that is being highlighted–they should not contain recommendations about how to fix the problem or value judgements. |
aschackmull
left a comment
aschackmull
left a comment
There was a problem hiding this comment.
Mostly LGTM, but the definitions of the main query tags are off.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Glad to see this information being added to our documentation. A few comments.
|
|
||
|
|
||
| * `@id java/tainted-format-string` | ||
| * `@id cpp/tainted-format-string` |
There was a problem hiding this comment.
Do we want to say anything about the possibility of grouping query IDs, or include an example that does this such as:
* @id cpp/dependency/redundant-include
|
|
||
|
|
||
| * alerts (`@kind problem`) | ||
| * alerts containing path information (`@kind path-problem`) |
There was a problem hiding this comment.
I don't think this information about path-problem is covered on our internal wiki. Perhaps we should add it or link to this doc page from https://wiki.semmle.com/display/IN/Writing+queries+and+query+help?
More generally, I think there's a widespread issue (not specific to this PR) of duplicating information between our internal wiki and publically available documentation, with possible consequences including inconsistencies, and Semmle developers with access to both forgetting to read / update one or other source of information. Perhaps in these cases we should aim for the wiki page to be a small stub page that only (1) provides a prominent link to the doc page and (2) provides any additional information and advice that for whatever reason we don't deem suitable for the general audience.
There was a problem hiding this comment.
Fair point. I'll be adding a 'query help style-guide' in a future PR, so a good solution for that wiki page would be to link to these docs.
| * `@problem.severity`–defines the level of severity of the alert: | ||
| * `error`–an issue that is likely to cause incorrect program behavior, for example a crash or vulnerability. | ||
| * `warning`–an issue that indicates a potential problem in the code, or makes the code fragile if another (unrelated) part of code is changed. | ||
| * `recommendation`–an issue that results in code that is hard to read, but is otherwise correct. |
There was a problem hiding this comment.
A recommendation isn't the same as a readability issue though. There's a big overlap, but we have for example recommendations about using or not using particular language features. I agree with the assessment that these issues flag places where the code is likely to be 'otherwise correct', i.e., not actually behaving incorrectly.
|
@aschackmull thanks for reviewing. Hopefully the |
|
|
||
| * `@tags correctness`–for queries that detect incorrect program behavior. | ||
| * `@tags maintainability`–for queries that detect patterns that make it hard for developers to make changes to the code. | ||
| * `@tags readability`–for queries that detect confusing patterns that make it hard for developers to read the code. |
There was a problem hiding this comment.
In both maintainability and readability I would prefer the wording "..make it harder for developers.." to the current "..make it hard for developers..", as "harder" indicates that things are more difficult than they could otherwise be, which is probably in line with what we claim in the various queries, but claiming that things are "hard" is a slightly different judgment that I don't think we can claim.
|
I have just added a sentence explaining that the metadata must be included as the body of QLDoc comment, and included an example. |
|
Looks fine. |
felicitymay
left a comment
felicitymay
left a comment
There was a problem hiding this comment.
This generally looks good to me. A few comments, mostly about links 😉.
| * `@tags readability`–for queries that detect confusing patterns that make it harder for developers to read the code. | ||
| * `@tags security`–for queries that detect security weaknesses. See below for further information. | ||
|
|
||
| There are also more specific `@tags` that can be specified. See, the following pages for more information on the low-level tags: |
There was a problem hiding this comment.
Those pages don't give any information about the tags, they just show the range of tags that are currently defined (albeit with some interpretation by the Confluence uploader, like splitting on slashes). These URLs will need to change to link to help.semmle.com/wiki - the current links are to internal-only preview pages.
There was a problem hiding this comment.
True about the lack of information on these pages. I've reworded slightly to simply say 'see these pages for more examples'.
| * Where you reference another program element, link to it if possible using a substitution (`$@`). Links should be used inline in the sentence, rather than as parenthesised lists or appositions. | ||
| * When a message contains multiple links, construct a sentence that has the most variable link (that is, the link with most targets) last. | ||
|
|
||
| See the query homepages for examples of alert messages. |
There was a problem hiding this comment.
I mean the uploaded query/qhelp pages here.
felicitymay
added
WIP
|
(I added the WIP label because the links need to be revised before this is merged.) |
|
@felicity-semmle - thanks for the comments. I have been able to address most of them. |
felicitymay
left a comment
felicitymay
left a comment
There was a problem hiding this comment.
Thanks for the changes. I spotted a few remaining small issues.
Regarding linking to the new Qhelp spaces or not, that probably depends on when we want to merge this PR. If we want to merge it soon, then delete the links for now. If you're happy to wait until middle/end of next week, then keep them.
| * alerts containing path information (`@kind path-problem`) | ||
|
|
||
| These `@kind` properties support two further mandatory properties which are added by Semmle after the query has been tested, prior to deployment to LGTM. The following information is for reference: | ||
| These `@kind` properties support two further properties which are added by Semmle after the query has been tested, prior to deployment to LGTM. The following information is for reference: |
There was a problem hiding this comment.
Apologies - my suggestions was bad - this now looks odd with two "properties". Suggest: "Alert queries (@kind problem or path-problem) support two further properties. These are usually added by Semmle after..."
There was a problem hiding this comment.
I should have noticed this too.
| * `@tags security`–for queries that detect security weaknesses. See below for further information. | ||
|
|
||
| There are also more specific `@tags` that can be specified. See, the following pages for more information on the low-level tags: | ||
| There are also more specific `@tags` that can be specified. See, the the following pages for examples of the low-level tags: |
| * Where you reference another program element, link to it if possible using a substitution (`$@`). Links should be used inline in the sentence, rather than as parenthesised lists or appositions. | ||
| * When a message contains multiple links, construct a sentence that has the most variable link (that is, the link with most targets) last. For further information, see [Defining select statements](https://help.semmle.com/QL/learn-ql/ql/writing-queries/select-statement.html) | ||
|
|
||
| See the following pages for examples of alert messages: |
There was a problem hiding this comment.
Unfortunately they don't show the alert messages. The queries show the select statements if you expand them, which might be the closest thing, unless you point them at LGTM.com where they can see real alerts and the queries that generate them.
There was a problem hiding this comment.
Ok, perhaps I'll rephrase this to make it clear that these are links to pages where you can see query source files, which contain examples of select clauses/alert messages.
jf205
force-pushed
the
metadata-guide
branch
from
f2fafe8 to
a0d5049
Compare
|
@felicity-semmle now you have published the query help pages and the new Learning QL topics are on help.semmle.com the links in this document all work as intended. Any more comments before we merge? |
|
Thanks for the updates. No further review comments from me. @aschackmull and @geoffw0 - are there any other changes needed before this can be merged? |
|
I'm happy with it. |
Packaging: Normalize src/qlpack.yml
4 participants
This PR adds a query metadata style-guide to the repository. It's aim is to help both internal and external query contributors write useful metadata for their alert queries, which are consistent with the style we adopt for the 'built-in' queries. In theory, this is a sibling of the QL style guide, which gives stylistic advice on the QL section of query files.
@calumgrant has provided technical assistance, but it would really help if another language developer could review it.
@aschackmull - I see you have been contributing to the QL style guide, would you mind providing a technical review please?
@felicity-semmle please could you provide the docs team review.