GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||
GeezerVison is showing github.com — Match obsolete comparison diagnostics by youknowdot · Pull Request #9074 · RustPython/RustPython Conversation
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews: auto_review: drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if let Some(error) = parse_error | ||
| && let Some(range) = self.invalid_legacy_operator(error) | ||
| { | ||
| if error.location.start() > range.start() { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
is this the right place instead of patching parser?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The parser is the cleaner place for this diagnostic. I used the existing BarrySource adapter to keep the extraction independent of a parser release, but the review uncovered a real weakness: for 2 <<> 3, the textual match overlaps the << token and incorrectly emits the obsolete-comparison hint. I confirmed that with compile-only CPython 3.14.8/native checks. A comparison-context check in the pinned parser is the appropriate fix; the compiler override must also stop replacing that parser diagnostic. I am checking the narrow dependency path rather than importing the broader parser changes. This draft is not ready in its current form.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The parser is still the cleaner owner of this grammar-context diagnostic. The current head, dd0c42f, fixes the concrete 2 <<> 3 overlap by matching complete adjacent tokens instead of searching the source text, and its exact-head CI passes. That does not resolve your architecture question.
The implementation still lives in BarrySource::diagnostic: it combines the token range with the parser's first error location, and the compiler/symbol-table paths call it before first_parse_error. The registry Ruff parser remains at 0.16.10. Moving the diagnostic into the comparison production would therefore also require removing or narrowing this compiler override, so the adapter does not replace the parser's result.
I've corrected the stale PR description and kept this boundary explicit. The PR remains a draft pending that design decision; I haven't made an unrelated parser rewrite. This clarification is AI-assisted with OpenAI Codex (model version unavailable), based on the current source and hosted results.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
5b0fac6 to
7b25dc1
Compare
7b25dc1 to
1c36965
Compare
1c36965 to
0596dee
Compare
0596dee to
aa02e48
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_mmap.py (TODO: 19) dependencies: dependent tests: (2 tests)
Legend:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
aa02e48 to
dd0c42f
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Preserve the final intent of PR RustPython#9074 after its prior main merge: the parser owns diagnostics, and this branch adds only Python-visible regression coverage. Assisted-by: OpenAI Codex:model identifier unavailable
16773b8 to
77ef757
Compare
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.