Check LinterSettings::preview for version-related syntax errors
#16429
Conversation
Summary -- Fixes part of the report in #16417 (comment) by checking `LinterSettings::preview` in both the language server and in the playground. That now covers all calls of `Parsed::unsupported_syntax_errors`. Test Plan -- Manual tests in both VS Code and the playground. I also added `preview = true` to the WASM test I added earlier.
|
| let unsupported_syntax_errors = if settings.linter.preview.is_enabled() { | ||
| parsed.unsupported_syntax_errors() | ||
| } else { | ||
| &[] | ||
| }; | ||
|
|
There was a problem hiding this comment.
Should we move this into check_path as that seems like the common denominator for the server, WASM, and the command-line?
There was a problem hiding this comment.
Hmm, that might not be possible in the server because it needs to know whether the user has enabled showSyntaxErrors or not
There was a problem hiding this comment.
Ah I think you're right. I kept putting it right next to the message/diagnostic generation code, but I think we could just truncate the vec inside of check_path because it's called right before each of these. I'll just have to add a new Parsed method because the field itself is private.
I don't think we can move this exact code unless we return an additional unsupported_syntax_errors from check_path.
There was a problem hiding this comment.
Hmm, that might not be possible in the server because it needs to know whether the user has enabled
showSyntaxErrorsor not
I think we could pass a ShowSyntaxErrors enum similar to flags::Noqa to check_path which in combination with the settings.preview.is_enabled() check be used to filter out syntax error diagnostics. But, ShowSyntaxErrors would need to be defined in ruff_linter as it can't be in ruff_server because otherwise it would create circular dependency.
There was a problem hiding this comment.
I like the idea of unifying the handling if it doesn't introduce a lot of complexity but I think we can ship the bug fix as is. That gives us more time to explore the refactor and allows us to ship the bugfix sooner.
There was a problem hiding this comment.
I think I might just leave this alone for now if it's okay with you both. I looked into it briefly today and realized that my parsed.unsupported_syntax_errors.truncate() idea would require a &mut Parsed, which I didn't really want to add. There are four of these calls, which is annoying, but we'll also get to simply delete them all once the feature is out of preview.
Another temporary option could be passing a preview: bool (or enum) argument to the unsupported_syntax_errors method to avoid duplicating the explicit ifs.
There was a problem hiding this comment.
Thank you for looking into it. I think it's fine to leave it as is.
|
Thanks for the quick review! I'd also like to get #16425 merged and then we could probably do a release if you want. That should cover all of the (known) bugs with the version-related errors. I'm going to take a look at your suggestion in the morning, though. It's getting pretty late |
* main: [red-knot] Switch to a handwritten parser for mdtest error assertions (#16422) [red-knot] Disallow more invalid type expressions (#16427) Bump version to Ruff 0.9.9 (#16434) Check `LinterSettings::preview` for version-related syntax errors (#16429) Avoid caching files with unsupported syntax errors (#16425) Prioritize "bug" label for changelog sections (#16433) [`flake8-copyright`] Add links to applicable options (`CPY001`) (#16421) Fix string-length limit in documentation for PYI054 (#16432) Show version-related syntax errors in the playground (#16419) Allow passing `ParseOptions` to inline tests (#16357) Bump version to 0.9.8 (#16414) [red-knot] Ignore surrounding whitespace when looking for `<!-- snapshot-diagnostics -->` directives in mdtests (#16380) Notify users for invalid client settings (#16361) Avoid indexing the project if `configurationPreference` is `editorOnly` (#16381)
* dcreager/alist-type: Remove unneeded shear override Update property test CI Move alist into red_knot_python_semantic [red-knot] Switch to a handwritten parser for mdtest error assertions (#16422) [red-knot] Disallow more invalid type expressions (#16427) Bump version to Ruff 0.9.9 (#16434) Check `LinterSettings::preview` for version-related syntax errors (#16429) Avoid caching files with unsupported syntax errors (#16425) Prioritize "bug" label for changelog sections (#16433) [`flake8-copyright`] Add links to applicable options (`CPY001`) (#16421) Fix string-length limit in documentation for PYI054 (#16432) Show version-related syntax errors in the playground (#16419) Allow passing `ParseOptions` to inline tests (#16357) Bump version to 0.9.8 (#16414) [red-knot] Ignore surrounding whitespace when looking for `<!-- snapshot-diagnostics -->` directives in mdtests (#16380) Notify users for invalid client settings (#16361) Avoid indexing the project if `configurationPreference` is `editorOnly` (#16381)
Summary -- This PR updates `check_path` in the `ruff_linter` crate to return a `Vec<Message>` instead of a `Vec<Diagnostic>`. The main motivation for this is to make it easier to convert semantic syntax errors directly into `Message`s rather than `Diagnostic`s in #16106. However, this also has the benefit of keeping the preview check on unsupported syntax errors in `check_path`, as suggested in #16429 (comment). All of the interesting changes are in the first commit. The second commit just renames variables like `diagnostics` to `messages`, and the third commit is a tiny import fix. I also updated the `ExpandedMessage::location` field name, which caused a few extra commits tidying up the playground code. I thought it was nicely symmetric with `end_location`, but I'm happy to revert that too. Test Plan -- Existing tests. I also tested the playground and server manually.
Summary
Fixes part of the report in #16417 (comment) by checking
LinterSettings::previewin both the language server and in the playground. That now covers all calls ofParsed::unsupported_syntax_errors.Test Plan
Manual tests in both VS Code and the playground. I also added
preview = trueto the WASM test I added earlier.