Emit config diagnostics#2289
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 :)
| }); | ||
| *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.
…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 :) |
connernilsen
left a comment
connernilsen
left a comment
There was a problem hiding this comment.
Alright, thanks for making the previous changes. And sorry about the wait. I was pulled into a few internal issues over the past few weeks that took up all of my time.
For point 5 about the caching strategy - could you clarify the intended behavior? Should the ConfigFinder track which config files were recently loaded and only publish diagnostics for those, rather than maintaining a separate cache in the Server?
What I'm thinking is, we have our ConfigFinder implementation that handles loading and caching configs. There are times where a config is re-loaded or we clear the config finder's cache, but it kind of knows the state of all configs at all times, so let's use that as our source of truth.
Your changes above removing anyhow::Error from ConfigError make it so we can add #[derive(Clone)] to ConfigError, so now we don't need to take errors from the config finder, we can just clone them when needed. Those errors will also automatically be cleared when they're not longer useful, so we can just:
- clear all config errors that currently exist
- clone all errors from the config finder
- process the errors to turn them into diagnostics
- push those diagnostics to the IDE
Then all we need to do is make sure we know which configs we've pushed diagnostics to, and for that, we can continue to use the config_files_with_diagnostics you created, but not have to manage complicated state in the server with cached_config_errors.
Sorry, I changed a few details from my first comment mentioning the caching changes, so let me know if this makes more sense.
| let error_str = toml_err.to_string(); | ||
| let line = error_str | ||
| .split("line ") | ||
| .nth(1) | ||
| .and_then(|s| s.split(&[',', ' ', '\n'][..]).next()) | ||
| .and_then(|s| s.parse::<usize>().ok()) | ||
| .unwrap_or(1); | ||
| let column = error_str | ||
| .split("column ") | ||
| .nth(1) | ||
| .and_then(|s| s.split(&[',', ' ', '\n'][..]).next()) | ||
| .and_then(|s| s.parse::<usize>().ok()) | ||
| .unwrap_or(1); | ||
| (line, column) |
There was a problem hiding this comment.
Instead of trying to parse this, can we convert the span to a line, col pair? You can probably do something like that with
| toml_err.span().map(|_span| { | ||
| // The span is in bytes, but toml errors also expose line/column in their Display | ||
| // For now, we parse the display string which includes "at line X, column Y" | ||
| let error_str = toml_err.to_string(); | ||
| let line = error_str | ||
| .split("line ") | ||
| .nth(1) | ||
| .and_then(|s| s.split(&[',', ' ', '\n'][..]).next()) | ||
| .and_then(|s| s.parse::<usize>().ok()) | ||
| .unwrap_or(1); | ||
| let column = error_str | ||
| .split("column ") | ||
| .nth(1) | ||
| .and_then(|s| s.split(&[',', ' ', '\n'][..]).next()) | ||
| .and_then(|s| s.parse::<usize>().ok()) | ||
| .unwrap_or(1); | ||
| (line, column) |
There was a problem hiding this comment.
Instead of trying to parse this, can we convert the span to a line, col pair? You can probably do something like this
use ruff_source_file::LineIndex;
let index = LineIndex::from_source_text(source);
let line_col = index.line_column(text_size_offset, source);
// line_col.line and line_col.column are both 0-indexed
|
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! |
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