[red-knot] feat: Inference for BytesLiteral comparisons#13746
Conversation
|
crates/red_knot_python_semantic/resources/mdtest/comparison/byte_literals.md
Show resolved
Hide resolved
Um, you can ignore that. It reports some spurious results sometimes when a PR branch is a bit behind There's no way this PR could have caused any of those changes, so it can be safely disregarded here! |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
| reveal_type(b"abc" == b"abc") # revealed: Literal[True] | ||
| reveal_type(b"abc" == b"ab") # revealed: Literal[False] |
There was a problem hiding this comment.
Does it work without reveal_type?
| reveal_type(b"abc" == b"abc") # revealed: Literal[True] | |
| reveal_type(b"abc" == b"ab") # revealed: Literal[False] | |
| b"abc" == b"abc" # revealed: Literal[True] | |
| b"abc" == b"ab" # revealed: Literal[False] |
Is comment aligning part of mdtest's design guide? (is it allowed or preferred?)
There was a problem hiding this comment.
Does it work without
reveal_type?
no, the reveal_type call is necessary to trigger a "Revealed" assertion when using mdtest-based tests. This is documented here.
Is comment aligning part of mdtest's design guide? (is it allowed or preferred?)
The comment alignment seems fine to me. At some point maybe we might start running our scripts/check_docs_formatted.py CI script on our mdtest test files. But until we do that, or introduce autoformatting of these Python snippets some other way, I don't think we should be too picky about the formatting of the Python snippets in our test files.
There was a problem hiding this comment.
This is probably something we have to align on. I recommend to not rely on comment alignment because it's something that will break if we ever decide to format our example code using ruff (which I think would be a nice addition).
| let b1 = salsa_b1.value(self.db).as_ref(); | ||
| let b2 = salsa_b2.value(self.db).as_ref(); |
There was a problem hiding this comment.
Nit: You could have used &**salsa_b1.value(self.db) to aovid the as_ref
| } | ||
|
|
||
| (Type::BytesLiteral(salsa_b1), Type::BytesLiteral(salsa_b2)) => { | ||
| let contains_subsequence = |needle: &[u8], haystack: &[u8]| { |
There was a problem hiding this comment.
Nit: We could consider using https://docs.rs/memchr/latest/memchr/memmem/fn.find.html
There was a problem hiding this comment.
Yes, thanks. Did a grep for memmem , didn't find anything, and wasn't sure if it warranted adding a new dependency. Should have looked for memchr.
==> #13750
This is a follow-up on #13746: - Use `memmem::find` instead of rolling our own inferior version. - Avoid `x.as_ref()` calls using `&**x`
|
This is excellent, thank you!! |
Summary
Implements inference for
BytesLiteralcomparisons along the lines of #13634. A refactoring that unifies some code with what we do forStringLiteral(and possibly other cases, see also #13712) is planned for a later PR.closes #13687
Test Plan
Added tests using the new Markdown framework. I added the tests in
mdtest/comparison/byte_literals.md, following the convention proposed in this comment. This is somewhat in conflict with the proposed structure in #13719 which puts theStringLiteraltests undermdtest/boolean.md, so this will have to be sorted out after we agreed on a naming/grouping convention for these tests.