Skip to content

Python: Use _one_ parameter position for normal parameters #12468

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.

Sign up for GitHub

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

Draft
RasmusWL wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Python: Use _one_ parameter position for normal parameters #12468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c0f50b
Python: Use _one_ parameter position for normal parameters
RasmusWL Mar 9, 2023
b1f0486
Python: Accept new parameter position names
RasmusWL Mar 13, 2023
330ef2c
Merge branch 'main' into one-parameter-position
RasmusWL Mar 13, 2023
ebeb123
Python: Autoformat
RasmusWL Mar 13, 2023
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
63 changes: 48 additions & 15 deletions python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,27 @@ private import semmle.python.internal.CachedStages
newtype TParameterPosition =
/** Used for `self` in methods, and `cls` in classmethods. */
TSelfParameterPosition() or
TPositionalParameterPosition(int index) {
index = any(Parameter p).getPosition()
TNormalParameterPosition(int index, string name) {
exists(Function f, Parameter p, int indexOffset |
p = f.getArg(index + indexOffset) and
p = f.getArgByName(name) and
indexOffset = any(DataFlowFunction dff | dff.getScope() = f).positionalOffset() and
// for things that will be considered a self parameter, we do not need to create a
// parameter position
index >= 0
)
} or
Comment on lines +45 to +54
Copy link
Member Author

@RasmusWL RasmusWL Mar 16, 2023

Choose a reason for hiding this comment

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

Maybe I should have explained better that for the method definition below, we need to take care for the self parameter, so want to have TNormalParameterPosition(0, "arg").

class Foo:
    def meth(self, arg): pass

TPositionalOnlyParameterPosition(int index) {
index = any(Parameter p).getPosition() and
// TODO: implement this when we have proper support for positional only
none()
or
// since synthetic parameters are made for a synthetic summary callable, based on
// what Argument positions they have flow for, we need to make sure we have such
// parameter positions available.
FlowSummaryImplSpecific::ParsePositions::isParsedPositionalArgumentPosition(_, index)
} or
TKeywordParameterPosition(string name) {
TKeywordOnlyParameterPosition(string name) {
name = any(Parameter p).getName()
or
// see comment for TPositionalParameterPosition
Expand All @@ -71,11 +83,14 @@ class ParameterPosition extends TParameterPosition {
/** Holds if this position represents a `self`/`cls` parameter. */
predicate isSelf() { this = TSelfParameterPosition() }

/** Holds if this position represents a positional parameter at (0-based) `index`. */
predicate isPositional(int index) { this = TPositionalParameterPosition(index) }
/** Holds if this position represents a normal parameter at (0-based) `index` with name `name`. */
predicate isNormal(int index, string name) { this = TNormalParameterPosition(index, name) }

/** Holds if this position represents a positional only parameter at (0-based) `index`. */
predicate isPositionalOnly(int index) { this = TPositionalOnlyParameterPosition(index) }

/** Holds if this position represents a keyword parameter named `name`. */
predicate isKeyword(string name) { this = TKeywordParameterPosition(name) }
/** Holds if this position represents a keyword only parameter named `name`. */
predicate isKeywordOnly(string name) { this = TKeywordOnlyParameterPosition(name) }

/** Holds if this position represents a `*args` parameter at (0-based) `index`. */
predicate isStarArgs(int index) { this = TStarArgsParameterPosition(index) }
Expand All @@ -96,9 +111,13 @@ class ParameterPosition extends TParameterPosition {
string toString() {
this.isSelf() and result = "self"
or
exists(int index | this.isPositional(index) and result = "position " + index)
exists(int index, string name |
this.isNormal(index, name) and result = "positional " + index + " or keyword " + name
)
or
exists(string name | this.isKeyword(name) and result = "keyword " + name)
exists(int index | this.isPositionalOnly(index) and result = "positional only " + index)
or
exists(string name | this.isKeywordOnly(name) and result = "keyword only " + name)
or
exists(int index | this.isStarArgs(index) and result = "*args at " + index)
or
Expand Down Expand Up @@ -168,9 +187,11 @@ class ArgumentPosition extends TArgumentPosition {
predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
ppos.isSelf() and apos.isSelf()
or
exists(int index | ppos.isPositional(index) and apos.isPositional(index))
exists(int index | apos.isPositional(index) |
ppos.isNormal(index, _) or ppos.isPositionalOnly(index)
)
or
exists(string name | ppos.isKeyword(name) and apos.isKeyword(name))
exists(string name | apos.isKeyword(name) | ppos.isNormal(_, name) or ppos.isKeywordOnly(name))
or
exists(int index | ppos.isStarArgs(index) and apos.isStarArgs(index))
or
Expand Down Expand Up @@ -297,11 +318,23 @@ abstract class DataFlowFunction extends DataFlowCallable, TFunction {
int positionalOffset() { result = 0 }

override ParameterNode getParameter(ParameterPosition ppos) {
exists(int index | ppos.isPositional(index) |
result.getParameter() = func.getArg(index + this.positionalOffset())
// normal
exists(int index, string name | ppos.isNormal(index, name) |
result.getParameter() = func.getArg(index + this.positionalOffset()) and
func.getArg(index + this.positionalOffset()) = func.getArgByName(name)
)
or
exists(string name | ppos.isKeyword(name) | result.getParameter() = func.getArgByName(name))
// positioanl only
exists(int index | ppos.isPositionalOnly(index) |
result.getParameter() = func.getArg(index + this.positionalOffset()) and
not func.getArg(index + this.positionalOffset()) = func.getArgByName(_)
)
or
// keyword only
exists(string name | ppos.isKeywordOnly(name) |
result.getParameter() = func.getArgByName(name) and
not func.getArgByName(name) = func.getArg(_)
)
or
// `*args`
exists(int index |
Expand Down Expand Up @@ -1467,7 +1500,7 @@ class SummaryParameterNode extends ParameterNodeImpl, TSummaryParameterNode {
pos.isDictSplat() and
exists(ParameterPosition keywordPos |
FlowSummaryImpl::Private::summaryParameterNodeRange(sc, keywordPos) and
keywordPos.isKeyword(_)
(keywordPos.isNormal(_, _) or keywordPos.isKeywordOnly(_))
)
)
}
Expand Down
12 changes: 10 additions & 2 deletions python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,11 @@ private predicate dictSplatParameterNodeClearStep(ParameterNode n, DictionaryEle
n = TSummaryParameterNode(callable.asLibraryCallable(), dictSplatPos)
) and
exists(callable.getParameter(keywordPos)) and
keywordPos.isKeyword(c.getKey())
(
keywordPos.isNormal(_, c.getKey())
or
keywordPos.isKeywordOnly(c.getKey())
)
)
}

Expand Down Expand Up @@ -268,7 +272,11 @@ predicate synthDictSplatParameterNodeReadStep(
exists(DataFlowCallable callable, ParameterPosition ppos |
nodeFrom = TSynthDictSplatParameterNode(callable) and
nodeTo = callable.getParameter(ppos) and
ppos.isKeyword(c.getKey())
(
ppos.isNormal(_, c.getKey())
or
ppos.isKeywordOnly(c.getKey())
)
)
}

Expand Down
4 changes: 3 additions & 1 deletion python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ newtype TNode =
TSynthDictSplatArgumentNode(CallNode call) { exists(call.getArgByName(_)) } or
/** A synthetic node to allow flow to keyword parameters from a `**kwargs` argument. */
TSynthDictSplatParameterNode(DataFlowCallable callable) {
exists(ParameterPosition ppos | ppos.isKeyword(_) | exists(callable.getParameter(ppos)))
exists(ParameterPosition ppos | ppos.isNormal(_, _) or ppos.isKeywordOnly(_) |
exists(callable.getParameter(ppos))
)
}

/** Helper for `Node::getEnclosingCallable`. */
Expand Down
16 changes: 12 additions & 4 deletions python/ql/lib/semmle/python/dataflow/new/internal/FlowSummaryImplSpecific.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,20 @@ string getParameterPosition(ParameterPosition pos) {
pos.isSelf() and result = "self"
or
exists(int i |
pos.isPositional(i) and
(
pos.isNormal(i, _)
or
pos.isPositionalOnly(i)
) and
result = i.toString()
)
or
exists(string name |
pos.isKeyword(name) and
(
pos.isNormal(_, name)
or
pos.isKeywordOnly(name)
) and
result = name + ":"
)
}
Expand Down Expand Up @@ -262,12 +270,12 @@ ArgumentPosition parseParamBody(string s) {
ParameterPosition parseArgBody(string s) {
exists(int i |
ParsePositions::isParsedPositionalArgumentPosition(s, i) and
result.isPositional(i)
result.isPositionalOnly(i)
)
or
exists(string name |
ParsePositions::isParsedKeywordArgumentPosition(s, name) and
result.isKeyword(name)
result.isKeywordOnly(name)
)
or
s = "self" and
Expand Down
16 changes: 0 additions & 16 deletions python/ql/test/experimental/dataflow/TestUtil/DataFlowConsistency.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,4 @@ private class MyConsistencyConfiguration extends ConsistencyConfiguration {
pos.isDictSplat() and
not exists(p.getLocation().getFile().getRelativePath())
}

override predicate uniqueParameterNodePositionExclude(
DataFlowCallable c, ParameterPosition pos, Node p
) {
// For normal parameters that can both be passed as positional arguments or keyword
// arguments, we currently have parameter positions for both cases..
//
// TODO: Figure out how bad breaking this consistency check is
exists(Function func, Parameter param |
c.getScope() = func and
p = parameterNode(param) and
c.getParameter(pos) = p and
param = func.getArg(_) and
param = func.getArgByName(_)
)
}
}
2 changes: 1 addition & 1 deletion python/ql/test/experimental/dataflow/basic/callGraphSinks.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| file://:0:0:0:0 | parameter position 0 of builtins.reversed |
| file://:0:0:0:0 | parameter positional only 0 of builtins.reversed |
| test.py:1:1:1:21 | SynthDictSplatParameterNode |
| test.py:1:19:1:19 | ControlFlowNode for x |
| test.py:7:5:7:20 | ControlFlowNode for obfuscated_id() |
2 changes: 1 addition & 1 deletion python/ql/test/experimental/dataflow/basic/global.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:1:5:1:17 | GSSA Variable obfuscated_id |
| test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:7:5:7:17 | ControlFlowNode for obfuscated_id |
| test.py:1:5:1:17 | GSSA Variable obfuscated_id | test.py:7:5:7:17 | ControlFlowNode for obfuscated_id |
Expand Down
2 changes: 1 addition & 1 deletion python/ql/test/experimental/dataflow/basic/globalStep.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:1:5:1:17 | GSSA Variable obfuscated_id |
| test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:1:5:1:17 | GSSA Variable obfuscated_id |
| test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:7:5:7:17 | ControlFlowNode for obfuscated_id |
Expand Down
6 changes: 3 additions & 3 deletions python/ql/test/experimental/dataflow/basic/local.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | [summary] to write: return (return) in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return) in builtins.reversed |
| file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | parameter position 0 of builtins.reversed | file://:0:0:0:0 | parameter position 0 of builtins.reversed |
| file://:0:0:0:0 | parameter positional only 0 of builtins.reversed | file://:0:0:0:0 | parameter positional only 0 of builtins.reversed |
| test.py:0:0:0:0 | GSSA Variable __name__ | test.py:0:0:0:0 | GSSA Variable __name__ |
| test.py:0:0:0:0 | GSSA Variable __package__ | test.py:0:0:0:0 | GSSA Variable __package__ |
| test.py:0:0:0:0 | GSSA Variable b | test.py:0:0:0:0 | GSSA Variable b |
Expand Down
2 changes: 1 addition & 1 deletion python/ql/test/experimental/dataflow/basic/localStep.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:1:5:1:17 | GSSA Variable obfuscated_id |
| test.py:1:5:1:17 | GSSA Variable obfuscated_id | test.py:7:5:7:17 | ControlFlowNode for obfuscated_id |
| test.py:1:19:1:19 | ControlFlowNode for x | test.py:1:19:1:19 | SSA variable x |
Expand Down
4 changes: 2 additions & 2 deletions python/ql/test/experimental/dataflow/basic/sinks.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed |
| file://:0:0:0:0 | [summary] to write: return (return) in builtins.reversed |
| file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | parameter position 0 of builtins.reversed |
| file://:0:0:0:0 | parameter positional only 0 of builtins.reversed |
| test.py:0:0:0:0 | GSSA Variable __name__ |
| test.py:0:0:0:0 | GSSA Variable __package__ |
| test.py:0:0:0:0 | GSSA Variable b |
Expand Down
4 changes: 2 additions & 2 deletions python/ql/test/experimental/dataflow/basic/sources.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed |
| file://:0:0:0:0 | [summary] to write: return (return) in builtins.reversed |
| file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | parameter position 0 of builtins.reversed |
| file://:0:0:0:0 | parameter positional only 0 of builtins.reversed |
| test.py:0:0:0:0 | GSSA Variable __name__ |
| test.py:0:0:0:0 | GSSA Variable __package__ |
| test.py:0:0:0:0 | GSSA Variable b |
Expand Down
2 changes: 1 addition & 1 deletion python/ql/test/experimental/dataflow/tainttracking/basic/LocalTaintStep.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| file://:0:0:0:0 | [summary] read: argument position 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| file://:0:0:0:0 | [summary] read: argument positional only 0.List element in builtins.reversed | file://:0:0:0:0 | [summary] to write: return (return).List element in builtins.reversed |
| test.py:3:1:3:7 | GSSA Variable tainted | test.py:4:6:4:12 | ControlFlowNode for tainted |
| test.py:3:11:3:16 | ControlFlowNode for SOURCE | test.py:3:1:3:7 | GSSA Variable tainted |
| test.py:6:1:6:11 | ControlFlowNode for FunctionExpr | test.py:6:5:6:8 | GSSA Variable func |
Expand Down