-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C#: Add tests for extractor fixes, and improve CFG for ConstCases #163
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
Conversation
- While statements - Object initializers
…andled. Move `getCondition` to `CaseStmt`. Implement the CFG and tests.
ce42d4d
to
3718237
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Just a few remarks.
csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more comments, but nothing urgent, so happy to merge as is.
// Flow from the last element of case expression to the condition | ||
cfe = lastConstCaseExpr(cc, c) and | ||
c.(MatchingCompletion).isMatch() and | ||
result = first(cc.getCondition()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure I wrote this originally, but github seems to have thrown my comment away:
I would replace
// Flow from last element of case expression to first element of statement
not exists(cc.getCondition()) and
cfe = lastConstCaseExpr(cc, c) and
c.(MatchingCompletion).isMatch() and
result = first(cc.getStmt())
or
// Flow from the last element of case expression to the condition
cfe = lastConstCaseExpr(cc, c) and
c.(MatchingCompletion).isMatch() and
result = first(cc.getCondition())
with
cfe = lastConstCaseExpr(cc, c) and
c.(MatchingCompletion).isMatch() and
if exists(cc.getCondition()) then
// Flow from the last element of case expression to the condition
result = first(cc.getCondition())
else
// Flow from last element of case expression to first element of statement
result = first(cc.getStmt())
private ControlFlowElement lastCaseStmt(CaseStmt cs, Completion c) { | ||
result = last(cs.(TypeCase).getStmt(), c) | ||
or | ||
result = last(cs.(ConstCase).getStmt(), c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could replace with
result = last(cs.(LabeledStmt).getStmt(), c)
Feel free to change if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that initially but additional edges appeared.
Ignore include/prepend statements in blocks
Kotlin: Add comments saying what generated TRAP files
…brary-after-2.20.4 PS: Fixup CFG library in preparation for 2.20.4
These tests will not pass until the corresponding extractor changes have been merged.
Move the
getCondition()
predicate fromConstCase
toCaseStmt
, because const cases can also havewhen
parts, that were not previously extracted.Implement the CFG for
ConstCases
that have awhen
part.Add some regression tests for extractor fixes.