Skip to content

Conversation

@chrchr-github
Copy link
Collaborator

No description provided.

@pfultz2
Copy link
Contributor

pfultz2 commented Dec 6, 2025

This is not really the correct fix. ValueFlowForward will check for variable changes.

The problem is that this comes from reverse analysis, so we need to traverse the subfunction in reverse as well. We don't really have a way to do that but we can skip the values that come from reverse analysis for now though.

@chrchr-github
Copy link
Collaborator Author

[...] but we can skip the values that come from reverse analysis for now though.

And how would we do that? Mark the values somehow?

@pfultz2
Copy link
Contributor

pfultz2 commented Dec 6, 2025

[...] but we can skip the values that come from reverse analysis for now though.

And how would we do that? Mark the values somehow?

You could check the tokvlaue, but it is a little hacky how we use that for the starting point (I would like to add an origin field for this instead).

Another way is to mark them but we need to mark when it goes reverse and forward as we do forward analysis from reverse analysis so the state would need to switch back. It should probably be an enum of Forward. Reverse, and Unknown.

@chrchr-github chrchr-github marked this pull request as draft December 7, 2025 09:54
@chrchr-github
Copy link
Collaborator Author

Another way is to mark them but we need to mark when it goes reverse and forward as we do forward analysis from reverse analysis so the state would need to switch back. It should probably be an enum of Forward. Reverse, and Unknown.

There seem to be many places where values flow forward, so I haven't implemented the switching logic. We also have no tests that require it yet.

@chrchr-github chrchr-github marked this pull request as ready for review December 12, 2025 09:11
@pfultz2
Copy link
Contributor

pfultz2 commented Dec 13, 2025

We could set the the direction of the value in the update method in the analyzer. That might be simpler than trying to do it for every forward.

Copy link
Contributor

@pfultz2 pfultz2 left a comment

Choose a reason for hiding this comment

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

**

@chrchr-github
Copy link
Collaborator Author

We could set the the direction of the value in the update method in the analyzer. That might be simpler than trying to do it for every forward.

I have tried that, but unless Reverse is set in makeReverseAnalyzer(), the FP still occurs. Maybe update() is not called for the value in question?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants