Skip to content

JS: fix most ql-for-ql warnings #7984

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

Merged
merged 1 commit into from
Feb 21, 2022
Merged

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Feb 11, 2022

Fixes 40 QL-for-QL warnings.
Mostly benign qldoc changes.

But there was a real bug in UnsafeCodeConstructionCustomizations.qll 😱
(The evaluation is on that query).

Evaluation looks fine.
(There are some new results in some minified files that are hard to triage).

@github-actions github-actions bot added the JS label Feb 11, 2022
@erik-krogh erik-krogh marked this pull request as ready for review February 11, 2022 17:10
@erik-krogh erik-krogh requested a review from a team February 11, 2022 17:10
@erik-krogh erik-krogh requested a review from a team as a code owner February 11, 2022 17:10
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Feb 11, 2022
Copy link

@annarailton annarailton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with the doc changes to the one CodeML file javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/BaseScoring.qll

Copy link

@kaeluka kaeluka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Glad to see that documentation is coming out looking very consistent 👍
I added one observation — but I think that's mostly about the analysis, rather than this specific PR.

@@ -635,39 +635,18 @@ module TaintTracking {
pred.asExpr() = succ.getAstNode().(MethodCallExpr).getReceiver() and
(
// sorted, interesting, properties of String.prototype
name = "anchor" or
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for really large sets, readability suffers — like in this case. I like the consistency, though. But maybe the real issue here is the auto-formatter that won't allow having one line per item for large sets?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+💯

One workaround we commonly is to put an empty line comment at the end of each item

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, thanks, I'll remember that trick :)

@erik-krogh erik-krogh merged commit 5f9bd7a into github:main Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants