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

PurePath(LazyIntSymbolicStr) error #280

Closed
tybug opened this issue Jul 19, 2024 · 6 comments
Closed

PurePath(LazyIntSymbolicStr) error #280

tybug opened this issue Jul 19, 2024 · 6 comments

Comments

@tybug
Copy link

tybug commented Jul 19, 2024

https://crosshair-web.org/?crosshair=0.1&python=3.8&gist=35e9e3726667c18ebffe94d2e88007de, in triaging test_can_generate_from_all_registered_types[PathLike] from HypothesisWorks/hypothesis#4034.

This one is my largest reach yet in correctly setting up the prereqs. This doesn't happen for "normal" symbolic strings, only LazyIntSymbolicStr. If I was to backseat and potentially embarrass myself, I would guess LazyIntSymbolicStr needs to declare _pytype as str?

@Zac-HD
Copy link
Contributor

Zac-HD commented Jul 20, 2024

If I was to backseat and potentially embarrass myself, I would guess LazyIntSymbolicStr needs to declare _pytype as str?

Hmm, I see a lot of failures that this could explain actually 🤔

@pschanely
Copy link
Owner

pschanely commented Jul 20, 2024

Oh wow this makes me so happy. And at the risk of embarrassing myself, I'll give you a needlessly detailed response:

  • The first argument to SymbolicBoundedIntTuple should be a sequence of integer pairs, not a sequence of integers - they represent ranges of values that the symbolic integers can have. We didn't do enough with the string in this example though for that to matter.
  • I think your example still fails with regular symbolic strings. Here are some examples along with some additional exposition in the comments.
  • CrossHair internals only consult .__ch_pytype__() to get a symbolic's type (which in most cases just returns _pytype, but not always); and the AnySymbolicStr definition happens to just return str directly. So, that part of things should be ok.
  • Unlike most other types, Python gives us no way to make something truly behave like a string. Pretty much ANY function that's written in C and consumes a string needs to have special handling, or you'll get these expected str, not LazyIntSymbolicStr errors. The pathlib module happens to be written in Python, so that's fine, but eventually we get to os.fspath which is implemented in C.
  • The easiest fix for things like this is to intercept the function and run it with a wrapper that realizes the arguments register_patch(os.fspath, with_realized_args(os.fspath)) (but of course, not great, because we want to defer realization as long as possible)
  • os.fspath is easily implementable in Python (and, little known fact: CPython includes a lot of pure Python variants and has one here). So, often I'll just copy those, or in this (rare) case it's actually exposed in the os module as os._fspath, So I can instead do this register_patch(os.fspath, os._fspath) (which is how I plan to fix it!)

@tybug
Copy link
Author

tybug commented Jul 20, 2024

I really appreciate you taking the time to write this up <3. The c conversion makes sense as a critical juncture.

I admit my diagnosis of 'some symbolic strings work fine' was a conclusion based on the following passing:

from pathlib import PurePath
                                           
def f(s: str) -> None:
    '''
    post: True
    '''
    PurePath(s)

@pschanely
Copy link
Owner

I admit my diagnosis of 'some symbolic strings work fine' was a conclusion based on the following passing:

Oooooh! Ok, so I'd totally forgotten about this, and it's really great that you brought it up. There is a hacky part of regular CrossHair that checks TypeErrors on their way out and looks for a few CrossHair type names in the exception message. If it finds a CrossHair type name, it suppresses the error and ignores that path. This code is tucked away in a corner that I simply hadn't thought to include in the hypothesis plugin.

Part of me doesn't want to copy that logic over to the plugin because it's been so great for issue discovery. However, it probably needs to stick around because we'll never support arbitrary 3rd party C modules. Plus, it'll fix a lot of hypothesis tests, and I can always disable the feature locally to go bug hunting at my leisure. So, I think I'll proceed with propagating my hack, unless someone has a counterpoint. Filed pschanely/hypothesis-crosshair#19

@Zac-HD
Copy link
Contributor

Zac-HD commented Jul 20, 2024

Hypothesis' InvalidArgument subclasses TypeError, so you might want to use an exact-match rather than isinstance - but otherwise that plan sounds good to me 🙂

@pschanely
Copy link
Owner

And ... support for os.fspath on symbolic strings has landed in v0.0.64.

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

No branches or pull requests

3 participants