WIP: Add copying where it might be necessary
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.Assignorast.AugAssignthat is not inside anast.Subscriptorast.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
valueofast.Subscriptorsliceinast.Slice - [ ] Get unit test passage to parity with main branch
- [ ] Correct documentation for
potentially_modified_vars - [ ] Rename visitors to have more intention-revealing names
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.
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 🙂
Closing this as stale; I'd be very happy to reopen if it updates again.