Skip to content

Fix false positive in a py/unreachable-statement #1779

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

Closed
wants to merge 1,022 commits into from

Conversation

xcorail
Copy link

@xcorail xcorail commented Aug 20, 2019

This is supposed to fix the FP reported in the forum: https://discuss.lgtm.com/t/python-false-positive-unreachable-statement/2240

The comparison start_bracket_index >= 0 > end_bracket_index is not well interpreted, and the subsequent comparison start_bracket_index > end_bracket_index is considered as unreachable. It appears that there is not even a FlowNode corresponding to this line, as if the CFG had been optimized and pruned of unreachable nodes.

So my change is to detect if in the same module, there is a previous chained comparison involving the same variables as the unreachable one. If there is one, then I don't report the statement as unreachable, because the CFG could have been erroneously pruned.

@ghost
Copy link

ghost commented Aug 20, 2019

CLA assistant check
All committers have signed the CLA.

@xcorail xcorail force-pushed the py-unreachable-stmt branch from 715c244 to c1381f3 Compare August 21, 2019 17:45
lcartey and others added 28 commits September 17, 2019 15:21
Added basic support for the using stmt, checked stmt, unchecked stmt
Note that the translations do not use the compiler generated element framework and hence they are just rough approximations. For accuracy, in the future their translation should use it.
The most common reason for the C# autobuilder to fail is because it
cannot determine a single unique .sln or .proj file to build, instead
reporting multiple sln or proj files at the same shortest depth. This
commit changes this to build all such files, rather than reporting an
error.
CPP: Add a test of ConditionalDeclExpr.
These files have come out of autoformat since the big commit that
autoformatted everything.
In theory this bug could associated CaptureOutNodes with the wrong transitively called
callable. However, in practice I could not create a test case that revealed incorrect
behaviour. I've included one such test case in the commit.

I believe that the cause of this is that OutNode::getACall() is not actually used in the
data flow libraries. Instead, DataFlowDispatch::Cached::getAnOutNode is the predicate
which is used to associated OutNode's with DataFlowCall's in practice, and that is always
used in a context that correctly binds the runtime target of the call.
…ent-tuple-size

Python: Modernise the `py/mixed-tuple-returns` query.
…check-performance

Approved by asger-semmle
As the linearity of the disjuncts is different, this enables us to
pick better join orders for each disjunct separately.
The user knows that an expression functionally determines its
hashCons value, and that an expression functionally determines
its number of children, but this is not provable from the
definitions, and so not usable by the optimiser. By storing
the result of those known-functional calls in a variable,
rather than repeating the call, we enable better join orders.
This is helpful for turning real-world cases into test cases.
@xcorail
Copy link
Author

xcorail commented Oct 1, 2019

@taus-semmle I applied your suggestions, thanks.
I will check if it solves also the unreacheable statement in #1989.

@taus-semmle
Copy link
Contributor

You may want to rebase onto master, as there have been considerable changes in the last month.
Also, I was looking at the performance of this, and there are some issues. You may need to pragma[inline] the allInvolvedVariablesAreInvolvedInPrevious predicate.

aschackmull and others added 2 commits October 2, 2019 14:32
A bit of manually applied "magic" ensures that we don't end up considering _all_ variables that are shared by two comparisons, but only those where the first comparison is complex. This is in general a much smaller number.
@taus-semmle
Copy link
Contributor

I... seem to have pushed directly to your branch. I apologise for the transgression -- I really did try creating a PR against this, but it seems something went wrong in the process.

@@ -22,6 +35,14 @@ predicate typing_import(ImportingStmt is) {
)
}

/* This predicate holds when a previous complex comparison occured before
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this typo: occurred

calumgrant and others added 2 commits October 2, 2019 15:36
@xcorail
Copy link
Author

xcorail commented Oct 2, 2019

I... seem to have pushed directly to your branch. I apologise for the transgression -- I really did try creating a PR against this, but it seems something went wrong in the process.

No worries. Do we still need the pragma[inline] after your change?

xcorail and others added 5 commits October 2, 2019 09:06
A bit of manually applied "magic" ensures that we don't end up considering _all_ variables that are shared by two comparisons, but only those where the first comparison is complex. This is in general a much smaller number.
@xcorail xcorail requested review from jf205, semmledocs-ac and a team as code owners October 2, 2019 16:12
@taus-semmle
Copy link
Contributor

Nope. We do not need inline any more. 🙂
(Also, I do hope the enormous merge resolves itself somewhat...)

@xcorail
Copy link
Author

xcorail commented Oct 2, 2019

Hmmmm ... @taus-semmle the rebase messed up this PR :-(
I'll close it and issue another one

@xcorail xcorail closed this Oct 2, 2019
@xcorail xcorail deleted the py-unreachable-stmt branch October 2, 2019 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.