hypothesis icon indicating copy to clipboard operation
hypothesis copied to clipboard

WIP: Add copying where it might be necessary

Open sternj opened this issue 1 year ago • 2 comments

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 avatar Sep 30 '24 20:09 sternj

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.

sternj avatar Oct 10 '24 19:10 sternj

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 🙂

Zac-HD avatar Oct 11 '24 01:10 Zac-HD

Closing this as stale; I'd be very happy to reopen if it updates again.

Zac-HD avatar Jan 27 '25 04:01 Zac-HD