Skip to content
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

WIP: Add copying where it might be necessary #4122

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sternj
Copy link

@sternj sternj commented Sep 30, 2024

NOTE: This is not ready for review, I have a significant amount of documentation to add, I need to update the test suite, and I missed several cases that I am writing out below. I am posting this early to receive feedback on the broad strokes of the approach, the implementation is (right now) entirely incorrect.

NOTE: a re-open of #4118, but with best practices observed!

Description

A proposed fix for #4113 that identifies what parameters might be mutated within a function that Ghostwriter is called on. To prevent side effects (particularly in equivalence mode), adds a call to copy.deepcopy to the parameter in the test function.

Right now, I mark a variable as potentially modified if:

  • there is an instance of is on the left hand side of an ast.Assign or ast.AugAssign that is not inside an ast.Subscript or ast.Slice. This includes when it is being indexed into or an attribute of it is being assigned
  • It or any of its attributes are being Called. Even if this isn't on the left-hand side of an assignment, it is safe to assume that any call may create side effects.

Todo

  • Ignore names used inside value of ast.Subscript or slice in ast.Slice
  • Get unit test passage to parity with main branch
  • Correct documentation for potentially_modified_vars
  • Rename visitors to have more intention-revealing names

@sternj
Copy link
Author

sternj commented Oct 10, 2024

Alright it's feature- and logic-complete, there's some import cleanup to do and @Zac-HD disagrees with me on one implementation decision which will need to be decided on, but further substantive work will be getting this new code to parity with the test suite.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 11, 2024

Looks like a good start to me! I think that design disagreement will basically come down to the tests; whatever default behavior we end up with should make at most a few percent of ghostwritten tests clearly worse.

You probably want to make format, use the --hypothesis-update-outputs argument to update expected outputs, write release notes in hypothesis-python/RELEASE.rst (see the example in that directory), and consider whether a function with ast.walk() would be simpler than some of the visitors. I'll aim for a more detailed review once CI is passing, or on request 🙂

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.

2 participants