Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

QL: Add query detecting suspiciously missing parameters from the QLDoc of a predicate #7450

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

QL: Add query detecting suspiciously missing parameters from the QLDoc of a predicate #7450

wants to merge 14 commits into from

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Dec 18, 2021 •
edited

I added a QL-for-QL query that flags suspicious QLDoc for a predicate when:

  • At least one parameter is documented in the QLDoc.
  • At least one paramter is not documented in the QLDoc.
  • The QLDoc has a codeblock (enclosed in backticks) mentioning something that looks like an identifier and is not a parameter.

This turns out to be good at identifying lots of QLDocs where the parameters mentioned in the qldoc do not match up with the actual parameter names.

I fixed most of the issues identified in the javascript/ folder. The remaining issues in javascript/ seem benign.
There are still lots of issues for the other languages. I have not tried to fix those.
@erik-krogh erik-krogh force-pushed the missDocParam branch 2 times, most recently from 13055ef to cb0a1c3 Dec 19, 2021
@erik-krogh erik-krogh force-pushed the missDocParam branch 3 times, most recently from 02958e6 to 55cdef4 Dec 20, 2021
@erik-krogh erik-krogh marked this pull request as ready for review Dec 20, 2021
@erik-krogh erik-krogh requested review from tausbn and as code owners Dec 20, 2021
@esbena
Copy link
Contributor

@esbena esbena commented Dec 20, 2021

Could this be tuned to handle the case where all parameters have been renamed? That case can not be caught currently due to the first of your requirements. (I'm particularly interested in the /1 case)

@erik-krogh
Copy link
Contributor Author

@erik-krogh erik-krogh commented Dec 20, 2021 •
edited

Could this be tuned to handle the case where all parameters have been renamed? That case can not be caught currently due to the first of your requirements. (I'm particularly interested in the /1 case)

I thought it would be bad, but it worked great 👍 (See the extra JS fixes).

I also found a few more tweaks to reduce the FP rate.
There are also more benign cases, but I think the precision is OK.

Edit: And I did some drive-by patching of explicit-this and redundant-cast, to further reduce the number of results in javascript/.

tausbn
tausbn requested changes Jan 3, 2022
View changes
Copy link
Contributor

@tausbn tausbn left a comment

Overall this looks okay to me, but it's still fairly noisy. One common source of noise is when the QLDoc mentions language-specific syntax (like null, break, or return).

I don't know if there's a way to annotate inline code snippets with the desired highlighting language. Some MarkDown flavours support this, but it's not clear to me that the one in VSCode does.

Either way, I think we need to cut down on the number of false positives before merging this.

javascript/ql/examples/queries/dataflow/TemplateInjection/TemplateInjection.ql Outdated Show resolved Hide resolved
ql/ql/src/queries/style/MissingParameterInQlDoc.ql Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tausbn tausbn

@github/codeql-javascript codeql-javascript

@github/codeql-ruby codeql-ruby

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
JS no-change-note-required Python QL-for-QL Ruby
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
3 participants
@erik-krogh @esbena @tausbn