Skip to content

Conversation

@yoff
Copy link
Contributor

@yoff yoff commented Oct 16, 2020

The main purpose of this PR is to make the argument passing tests more honest by adding a barrier for inflow at sources.
The way we wrote tests, for instance

@expects(9)
def test_multiple_kw_args():
    with_multiple_kw_args(b=arg2, c=arg3, a=arg1)
    with_multiple_kw_args(arg1, *(arg2,), arg3)
    with_multiple_kw_args(arg1, **{"c": arg3}, b=arg2)
    with_multiple_kw_args(**{"b": arg2}, **{"c": arg3}, **{"a": arg1})

meant that arg2 in the second and third calls would flow to arg2 in the fourth call and from there to the sink.
This made it look like the second and third call handled arg2 correctly just because the fourth call does.

The first commit adds the BarrierIn to all sources, removing the spurious flows from the test output.

This problem was discovered by looking at paths, so the second commit changes the tests to be path queries. I also added the remaining tests (for arguments 5 through 7).

I changed the first reported location to be source rather than sink, so that the current test output can easily be compared with the proposed output after select.

@yoff yoff added the Python label Oct 16, 2020
@yoff yoff requested a review from a team as a code owner October 16, 2020 08:08
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Very good catch 👍

Embarrassingly, I jumped straight into read the first commit before looking at the nice description you made in the PR. However, that illuminated that fact that I wasn't able to understand the problem from the comment for isBarrierIn, so I made a suggestion I think I would have understood if I didn't already understand the problem 😛

I did not look over changes to expected files in cced335

Comment on lines 35 to 36
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
* We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)`
* use-use flow would make it seem like we handled both cases, where in fact we only handled the last case.
* Removing flow into source nodes prevents this unwanted use-use flow.

Comment on lines 27 to 28
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
* We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)`
* use-use flow would make it seem like we handled both cases, where in fact we only handled the last case.
* Removing flow into source nodes prevents this unwanted use-use flow.

Comment on lines 27 to 28
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
* We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)`
* use-use flow would make it seem like we handled both cases, where in fact we only handled the last case.
* Removing flow into source nodes prevents this unwanted use-use flow.

Comment on lines 27 to 28
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
* We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)`
* use-use flow would make it seem like we handled both cases, where in fact we only handled the last case.
* Removing flow into source nodes prevents this unwanted use-use flow.

Comment on lines 27 to 28
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
* We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)`
* use-use flow would make it seem like we handled both cases, where in fact we only handled the last case.
* Removing flow into source nodes prevents this unwanted use-use flow.

Comment on lines 27 to 28
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
* We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)`
* use-use flow would make it seem like we handled both cases, where in fact we only handled the last case.
* Removing flow into source nodes prevents this unwanted use-use flow.

Comment on lines 27 to 28
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This prevents the argument from one call to reach the sink
* via a different call, by flowing to an argument of that.
* We had a problem where the fact that we use `arg` in a sequence of calls such as `func(kw=arg); func(arg)`
* use-use flow would make it seem like we handled both cases, where in fact we only handled the last case.
* Removing flow into source nodes prevents this unwanted use-use flow.

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

👍

@RasmusWL RasmusWL merged commit 5874a7b into github:main Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants