Conversation
Emit diagnostics for config file parse errors (facebook#2078) Config errors (pyrefly.toml/pyproject.toml) now appear as LSP diagnostics in the Problems panel and clear when fixed.
sgavriil01
force-pushed
the
emit-config-diagnostics
branch
from
c487ef4 to
5393991
Compare
sgavriil01
force-pushed
the
emit-config-diagnostics
branch
from
5393991 to
7e85055
Compare
This comment has been minimized.
This comment has been minimized.
connernilsen
added
configuration
language-server
|
Hey @sgavriil01, thanks for making this! It seems like something we definitely need. I'll review early next week |
connernilsen
left a comment
connernilsen
left a comment
There was a problem hiding this comment.
Hey, thanks for doing this! It's looking great so far and I love the motivation. I left a few comments, mostly small things, but I do think there needs to be a different approach for how we figure out which files to update for, which I left in the else branch when we're getting the diagnostics to publish.
Let me know if you have any questions or disagree, I'm totally down to discuss things or give more thoughts if you want :)
crates/pyrefly_config/src/finder.rs
Outdated
| pub fn error(msg: anyhow::Error) -> Self { | ||
| Self { | ||
| severity: Severity::Error, | ||
| msg, | ||
| msg: format!("{:#}", msg), | ||
| file_path: None, | ||
| } | ||
| } | ||
|
|
||
| pub fn warn(msg: anyhow::Error) -> Self { | ||
| Self { | ||
| severity: Severity::Warn, | ||
| msg, | ||
| msg: format!("{:#}", msg), | ||
| file_path: None, | ||
| } |
There was a problem hiding this comment.
Thoughts on instead making this take an optional error? Then when creating a config error it will be more apparent that it can be tied to a config at a path? I could go either way on this/am open to discussion here though :)
pyrefly/lib/lsp/non_wasm/server.rs
Outdated
| /// Extract line and column numbers from TOML error messages | ||
| /// Returns (line, column) with 1-based indexing, defaults to (1, 1) | ||
| fn extract_line_col_from_toml_error(error_msg: &str) -> (usize, usize) { | ||
| let line = error_msg |
There was a problem hiding this comment.
Instead of trying to parse a position out of an error message, can we instead keep this information as part of the error we construct? TOML deserialization should have spans that will tell you this
pyrefly/lib/lsp/non_wasm/server.rs
Outdated
| // If we got fresh errors, deduplicate and update the cache | ||
| if !config_errors.is_empty() { | ||
| // Deduplicate by (file_path, message) since the same error can be added multiple times | ||
| let mut seen = std::collections::HashSet::new(); |
There was a problem hiding this comment.
nit: can we use a SmallSet here instead? They're more efficient, especially when working with small cardinalities or if you aren't removing things from the set
| }); | ||
| *self.cached_config_errors.lock() = config_errors.clone(); | ||
| } else { | ||
| // If get_config_errors() returned empty (consumed by mem::take), |
There was a problem hiding this comment.
Those errors might be empty because all errors have been fixed. If that's the case, will we not end up clearing diagnostics here?
What we might need to do instead to make sure this is working is to keep a map in the config loader of all config paths to errors that have been loaded since the last time they were taken. Then we can use that to determine if a config has been changed since the last time we published errors. If it's not in the map, then we don't update diagnostics from the previous result. If it is, then completely overwrite with new errors.
pyrefly/lib/lsp/non_wasm/server.rs
Outdated
| uri, | ||
| Vec::new(), | ||
| None, | ||
| DiagnosticSource::DidClose, |
There was a problem hiding this comment.
I don't think we want to use DidClose here unless we're sure it's from a close operation.
Instead, this function (publish_config_diagnostics) is only called from publish_for_handles which takes a DiagnosticSource as an arg. I think we should reuse that.
pyrefly/lib/lsp/non_wasm/server.rs
Outdated
| uri, | ||
| diagnostics, | ||
| None, | ||
| DiagnosticSource::CommittingTransaction, |
…efactor ConfigError API - Replace std::collections::HashSet with SmallSet for deduplication - Add source: DiagnosticSource parameter to publish_config_diagnostics - Consolidate ConfigError::error/warn methods to accept Option<PathBuf> - Update all callsites to use the new unified API
- Add span field to ConfigError for line/column information - Extract span from toml::de::Error in from_file() - Update config_error_to_diagnostic to use ConfigError.span() - Remove extract_line_col_from_toml_error() regex function - Add error_with_span() constructor for TOML errors
|
Thanks for the review! I've addressed the first 4 points:
For point 5 about the caching strategy - could you clarify the intended behavior? Should the |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
This pull request has been automatically marked as stale because it has not had recent activity for more than 2 weeks. If you are still working on this this pull request, please add a comment or push new commits to keep it active. Otherwise, please unassign yourself and allow someone else to take over. Thank you for your contributions! |
|
Hey @sgavriil01, sorry for the long delay. I've had to shift my focus internally for a bit to focus on a bug that's been causing a lot of problems. I think it should be resolved soon, so I'll come back and check out your changes here hopefully this week :) |
2 participants
Summary
This PR adds LSP diagnostics for config file (pyrefly.toml/pyproject.toml) parsing errors. When a config file has invalid syntax, diagnostics now appear in VS Code's Problems panel and clear automatically when the file is fixed.
Changes:
ConfigErrorto include file path informationworkspace.rswhere config errors were being discardedpublish_config_diagnostics()to emit LSP diagnostics for config filesFixes #2078
Test Plan
test_config_file_diagnostics()that creates invalid TOML and verifies proper handling./test.py --no-test --no-conformance- formatting and linting pass