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

AlertFiltering: add restrictAlertsToExactLocation #18603

Merged
merged 2 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 71 additions & 17 deletions shared/util/codeql/util/AlertFiltering.qll
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,92 @@
private import codeql.util.Location

/**
* Restricts alerts to a specific location in specific files.
* Holds if the query should produce alerts that match the given line ranges.
*
* If this predicate is empty, accept all alerts. Otherwise, accept alerts only at the specified
* locations. Note that alert restrictions apply only to the start line of an alert (even if the
* alert location spans multiple lines) because alerts are displayed on their start lines.
* This predicate is active if and only if it is nonempty. If this predicate is inactive, it has no
* effect. If it is active, it accepts any alert that has at least one matching location.
*
* - filePath: Absolute path of the file to restrict alerts to.
* - startLine: Start line number (starting with 1, inclusive) to restrict alerts to.
* - endLine: End line number (starting with 1, inclusive) to restrict alerts to.
* Note that an alert that is not accepted by this filtering predicate may still be included in the
* query results if it is accepted by another active filtering predicate in this module. An alert is
* excluded from the query results if only if (1) there is at least one active filtering predicate,
* and (2) it is not accepted by any active filtering predicate.
*
* If startLine and endLine are both 0, accept alerts anywhere in the file.
* An alert location is a match if it matches a row in this predicate. If `startLineStart` and
* `startLineEnd` are both 0, the row specifies a whole-file match, and a location is a match if
* its file path matches `filePath`. Otherwise, the row specifies a line-range match, and a
* location is a match if its file path matches `filePath`, and its start line is between
* `startLineStart` and `startLineEnd`, inclusive. (Note that only start line of the location is
* used for matching because an alert is displayed on the first line of its location.)
*
* A query should either completely ignore this predicate (i.e., perform no filtering whatsoever),
* or only return alerts that meet the filtering criteria as specified above.
* - filePath: alert location file path (absolute).
* - startLineStart: inclusive start of the range for alert location start line number (1-based).
* - startLineEnd: inclusive end of the range for alert location start line number (1-based).
*
* A query should either perform no alert filtering, or adhere to all the filtering rules in this
* module and return all and only the accepted alerts.
*
* This predicate is suitable for situations where we want to filter alerts at line granularity,
* such as based on the pull request diff.
*
* See also: `restrictAlertsToExactLocation`.
*/
extensible predicate restrictAlertsTo(string filePath, int startLineStart, int startLineEnd);

/**
* Holds if the query should produce alerts that match the given locations.
*
* This predicate is active if and only if it is nonempty. If this predicate is inactive, it has no
* effect. If it is active, it accepts any alert that has at least one matching location.
*
* Note that an alert that is not accepted by this filtering predicate may still be included in the
* query results if it is accepted by another active filtering predicate in this module. An alert is
* excluded from the query results if only if (1) there is at least one active filtering predicate,
* and (2) it is not accepted by any active filtering predicate.
Comment on lines +46 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

The duplication of these paragraphs between the two sets of QLDoc feels a bit awkward. Is there a way to have it appear only once, e.g. in a comment for the whole file?

If that is not possible, it might still be better to explicitly separate the general rules from the ones that are specific to the filtering predicate in question. As it is, each predicate has two general paragrams ("Note that an alert ..." as well as "A query should either perform ...") interlaced with specific information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main goal is to ensure that the qldoc for each filtering predicate provides all relevant information for the behavior and the use of that predicate, as I consider the predicate qldoc to be the only piece of documentation that everyone can be reasonably expected to read. That is why some of the same text appears in both qldocs.

When seeing both qldocs together in the source code, I understand that the duplication feels redundant and messy, especially when common information is interleaved with specific information. That said, qldoc can appear outside the context of the QL source code, and one qldoc may be shown without the other. I want to optimize for the ease of understanding for each qldoc by itself, even at the risk of being overly verbose.

Which is not to say that the qldocs are above criticism—I am sure there is a lot of room for improvement, and I am happy to hear your suggestions. Though I will also assess any suggestions using the criteria outlined above, unless someone makes a good case that those are not the goals to optimize for.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I write a long piece of duplicated QLDoc like that, I like to end it with "See also: [names of the predicates with similar QLDoc]". Then future maintainers are also alerted to the coupling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I write a long piece of duplicated QLDoc like that, I like to end it with "See also: [names of the predicates with similar QLDoc]". Then future maintainers are also alerted to the coupling.

Thanks. I updated the PR to add the cross references.

*
* An alert location is a match if it matches a row in this predicate. Each row specifies an exact
* location: an alert location is a match if its file path matches `filePath`, its start line and
* column match `startLine` and `startColumn`, and its end line and column match `endLine` and
* `endColumn`.
*
* - filePath: alert location file path (absolute).
* - startLine: alert location start line number (1-based).
* - startColumn: alert location start column number (1-based).
* - endLine: alert location end line number (1-based).
* - endColumn: alert location end column number (1-based).
*
* A query should either perform no alert filtering, or adhere to all the filtering rules in this
* module and return all and only the accepted alerts.
*
* This predicate is suitable for situations where we want to filter by the exact alert location,
* distinguishing between alerts on the same line.
*
* See also: `restrictAlertsTo`.
*/
extensible predicate restrictAlertsTo(string filePath, int startLine, int endLine);
extensible predicate restrictAlertsToExactLocation(
string filePath, int startLine, int startColumn, int endLine, int endColumn
);

/** Module for applying alert location filtering. */
module AlertFilteringImpl<LocationSig Location> {
/** Applies alert filtering to the given location. */
bindingset[location]
predicate filterByLocation(Location location) {
not restrictAlertsTo(_, _, _)
not restrictAlertsTo(_, _, _) and not restrictAlertsToExactLocation(_, _, _, _, _)
or
exists(string filePath, int startLine, int endLine |
restrictAlertsTo(filePath, startLine, endLine)
exists(string filePath, int startLineStart, int startLineEnd |
restrictAlertsTo(filePath, startLineStart, startLineEnd)
|
startLine = 0 and
endLine = 0 and
startLineStart = 0 and
startLineEnd = 0 and
location.hasLocationInfo(filePath, _, _, _, _)
or
location.hasLocationInfo(filePath, [startLine .. endLine], _, _, _)
location.hasLocationInfo(filePath, [startLineStart .. startLineEnd], _, _, _)
)
or
exists(string filePath, int startLine, int startColumn, int endLine, int endColumn |
restrictAlertsToExactLocation(filePath, startLine, startColumn, endLine, endColumn)
|
location.hasLocationInfo(filePath, startLine, startColumn, endLine, endColumn)
)
}
}
6 changes: 6 additions & 0 deletions shared/util/ext/default-alert-filter.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,9 @@ extensions:
extensible: restrictAlertsTo
# Empty predicate means no restrictions on alert locations
data: []

- addsTo:
pack: codeql/util
extensible: restrictAlertsToExactLocation
# Empty predicate means no restrictions on alert locations
data: []
Loading