Conversation
We should be using all subtypes of `FieldBase`. This allows us to find more type expressions, and is also simpler to evaluate.
f94f6aa to
13675c4
Compare
|
DCA results look fine. No significant change to performance. I don't know why one of the aws-sdk-go projects has a speedup and one has a slowdown. I don't think it is significant. Note that these tweaks will have more effect when we expand overlay annotations. |
There was a problem hiding this comment.
Pull request overview
This PR applies small Go QL library/query tweaks intended to improve evaluation performance and make type-expression detection more robust when type information is incomplete.
Changes:
- Introduce and adopt
exprRefersToNil(Expr)as a centralized way to recognizenilreferences across queries/libs. - Add optimizer hints (
pragma[nomagic],pragma[inline]) and small refactorings to simplify evaluation. - Expand
isTypeExprTopDown’s coverage by considering allFieldBasetype expressions (instead of specific subclasses).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| go/ql/src/experimental/CWE-203/Timing.ql | Switch nil-comparison exclusion to use exprRefersToNil. |
| go/ql/src/RedundantCode/UnreachableStatement.ql | Use exprRefersToNil when allowlisting constant return values. |
| go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll | Use exprRefersToNil in constant-guard reasoning for nil returns. |
| go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll | Use exprRefersToNil in return-value nil/non-nil reasoning. |
| go/ql/lib/semmle/go/dataflow/Properties.qll | Use exprRefersToNil for IsNil property checks. |
| go/ql/lib/semmle/go/Expr.qll | Add exprRefersToNil, add optimizer pragmas, and update/improve isTypeExprTopDown comment + logic. |
| go/ql/lib/semmle/go/Decls.qll | Add optimizer pragmas around FieldBase/FieldDecl definitions used by type-expression detection. |
jbj
left a comment
There was a problem hiding this comment.
This looks fine, but I'm guessing you want a DCA run that shows a performance improvement before taking on the added complexity that comes from some of these commits.
The only functional change is that
isTypeExprTopDownnow finds more type expressions.FieldBaseis the class used for all places where we extract something of the form<type> <name>. For example: a fields in a struct, a function parameter, or a method in an interface declaration. In all cases we know that one of the sub-expressions represents a type, so we want to include it inisTypeExprTopDown. Previously we had been individually listing some subclasses, which (a) meant that we were missing some type expressions and (b) it actually took slightly longer to evaluate. This should only make a difference when type information is incomplete - for example, when some dependencies could not be reached during extraction.I do not think that this needs a change note. We will still have degraded analysis when type information is incomplete.
The other changes are either hints to the compiler (like
pragma[nomagic]) or small refactorings that make optimization simpler during evaluation (e.g. we know that anyExprthat refers tonilwill be aConstantName, and specifying that in the code helps the optimizer).