Skip to content

Python: No fieldFlowBranchLimit for SummarizedCallables #15936

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

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

yoff
Copy link
Contributor

@yoff yoff commented Mar 15, 2024

We noticed that when

  • a function has more than one summary (with different charpred)
  • one summary is subsumed by a subpath (or something happens around the function being extracted)
  • the function is called multiple times(we needed at least three)

one of the summaries would no longer lead to flow.

Fixed by doing the same as Ruby in #15689

We noticed that when
- a function has more than one summary (with different charpred)
- one summary is subsumed by a subpath (or something happens around the function being extracted)
- the function is called multiple times(we needed at least three)
one of the summaries would no longer lead to flow.
@yoff yoff added the no-change-note-required This PR does not need a change note label Mar 15, 2024
so python2 also respects it
@yoff yoff marked this pull request as ready for review March 18, 2024 08:12
@yoff yoff requested a review from a team as a code owner March 18, 2024 08:12
@hvitved
Copy link
Contributor

hvitved commented Mar 18, 2024

I'm guessing you probably need to implement #15689 for Python.

@yoff
Copy link
Contributor Author

yoff commented Mar 18, 2024

I'm guessing you probably need to implement #15689 for Python.

Thanks! That did indeed solve it.

@sidshank sidshank changed the title Pyhton: add test for conflicting summaries Python: add test for conflicting summaries Mar 18, 2024
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.

LGTM (I'll change the title of the PR though, since you're actually fixing the problem as well)

@RasmusWL RasmusWL changed the title Python: add test for conflicting summaries Python: No fieldFlowBranchLimit for SummarizedCallables Mar 19, 2024
@RasmusWL
Copy link
Member

Before we merge though: let's do some testing on performance impact! I think that's the main reason we didn't just implement it when Ruby did:

The change does come with a performance penalty, though, since we are now computing more results, and (hopefully) reducing false negatives.

So we should at least gauge that performance impact

@yoff yoff added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 19, 2024
@yoff
Copy link
Contributor Author

yoff commented Mar 19, 2024

No frightening performance cost (less than 1%), so I shall boldly merge this.

@yoff yoff removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 19, 2024
@yoff yoff merged commit ee411cc into github:main Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants