Fix syntax errors in QL comments #8952

cklin · 2022-04-28T18:58:36Z

This PR fixes QL comments used to generate QL documentation. They are mostly about back-quoting angle brackets so that they are property escaped in the HTML output.

aschackmull · 2022-04-29T06:53:25Z

javascript/ql/lib/semmle/javascript/Externs.qll

@@ -53,7 +53,7 @@ import javascript
 *
 * /**
 *  * @param {!Object} obj
- *  * @return {!Array<string>}
+ *  * @return {!Array&lt;string&gt;}


These are wrapped in <pre>...</pre>, so are these changes necessary? Changing < to < hurts readability.

Reserved characters such as < and > still needs to be escaped using their HTML entity. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/pre

You can see that @return {!Array<string>} currently shows up as @return {!Array} because the browser treats <string> as an unknown element.
https://codeql.github.com/codeql-standard-libraries/javascript/semmle/javascript/Externs.qll/type.Externs$ExternalDecl.html

Could we replace the <pre> with triple backquotes instead?

You could, but the */ in the next line would stop working.

See - I have no idea what entity number 47 is - I had to look that up, that's pretty poor readability. Luckily the obvious answer is just to replace / with /.

Hmm, right, I see why the entity is used for /. But the <pre> isn't really a good solution - it doesn't appear to render correctly in VSCode (on predicate mouse-over).

aschackmull · 2022-04-29T06:57:57Z

Most of the changes look fine, so mostly no objections from me. Except for the <-to-<-rewrite, which I think hurts readability. I also wonder why these changes are even necessary - our qldoc is mostly markdown, so would it instead make sense to simply remove html support in our qldoc? (I know that's likely a much larger change, but still worth a thought.) Then a translation into html could simply do whatever escaping was needed without qldoc writers having to worry about accidental html interpretations of angle brackets.

hvitved · 2022-04-29T07:36:32Z

csharp/ql/lib/semmle/code/csharp/exprs/Call.qll

-   * - Line 16: There is no static target (delegate call) but the delegate `i => { }` (line
-   *   20) is a run-time target.
+   * - Line 16: There is no static target (delegate call) but the delegate `i => { }`
+   *   (line 20) is a run-time target.


I don't mind this change, but hopefully it is not needed to generete proper QL doc?

It actually is needed. Here is what the rendered documentation currently looks like, without this fix:

https://codeql.github.com/codeql-standard-libraries/csharp/semmle/code/csharp/exprs/Call.qll/predicate.Call$Call$getARuntimeTarget.0.html

Any explanation why it renders that way? Is it a rendering bug?

I think that is because 20) is interpreted as a numbered list item starting at number 20.

Ah, that makes a bit more sense then. A markdown gotcha.

hvitved · 2022-04-29T07:39:21Z

csharp/ql/src/Documentation/XmldocExtraTypeParam.ql

@@ -1,6 +1,6 @@
 /**
 * @name Incorrect type parameter name in documentation
- * @description The type parameter name given in a '<typeparam>' tag does not exist. Rename the parameter or
+ * @description The type parameter name given in a `<typeparam>` tag does not exist. Rename the parameter or


If this is the new way of doing things, we should update the comment Any code elements included in the description should be enclosed in single quotes (https://github.com/github/codeql/blob/main/docs/query-metadata-style-guide.md?plain=1#L63), as well as the example.

I should clarify that this is not a style change but a rendering fix. Here is what the documentation currently looks like:

https://codeql.github.com/codeql-standard-libraries/csharp/Documentation/XmldocExtraTypeParam.ql/module.XmldocExtraTypeParam.html

QL comments can include Markdown and HTML, so the < and > characters need to be escaped, either by putting them in Markdown code format (the approach I took here) or by replacing them with < and >. I can switch it to the second approach if that is more style-compliant.

I agree that using Markdown backquotes for code elements seems sensible, but that should probably be a separate discussion.

this is not a style change but a rendering fix

No, it's both a style change and a rendering fix.

MathiasVP

C/C++ 👍

aschackmull · 2022-05-02T07:17:21Z

I think it is a problem that we allow html in our qldoc, we should restrict ourselves to markdown and then we wouldn't need such readability-impeding changes like writing <. Could we just get rid of html in our qldoc? Then any html rendering could just be plain markdown-to-html.

cklin · 2022-05-02T16:53:56Z

I intended this PR to be a quick low-effort fix for QL documentation that is already broken. Unfortunately, I really do not have more time to devote to this PR or the issue of broken documentation, so I will just have to leave this PR as-is.

I am sure that you all as the owners of the documentation will be able to resolve the open questions and find the best way to address both the immediate and the long-term issues. Thank you! 🙏

erik-krogh

JS 👍

The change in Externs.qll makes it slightly harder to read the source code.
But it seems like the best solution, because the rendered documentation should definitely render correctly.

aschackmull

As commented above.

aschackmull

The change in javascript/ql/lib/semmle/javascript/Externs.qll is unfortunate and makes me want to abolish html entirely from our qldoc, but currently I can't think of a way to render */ inside a triple-backtick code block. Note that the <pre> block doesn't render correctly in VSCode, so going for a markdown solution would be preferable, but I'll revert my position on whether just to merge this PR for now.

Fix syntax errors in QL comments

d6f0bbb

cklin requested review from a team as code owners April 28, 2022 18:58

github-actions bot added C# C++ Java JS labels Apr 28, 2022

cklin added the no-change-note-required This PR does not need a change note label Apr 28, 2022

aschackmull reviewed Apr 29, 2022

View reviewed changes

hvitved reviewed Apr 29, 2022

View reviewed changes

MathiasVP approved these changes Apr 29, 2022

View reviewed changes

erik-krogh approved these changes May 2, 2022

View reviewed changes

aschackmull requested changes May 3, 2022

View reviewed changes

aschackmull approved these changes May 3, 2022

View reviewed changes

aschackmull merged commit 249f771 into github:main May 3, 2022

cklin deleted the fix-ql-comments-syntax branch August 16, 2022 15:45

Fix syntax errors in QL comments #8952

Fix syntax errors in QL comments #8952

Uh oh!

Conversation

cklin commented Apr 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull commented Apr 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

aschackmull commented May 2, 2022

Uh oh!

cklin commented May 2, 2022

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!