-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fallback to requires-python
in certain cases when target-version
is not found
#16319
Conversation
|
ad3eedb
to
0c589b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this.
We may need to restructure the code more. I believe the current implementation also applies the fallback for inherited and maybe even user configurations.
We only want the fallback to apply to top-level project configurations.
Since configurations are resolved relative the to the file being checked, I just want to confirm that what you mean here is something like the below. (Maybe the implementation is not what you'd prefer, but is the logic doing what you meant?) diff --git a/crates/ruff_workspace/src/pyproject.rs b/crates/ruff_workspace/src/pyproject.rs
index 9d9bc4199..cbc3958ab 100644
--- a/crates/ruff_workspace/src/pyproject.rs
+++ b/crates/ruff_workspace/src/pyproject.rs
@@ -152,7 +152,10 @@ pub fn find_user_settings_toml() -> Option<PathBuf> {
}
/// Load `Options` from a `pyproject.toml` or `ruff.toml` file.
-pub(super) fn load_options<P: AsRef<Path>>(path: P) -> Result<Options> {
+pub(super) fn load_options<P: AsRef<Path>>(
+ path: P,
+ version_strategy: TargetVersionStrategy,
+) -> Result<Options> {
if path.as_ref().ends_with("pyproject.toml") {
let pyproject = parse_pyproject_toml(&path)?;
let mut ruff = pyproject
@@ -172,16 +175,21 @@ pub(super) fn load_options<P: AsRef<Path>>(path: P) -> Result<Options> {
if let Ok(ref mut ruff) = ruff {
if ruff.target_version.is_none() {
debug!("No `target-version` found in `ruff.toml`");
- if let Some(dir) = path.as_ref().parent() {
- let fallback = get_fallback_target_version(dir);
- if fallback.is_some() {
- debug!(
+ match version_strategy {
+ TargetVersionStrategy::Standard => {}
+ TargetVersionStrategy::RequiresPythonFallback => {
+ if let Some(dir) = path.as_ref().parent() {
+ let fallback = get_fallback_target_version(dir);
+ if fallback.is_some() {
+ debug!(
"Deriving `target-version` from `requires-python` in `pyproject.toml`"
);
- } else {
- debug!("No `pyproject.toml` with `requires-python` in same directory; `target-version` unspecified");
+ } else {
+ debug!("No `pyproject.toml` with `requires-python` in same directory; `target-version` unspecified");
+ }
+ ruff.target_version = fallback;
+ }
}
- ruff.target_version = fallback;
}
}
}
@@ -236,6 +244,13 @@ fn get_minimum_supported_version(requires_version: &VersionSpecifiers) -> Option
PythonVersion::iter().find(|version| Version::from(*version) == minimum_version)
}
+#[derive(Debug, Default)]
+pub(super) enum TargetVersionStrategy {
+ #[default]
+ Standard,
+ RequiresPythonFallback,
+}
+
#[cfg(test)]
mod tests {
use std::fs;
diff --git a/crates/ruff_workspace/src/resolver.rs b/crates/ruff_workspace/src/resolver.rs
index f550f810f..ef10119c3 100644
--- a/crates/ruff_workspace/src/resolver.rs
+++ b/crates/ruff_workspace/src/resolver.rs
@@ -23,7 +23,7 @@ use ruff_linter::package::PackageRoot;
use ruff_linter::packaging::is_package;
use crate::configuration::Configuration;
-use crate::pyproject::settings_toml;
+use crate::pyproject::{settings_toml, TargetVersionStrategy};
use crate::settings::Settings;
use crate::{pyproject, FileResolverSettings};
@@ -319,7 +319,15 @@ pub fn resolve_configuration(
}
// Resolve the current path.
- let options = pyproject::load_options(&path).with_context(|| {
+ let version_strategy = if configurations.is_empty() {
+ TargetVersionStrategy::RequiresPythonFallback
+ } else {
+ // For inherited configurations, we do not attempt to
+ // fill missing `target-version` with `requires-python`
+ // from a nearby `pyproject.toml`
+ TargetVersionStrategy::Standard
+ };
+ let options = pyproject::load_options(&path, version_strategy).with_context(|| {
if configurations.is_empty() {
format!(
"Failed to load configuration `{path}`", |
Uhm, hard to say because I'm mostly unfamiliar with this part of Ruff. Different strategies could be a solution or we start using different methods based on what configuration we're resolving (I think that's what we do in Red Knot) |
I guess what I'm trying to say is: what do you mean by "project root"? It doesn't seem to me like that's really a first-class notion in Ruff in the same way that it kind of has to be in Probably I'm just not understanding the design spec here - sorry! So far I think I understand these requirements:
But I'm not understanding the requirement around the project root. Would it be possible to give an example of the behavior you're looking for? |
That's a good point and using the term project root was probably more confusing than helpful (because Ruff doesn't know that concept). What I meant is that this fallback shouldn't apply to user-level configurations or an extended ( |
Excellent, thank you for clarifying! That's what I suspected (and is handled by the diff above). I'll tackle the user-config and inherited config adjustments now. Sorry for the confusion! |
@dhruvmanila no urgency here, but before we add this to v0.10 it'd be great if you could double-check that the changes to |
Yeah, I think it makes sense although I'd also test it out in an editor context. I can add it to my todo but I was wondering if you had done any testing in an editor (VS Code or any other)? |
debug!("Deriving `target-version` from found `requires-python`"); | ||
} | ||
config.target_version = fallback.map(Into::into); | ||
} | ||
let settings = config.into_settings(&path_dedot::CWD)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract the path into a variable and use it both in find_fallback_target_version
and when calling into_settings
. It required me few cycles to understand whether path_dedot::CWD
is correct but I then noticed that it's already what into_setting
uses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, it depends on what behavior we want.
The way I have it currently set up I'm trying to make the behavior change as little as possible, so I've kept the settings resolution path the same no matter what, but attempted to find a fallback version using the same logic as we did when searching for a user configuration file. That logic would start from the stdin filename directory if it was passed in.
In other words, I don't think I can extract a path
variable here since I may be passing a different value to find_fallback_target_version
than I am to into_settings
.
"Deriving `target-version` from `requires-python` in `pyproject.toml`" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Deriving `target-version` from `requires-python` in `pyproject.toml`" | |
); | |
"Deriving `target-version` from `requires-python` in `pyproject.toml`" | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead I made the spacing match the debug
call in the other branch: ffd2dbf
@@ -699,7 +732,7 @@ pub fn python_file_at_path( | |||
for ancestor in path.ancestors() { | |||
if let Some(pyproject) = settings_toml(ancestor)? { | |||
let (root, settings) = | |||
resolve_scoped_settings(&pyproject, Relativity::Parent, transformer)?; | |||
resolve_scoped_settings(&pyproject, Relativity::Parent, transformer, None)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why using None
here is correct (or when using None
is correct in general). Can you tell me a bit more about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried to clarify here (replacing None
with new variant Unknown
which signals that the configuration origin is unknown to the caller: af82f19
3a1033a
to
38bf1bd
Compare
I haven't tried it in an editor yet but I will! |
583edc8
to
8aa8c56
Compare
@dylwil3 feel free to merge this into the ruff-0.10 feature branch once it's ready (I already changed the base) |
44eec2b
to
e4e1c58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes all look correct to me for the server settings.
Edit: Wait, I'm seeing some discrepancies in the editor context.
I'm currently testing out this in an editor context using some of the test cases mentioned in https://github.com/astral-sh/ruff/blob/8aa8c56cad2697c0e4c6a86aa997221e842dcfea/crates/ruff/tests/lint.rs ruff/crates/ruff/tests/lint.rs Lines 2088 to 2094 in 8aa8c56
For this test case, the target version in the editor is still giving me 3.9. As per the test case, it should be 3.11 ? The debug information gives me that the indexed configuration file is empty even though the project has a ruff/crates/ruff_workspace/src/pyproject.rs Lines 81 to 85 in ba44e9d
I believe this isn't expected? Sorry for taking this on at the last moment before the 0.10 release. ruff/crates/ruff/tests/lint.rs Lines 2716 to 2724 in 8aa8c56
For this test case, I'm seeing the incorrect behavior where the target version is still 3.9. ruff/crates/ruff/tests/lint.rs Lines 2397 to 2404 in 8aa8c56
For this test case, I'm seeing the correct behavior where the target version is 3.11. |
It's relatively straightforward for as long as it doesn't require changes to the extension itself.
The following is my configuration {
"ruff.path": [
"/Users/micha/astral/ruff/target/debug/ruff",
]
} |
Update: It looks like the case where there's no ruff/crates/ruff/src/resolve.rs Lines 104 to 115 in b098255
Looking into it now - sorry for all the last minute hullabaloo close to the release! |
I believe the |
Ok(None) => {} | ||
Ok(None) => { | ||
let settings = RuffSettings::editor_only(editor_settings, &directory); | ||
index.write().unwrap().insert(directory, Arc::new(settings)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this change correctly, we'd now have a fallback settings for every directory in the project. I think this might create an unexpected behavior because for a file in a nested directory, it would use this fallback settings instead of the settings from the project root? Let me test this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I tested out, I think this is an incorrect fix. Consider the following directory structure:
.
├── nested
│ └── foo
│ └── test.py
├── pyproject.toml
└── test.py
The nested/foo/test.py
does not consider any settings from pyproject.toml
, it's only the test.py
file that will consider it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not entirely clear to me why the change here is necessary. Shouldn't the changes above be sufficient?
let mut config = Configuration::default(); | ||
if let Some(fallback_version) = find_fallback_target_version(root) { | ||
config.target_version = Some(PythonVersion::from(fallback_version)); | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that we'll pick up the requires-python
constraint even if a user uses editor-only
in their settings. I'd be a bit confused if editor-only
loads any project settings. @dhruvmanila what's your take on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think we should be looking at any of the config files if the configuration preference is editorOnly
unless the user has explicitly asks us using the ruff.configuration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we won't have gotten this far in the code in that case, though, right? We exited earlier here:
ruff/crates/ruff_server/src/session/index/ruff_settings.rs
Lines 114 to 123 in dd2313a
if editor_settings.configuration_preference == ConfigurationPreference::EditorOnly { | |
tracing::debug!( | |
"Using editor-only settings for workspace: {} (skipped indexing)", | |
root.display() | |
); | |
return RuffSettingsIndex { | |
index: BTreeMap::default(), | |
fallback: Arc::new(RuffSettings::editor_only(editor_settings, root)), | |
}; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That branch will be calling RuffSettings::editor_only
to create a default configuration so we'll be getting there in this case.
crates/ruff/src/resolve.rs
Outdated
let settings = resolve_root_settings( | ||
&pyproject, | ||
config_arguments, | ||
ConfigurationOrigin::UserSettings, | ||
)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems inconsistent that we don't apply the fallback behavior if a user only has the user-level configuration. It means that we won't pick up the right python version if you have:
- a user level ruff configuration
- but use a
pyproject.toml
without atool.ruff
section at the project level
I think it would be good to apply the requires-python
fallback in this case as well.
I pushed three new commits:
@dhruvmanila and @dylwil3 could yo uboth take a look |
@dylwil3 we should also update https://docs.astral.sh/ruff/configuration/#config-file-discovery or have a new section explaining the |
IIUC, between a user-level config ( Why are you not sure about the behavior in Ruff? I think preferring a project specific version makes sense intuitively, I'd be surprised to see that the |
Thank you, this looks awesome!
✅
This seems intuitive to me.
Tested this in VSCode and looks good! The behavior Dhruv pointed out before with a nested file no longer occurs, and the fallback matches the CLI behavior in the edge case where there is no I will work on some documentation! |
Okay, so I think this PR is ready to land, assuming my changes look good. |
The code looks good, thank you for addressing the issues. I also did some quick testing (not the same ones as Dylan). |
Thanks so much @dhruvmanila and @MichaReiser !! |
…ot found (#16721) ## Summary Restores #16319 after it got dropped from the 0.10 release branch :( --------- Co-authored-by: dylwil3 <[email protected]>
## Summary Follow-up release for Ruff v0.10 that now includes the following two changes that we intended to ship but slipped: * Changes to how the Python version is inferred when a `target-version` is not specified (#16319) * `blanket-noqa` (`PGH004`): Also detect blanked file-level noqa comments (and not just line level comments). ## Test plan I verified that the binary built on this branch respects the `requires-python` setting ([logs](https://www.diffchecker.com/qyJWYi6W/), left: v0.10, right: v0.11)
Summary
This PR introduces the following modifications in configuration resolution:
In the event where we are reading in a configuration file
myconfig
with notarget-version
specified, we will search for apyproject.toml
in the same directory asmyconfig
and see if it has arequires-python
field. If so, we will use that as thetarget-version
.In the event where...
--isolated
, and--config
specifying a target versionthen we will search for a
pyproject.toml
file withrequired-python
in an ancestor directory and use that if we find it.We've also added some debug logs to indicate which of these paths is taken.
Implementation
Two small things:
pyproject.toml
file that has already been parsed at some earlier stage in the resolution process. It seemed like avoiding that would require more drastic changes - but maybe there is a clever way that I'm not seeing!pyproject.toml
s rather than propagate them. The reasoning here is that we have already found or decided upon a perfectly good configuration, and this is just a "bonus" to find a better guess for the target version.Closes #14813, #16662
Testing
The linked issue contains a repo for reproducing the behavior, which we used for a manual test:
In addition, we've added snapshot tests with the CLI output in some examples. Please let me know if there are some additional scenarios you'd like me to add tests for!