Python: modernise variables queries#2540
Conversation
RasmusWL
force-pushed
the
python-modernise-variables-queries
branch
from
0908e4e to
3908994
Compare
tausbn
left a comment
tausbn
left a comment
There was a problem hiding this comment.
A large variety of comments, but otherwise looking good!
| // Escapes if used out side of for loop or is a lambda in a comprehension | ||
| ( | ||
| exists(Expr e, For forloop | forloop = loop and e.refersTo(_, _, capturing) | not forloop.contains(e)) | ||
| exists(Expr e, For forloop | forloop = loop and e.pointsTo(_, _, capturing) | not forloop.contains(e)) |
There was a problem hiding this comment.
I wonder if forloop = loop is a holdover from back when there was no syntax for casting. I think writing loop.(For).contains(e) and eliminating the For forloop part of the exists would be better.
| exists(Object::builtin(l.getId())) | ||
| exists(Builtin::builtin(l.getId())) | ||
| ) and | ||
| scope instanceof Function and |
There was a problem hiding this comment.
Perhaps we could simply change Scope to Function in the definition of the scope parameter?
There was a problem hiding this comment.
I'm starting to wonder if the restriction of scope to Function makes sense?
Looking at the original code
predicate shadows(Name d, string name, Scope scope, int line) {
exists(LocalVariable l | d.defines(l) and scope instanceof Function and
l.getId() = name and
exists(Object::builtin(l.getId()))
) and
d.getScope() = scope and
d.getLocation().getStartLine() = line and
not white_list(name) and
not optimizing_parameter(d)
}
it might simply have been an oversigt? What do you think? (that is, do we not care about shadowing a builting when you are making a global variable?)
|
|
||
| predicate shadows(Name d, GlobalVariable g, Scope scope, int line) { | ||
| exists(LocalVariable l | d.defines(l) and l.getId() = g.getId() and | ||
| scope instanceof Function and g.getScope() = scope.getScope() and |
There was a problem hiding this comment.
Again, scope instanceof Function can be moved to the scope parameter declaration, methinks.
| d.defines(l) and | ||
| l.getId() = g.getId() and | ||
| scope instanceof Function and | ||
| g.getScope() = scope.getScope() and |
There was a problem hiding this comment.
This line and the two that follow should probably be moved outside the exists (since they don't relate to the bound variable l.)
| f.refersTo(call) and | ||
| call.(CallNode).getFunction().refersTo(range) | ||
| f.pointsTo(call) and | ||
| call.getACall().getFunction().pointsTo(range) |
There was a problem hiding this comment.
This seems a bit overwrought. Would f = range.getACall() not suffice?
There was a problem hiding this comment.
Yes, I found it very strange and just kept it as is. tests passes with f = range.getACall() so that's very good!
| exists(Module m | | ||
| m = this.getSourceModule() | | ||
| not exists(Call modify, Attribute attr, GlobalVariable all | | ||
| modify.getScope() = m and modify.getFunc() = attr and |
There was a problem hiding this comment.
Maybe just do modify.getScope() = this.getSourceModule() and then you don't need the existentially quantified variable m at all!
| exists(ModuleValue imported | | ||
| imported.importedAs(imp.getImportedModuleName()) | | ||
| not imported.exportsComplete() | ||
| not imported.hasCompleteExportInfo() |
There was a problem hiding this comment.
You can combine these two exists and push the disjunction inside. Also, wouldn't imported.isAbsent() imply not imported.hasCompleteExportInfo() anyway?
+ moved `scope instanceof Function` so it makes more sense :)
|
I realized there were quite a few files in Tests were failing since the I have not forgotten about all your suggestions, will fix them 👍 |
RasmusWL
force-pushed
the
python-modernise-variables-queries
branch
from
3908994 to
b8a9a35
Compare
Original code:
exists(Expr e, For forloop | forloop = loop and e.pointsTo(_, _, capturing) |
not loop.contains(e)
)
The new version will preserve the same semantics. The problem with the first
rewrite was that `not loop.(For).somethingMore` would hold for any AstNode that
was not a For
2 participants
Some of these changes would only fix 1 line, so I have bundled them all together.
Wanted to make a draft PR so tests can run to verify I didn't miss something 😉