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.

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

Ruby: Rework (hash) splat argument/parameter matching #17222

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Ruby: Rework (hash) splat argument/parameter matching #17222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6d4f3bd
Ruby: Rework splat argument/parameter matching
hvitved Aug 14, 2024
20dc242
Ruby: Rework hash splat argument/parameter matching
hvitved Aug 14, 2024
c4b0f81
Ruby: Prevent positional matching when preceded by a splat
hvitved Aug 20, 2024
d15e1b5
Ruby: Prevent synthetic splat matching for actual splats at same posi…
hvitved Aug 20, 2024
b0003c0
Ruby: Remove two redundant checks
hvitved Aug 21, 2024
cb1b1da
Ruby: Add another array flow test
hvitved Aug 21, 2024
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
62 changes: 50 additions & 12 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,11 @@ private module Cached {
THashSplatArgumentPosition() or
TSynthHashSplatArgumentPosition() or
TSplatArgumentPosition(int pos) { exists(Call c | c.getArgument(pos) instanceof SplatExpr) } or
TSynthSplatArgumentPosition() or
TSynthSplatArgumentPosition(int actualSplatPos) {
actualSplatPos = -1 // represents no actual splat
or
exists(Call c | c.getArgument(actualSplatPos) instanceof SplatExpr)
} or
TAnyArgumentPosition() or
TAnyKeywordArgumentPosition()

Expand All @@ -590,11 +594,15 @@ private module Cached {
THashSplatParameterPosition() or
TSynthHashSplatParameterPosition() or
TSplatParameterPosition(int pos) {
pos = 0
pos = 0 // needed for flow summaries
or
exists(Parameter p | p.getPosition() = pos and p instanceof SplatParameter)
} or
TSynthSplatParameterPosition() or
TSynthSplatParameterPosition(int actualSplatPos) {
actualSplatPos = -1 // represents no actual splat
or
exists(Callable c | c.getParameter(actualSplatPos) instanceof SplatParameter)
} or
TAnyParameterPosition() or
TAnyKeywordParameterPosition()
}
Expand Down Expand Up @@ -1383,8 +1391,14 @@ class ParameterPosition extends TParameterPosition {
/** Holds if this position represents a splat parameter at position `n`. */
predicate isSplat(int n) { this = TSplatParameterPosition(n) }

/** Holds if this position represents a synthetic splat parameter. */
predicate isSynthSplat() { this = TSynthSplatParameterPosition() }
/**
* Holds if this position represents a synthetic splat parameter.
*
* `actualSplatPos` indicates the position of the (unique) actual splat
* parameter belonging to the same method, with `-1` representing no actual
* splat parameter.
*/
predicate isSynthSplat(int actualSplatPos) { this = TSynthSplatParameterPosition(actualSplatPos) }

/**
* Holds if this position represents any parameter, except `self` parameters. This
Expand Down Expand Up @@ -1419,7 +1433,11 @@ class ParameterPosition extends TParameterPosition {
or
exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
or
this.isSynthSplat() and result = "synthetic *"
exists(int actualSplatPos, string suffix |
this.isSynthSplat(actualSplatPos) and
result = "synthetic *" + suffix and
if actualSplatPos = -1 then suffix = "" else suffix = " (actual at " + actualSplatPos + ")"
)
}
}

Expand Down Expand Up @@ -1458,8 +1476,14 @@ class ArgumentPosition extends TArgumentPosition {
/** Holds if this position represents a splat argument at position `n`. */
predicate isSplat(int n) { this = TSplatArgumentPosition(n) }

/** Holds if this position represents a synthetic splat argument. */
predicate isSynthSplat() { this = TSynthSplatArgumentPosition() }
/**
* Holds if this position represents a synthetic splat argument.
*
* `actualSplatPos` indicates the position of the (unique) actual splat
* argument belonging to the same call, with `-1` representing no actual
* splat argument.
*/
predicate isSynthSplat(int actualSplatPos) { this = TSynthSplatArgumentPosition(actualSplatPos) }

/** Gets a textual representation of this position. */
string toString() {
Expand All @@ -1483,7 +1507,11 @@ class ArgumentPosition extends TArgumentPosition {
or
exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
or
this.isSynthSplat() and result = "synthetic *"
exists(int actualSplatPos, string suffix |
this.isSynthSplat(actualSplatPos) and
result = "synthetic *" + suffix and
if actualSplatPos = -1 then suffix = "" else suffix = " (actual at " + actualSplatPos + ")"
)
}
}

Expand Down Expand Up @@ -1517,19 +1545,29 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
exists(string name | ppos.isKeyword(name) and apos.isKeyword(name))
or
(ppos.isHashSplat() or ppos.isSynthHashSplat()) and
(apos.isHashSplat() or apos.isSynthHashSplat())
(apos.isHashSplat() or apos.isSynthHashSplat()) and
// prevent synthetic hash-splat parameters from matching synthetic hash-splat
// arguments when direct keyword matching is possible
not (ppos.isSynthHashSplat() and apos.isSynthHashSplat())
or
exists(int pos |
(
ppos.isSplat(pos)
or
ppos.isSynthSplat() and pos = 0
ppos.isSynthSplat(_) and
pos = 0
) and
(
apos.isSplat(pos)
or
apos.isSynthSplat() and pos = 0
apos.isSynthSplat(_) and pos = 0
)
) and
// prevent synthetic splat parameters from matching synthetic splat arguments
// when direct positional matching is possible
not exists(int actualSplatPos |
ppos.isSynthSplat(actualSplatPos) and
apos.isSynthSplat(actualSplatPos)
)
or
ppos.isAny() and argumentPositionIsNotSelf(apos)
Expand Down
Loading
Loading