github
Enable warnings for undocumented QL entities
Is your feature request related to a problem? Please describe.
The CodeQL language teams track their QLDoc coverage and maintain it at a high level. We have CI checks on github/codeql that flag public QL APIs without QLDoc comments, to prevent coverage regressions. @yoff suggested it would be easier to see this feedback even earlier, in the editor, when writing a new part of the library.
Describe the solution you'd like The editor shows warnings when a public CodeQL predicate, class, or module does not have a QLDoc comment. Consider having a user setting to toggle this feature, since it might be too noisy for all users.
Describe alternatives you've considered CI currently runs an internal test that builds all the QLDoc HTML and examines the textual differences using a shell script.
Additional context This can be done through language server/compiler changes, but would become more involved if we wanted the ability to toggle it. We would also need to correctly identify which APIs are public, or overapproximate.
Note that the current rules rely hardcoding special names. We may need special features in qlpacks to state that a directory doesn't need qldoc or we could take the existing name based rules and bake them into the compiler.
Finally the warning is global so it depends on which set of files we are checking. For example with the following
internal.qll
====
predicate foo() {
any()
}
====
public.qll
import internal
With our current strategy when checking internal.qll it looks fine as foo is only available from a internal library and hence we would ignore it. However when in the context of public.qll it is actually exported and there for need docuementing. To solve this we either need to come up with a more local set of rules (e.g. require documenting any entity visible outside of its declaring file) or change the implementation of checking to be global which will slow down everything. However the approximation may be good enough for most cases if we are willing to have false negatives. Another option would be to put the warning at the export site, but that is very hard to work out (especially with recursive imports).
It might be fine to simply have an opt-in warning for every non-private entity in any file. I think we would like to document most of those anyway.
As a different opt-in mechanism, we could have a command to run rather than a setting. Either would work fine for a user examining their code before submission.
As a different opt-in mechanism, we could have a command to run rather than a setting. Either would work fine for a user examining their code before submission.
Yes, that would work fine for me 👍