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

[red-knot] MDTest parsing problems #15923

Closed
sharkdp opened this issue Feb 4, 2025 · 0 comments · Fixed by #15926
Closed

[red-knot] MDTest parsing problems #15923

sharkdp opened this issue Feb 4, 2025 · 0 comments · Fixed by #15926
Assignees
Labels
bug Something isn't working red-knot Multi-file analysis & type inference

Comments

@sharkdp
Copy link
Contributor

sharkdp commented Feb 4, 2025

Description

Unfortunately, I only noticed this after merging #15890 when I worked on some tests. This looks like a regression. In the following file, the code snippet will not be detected and never executed. In a larger file, this could easily lead to mistakes where someone writes a test that seems to succeed.

This "succeeds" on main, but failed with "unexpected error: [invalid-syntax]" before merging pre-#15890 (as it should):

# My test

No newline between prose and code:
```py
# A syntax error:
;
```

While writing a MRE for this bug, I also found this weird behavior which definitely looks like a bug. When the ; in the example above is replaced with §, I get:

thread 'mdtest__binary_integers' panicked at crates/ruff_python_trivia/src/cursor.rs:138:41:
byte index 1 is not a char boundary; it is inside '§' (bytes 0..2) of `§
```
`
@sharkdp sharkdp added bug Something isn't working red-knot Multi-file analysis & type inference labels Feb 4, 2025
@sharkdp sharkdp self-assigned this Feb 4, 2025
sharkdp added a commit that referenced this issue Feb 4, 2025
## Summary

Replaces our existing Markdown test parser with a fully hand-written
parser. I tried to fix this bug using the old approach and kept running
into problems. Eventually this seemed like the easier way. It's more
code (+50 lines, excluding the new test), but I hope it's relatively
straightforward to understand, compared to the complex interplay between
the byte-stream-manipulation and regex-parsing that we had before.

I did not really focus on performance, as the parsing time does not
dominate the test execution time, but this seems to be slightly faster
than what we had before (executing all MD tests; debug):

| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| this branch | 2.775 ± 0.072 | 2.690 | 2.877 | 1.00 |
| `main` | 2.921 ± 0.034 | 2.865 | 2.967 | 1.05 ± 0.03 |

closes #15923

## Test Plan

One new regression test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant