diff --git a/conformance/third_party/conformance.exp b/conformance/third_party/conformance.exp index 4976b2ac1c..ea2568f707 100644 --- a/conformance/third_party/conformance.exp +++ b/conformance/third_party/conformance.exp @@ -5075,6 +5075,39 @@ } ], "enums_member_values.py": [ + { + "code": -2, + "column": 12, + "concise_description": "assert_type(int, Literal[1]) failed", + "description": "assert_type(int, Literal[1]) failed", + "line": 21, + "name": "assert-type", + "severity": "error", + "stop_column": 43, + "stop_line": 21 + }, + { + "code": -2, + "column": 12, + "concise_description": "assert_type(int, Literal[1]) failed", + "description": "assert_type(int, Literal[1]) failed", + "line": 22, + "name": "assert-type", + "severity": "error", + "stop_column": 41, + "stop_line": 22 + }, + { + "code": -2, + "column": 16, + "concise_description": "assert_type(int, Literal[1, 3]) failed", + "description": "assert_type(int, Literal[1, 3]) failed", + "line": 26, + "name": "assert-type", + "severity": "error", + "stop_column": 50, + "stop_line": 26 + }, { "code": -2, "column": 16, @@ -5111,8 +5144,8 @@ { "code": -2, "column": 5, - "concise_description": "Enum member `GREEN` has type `Literal['green']`, must match the `_value_` attribute annotation of `int`", - "description": "Enum member `GREEN` has type `Literal['green']`, must match the `_value_` attribute annotation of `int`", + "concise_description": "Enum member `GREEN` has type `str`, must match the `_value_` attribute annotation of `int`", + "description": "Enum member `GREEN` has type `str`, must match the `_value_` attribute annotation of `int`", "line": 78, "name": "bad-assignment", "severity": "error", diff --git a/crates/pyrefly_config/src/config.rs b/crates/pyrefly_config/src/config.rs index 1984ccee34..8aeb344fb4 100644 --- a/crates/pyrefly_config/src/config.rs +++ b/crates/pyrefly_config/src/config.rs @@ -730,7 +730,10 @@ impl ConfigFile { } pub fn from_real_config_file(&self) -> bool { - matches!(self.source, ConfigSource::File(_)) + matches!( + self.source, + ConfigSource::File(_) | ConfigSource::FailedParse(_) + ) } pub fn python_version(&self) -> PythonVersion { @@ -1225,7 +1228,7 @@ impl ConfigFile { let mut configure_source_db = |build_system: &mut BuildSystem| { let root = match &self.source { - ConfigSource::File(path) => { + ConfigSource::File(path) | ConfigSource::FailedParse(path) => { let mut root = path.to_path_buf(); root.pop(); root @@ -1278,7 +1281,7 @@ impl ConfigFile { )); } - if let ConfigSource::File(path) = &self.source { + if let ConfigSource::File(path) | ConfigSource::FailedParse(path) = &self.source { configure_errors .into_map(|e| ConfigError::warn(e.context(format!("{}", path.display())))) } else { @@ -2490,6 +2493,10 @@ output-format = "omit-errors" "Expected FailedParse, got {:?}", config.source ); + assert!( + config.from_real_config_file(), + "FailedParse config should be treated as a real config file" + ); assert!(!errors.is_empty(), "Expected errors for invalid TOML"); // The config should still respect the file's location for project root detection. assert_eq!(config.source.root(), Some(root.path())); diff --git a/crates/pyrefly_types/src/literal.rs b/crates/pyrefly_types/src/literal.rs index d332b6c46f..95584ba259 100644 --- a/crates/pyrefly_types/src/literal.rs +++ b/crates/pyrefly_types/src/literal.rs @@ -13,7 +13,6 @@ use pyrefly_derive::TypeEq; use pyrefly_derive::Visit; use pyrefly_derive::VisitMut; use pyrefly_util::assert_words; -use pyrefly_util::visit::VisitMut; use ruff_python_ast::ExprBooleanLiteral; use ruff_python_ast::ExprBytesLiteral; use ruff_python_ast::ExprFString; @@ -58,27 +57,15 @@ pub enum Lit { } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -#[derive(Visit, TypeEq)] +#[derive(Visit, VisitMut, TypeEq)] pub struct LitEnum { pub class: ClassType, pub member: Name, /// Raw type assigned to name in class def. /// We store the raw type so we can return it when the value or _value_ attribute is accessed. - /// NOTE: Intentionally excluded from VisitMut so that type transformations like - /// `promote_implicit_literals` don't mutate this metadata field, which must remain - /// stable for `.value` resolution. pub ty: Type, } -/// Manual VisitMut that skips the `ty` field. The `ty` is semantic metadata for `.value` -/// resolution and must not be mutated by recursive type transformations (e.g. literal promotion). -/// We still recurse into `class` so that type arguments on generic enums are visited. -impl VisitMut for LitEnum { - fn recurse_mut(&mut self, f: &mut dyn FnMut(&mut Type)) { - self.class.visit_mut(f); - } -} - impl Display for Lit { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { diff --git a/pyrefly/lib/alt/class/class_field.rs b/pyrefly/lib/alt/class/class_field.rs index 92bddea4d0..f4efe24fdc 100644 --- a/pyrefly/lib/alt/class/class_field.rs +++ b/pyrefly/lib/alt/class/class_field.rs @@ -1762,8 +1762,8 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { // Determine the final type, promoting literals when appropriate. // Skip literal promotion for NNModule types: their fields are captured // constructor args that must preserve literal types for shape inference. - let (ty, unpromoted_ty) = if matches!(value_ty, Type::NNModule(_)) { - (value_ty, None) + let ty = if matches!(value_ty, Type::NNModule(_)) { + value_ty } else { let mut has_implicit_literal = value_ty.is_implicit_literal(); if !has_implicit_literal && matches!(initialization, ClassFieldInitialization::Method) { @@ -1771,7 +1771,6 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { has_implicit_literal |= current_type_node.is_implicit_literal(); }); } - // Save any unpromoted literal types, we need them for enums if annotation .as_ref() .and_then(|ann| ann.ty.as_ref()) @@ -1779,10 +1778,9 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { && matches!(read_only_reason, None | Some(ReadOnlyReason::NamedTuple)) && has_implicit_literal { - let pre = value_ty.clone(); - (value_ty.promote_implicit_literals(self.stdlib), Some(pre)) + value_ty.promote_implicit_literals(self.stdlib) } else { - (value_ty, None) + value_ty } }; @@ -1866,7 +1864,6 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { name, direct_annotation.as_ref(), &ty, - unpromoted_ty.as_ref(), field_definition, descriptor.is_some(), range, @@ -2108,7 +2105,6 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { name: &Name, direct_annotation: Option<&Annotation>, ty: &Type, - unpromoted_ty: Option<&Type>, field_definition: &ClassFieldDefinition, is_descriptor: bool, range: TextRange, @@ -2118,7 +2114,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { class, name, direct_annotation, - unpromoted_ty.unwrap_or(ty), + ty, field_definition, is_descriptor, range, @@ -3997,21 +3993,11 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { field: &Name, ancestor: (&str, &str), ) -> bool { - self.field_defining_class_matches(cls, field, |c| { - c.has_toplevel_qname(ancestor.0, ancestor.1) - }) - } - - /// Check whether the defining class of `field` on `cls` satisfies `predicate`. - /// Returns false if the field does not exist. - pub fn field_defining_class_matches( - &self, - cls: &Class, - field: &Name, - predicate: impl FnOnce(&Class) -> bool, - ) -> bool { - self.get_class_member_with_defining_class(cls, field) - .is_some_and(|member| predicate(&member.defining_class)) + let member = self.get_class_member_with_defining_class(cls, field); + match member { + Some(member) => member.is_defined_on(ancestor.0, ancestor.1), + None => false, + } } /// Get the class's `__new__` method. diff --git a/pyrefly/lib/alt/class/enums.rs b/pyrefly/lib/alt/class/enums.rs index 76115f90ac..56f58c8715 100644 --- a/pyrefly/lib/alt/class/enums.rs +++ b/pyrefly/lib/alt/class/enums.rs @@ -123,8 +123,17 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { .or_else(|| self.get_instance_attribute(class, attr_name)) } - /// Special-case enum attribute lookups. Dispatches to the appropriate helper - /// based on the attribute name and whether we have a known enum literal. + /// Special-case enum attribute lookups: + /// - if this is an enum and the attribute is `value`, we'll redirect it to + /// look up the type of `_value_` so that the `value` property understands + /// annotated `_value_`. + /// - furthermore, if there is no annotation on `_value_` (meaning it inherits + /// the `Any` annotation from `enum.Enum` we will compute the type based + /// on the observed types of members). + /// + /// The resulting attribute is read-only if it is `value`, which is a property, + /// and read-write if it is `_value_`. Whether `_value_` should be considered + /// writable is unspecified, but we at least have to allow it in `__init__`. /// /// `enum_literal` is set if we're looking this up on a known member, like `Literal[MyEnum.X]` /// @@ -138,149 +147,80 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { name: &Name, ) -> Option { let enum_metadata = metadata.enum_metadata()?; - if name == &VALUE { - if !self.field_defining_class_matches(class.class_object(), &VALUE, |c| { - c.has_toplevel_qname(ModuleName::enum_().as_str(), "Enum") - || c.has_toplevel_qname(ModuleName::enum_().as_str(), "IntEnum") - || c.has_toplevel_qname(ModuleName::enum_().as_str(), "StrEnum") - || c.has_toplevel_qname(ModuleName::django_models_enums().as_str(), "Choices") - }) { - return None; - } - let ty = if let Some(lit_enum) = enum_literal { - self.enum_value_lookup_on_member(class, lit_enum, enum_metadata) - } else { - self.enum_value_lookup_on_class(class, enum_metadata) - }; - Some(ClassAttribute::read_write(ty)) - } else if name == &VALUE_PROP { - if !self.field_defining_class_matches(class.class_object(), &VALUE_PROP, |c| { - c.has_toplevel_qname(ModuleName::enum_().as_str(), "Enum") - || c.has_toplevel_qname(ModuleName::enum_().as_str(), "IntEnum") - || c.has_toplevel_qname(ModuleName::enum_().as_str(), "StrEnum") - || c.has_toplevel_qname(ModuleName::django_models_enums().as_str(), "Choices") - }) { - return None; - } - if let Some(lit_enum) = enum_literal { - self.get_enum_literal_or_instance_attribute(lit_enum, metadata, &VALUE) - .map(|attr| attr.read_only_equivalent(ReadOnlyReason::EnumMemberValue)) - } else { - self.get_enum_or_instance_attribute(class, metadata, &VALUE) - .map(|attr| attr.read_only_equivalent(ReadOnlyReason::EnumMemberValue)) - } - } else { - None - } - } - - /// Look up the `_value_` attribute for a specific enum member (e.g. `MyEnum.X._value_`). - /// Whether `_value_` should be read-write is unspecified, but we need to allow assigning - /// it in `__init__` so we make it read-write. - fn enum_value_lookup_on_member( - &self, - class: &ClassType, - lit_enum: &LitEnum, - enum_metadata: &EnumMetadata, - ) -> Type { - let mixed_in = self.mixed_in_enum_data_type(class.class_object()); - let has_new = self - .get_class_fields(class.class_object()) - .is_some_and(|f| f.contains(&dunder::NEW)); - // When `__new__` is defined, it can rewrite `_value_` at runtime, so the raw - // RHS type is unreliable. - // Fallbacks in order of priority: mixed-in type, type of `_value_`, `Any` - if has_new { - return if let Some(mixed_in) = mixed_in { - mixed_in - } else if let Some(value_ty) = self.type_of_enum_value(enum_metadata) { - value_ty - } else { - self.heap.mk_any_implicit() - }; - } - let value_ty = self.enum_literal_to_value_type(lit_enum.clone(), enum_metadata.is_django); - // Only preserve the literal type if its base class type matches the mixin exactly. - if let Some(ref mixed_in) = mixed_in { - let promoted = value_ty.clone().promote_implicit_literals(self.stdlib); - if &promoted == mixed_in { - value_ty - } else { - mixed_in.clone() - } - } else { - value_ty - } - } - - /// Look up the `_value_` attribute for an enum type (not a specific member). - /// Whether `_value_` should be read-write is unspecified, but we need to allow assigning - /// it in `__init__` so we make it read-write. - fn enum_value_lookup_on_class(&self, class: &ClassType, enum_metadata: &EnumMetadata) -> Type { - let mixed_in = self.mixed_in_enum_data_type(class.class_object()); - let has_new = self - .get_class_fields(class.class_object()) - .is_some_and(|f| f.contains(&dunder::NEW)); - // When `__new__` is defined, it can rewrite `_value_` at runtime. Fall back to the - // mixed-in type, or `Any` if there is no mixin. - if has_new { - return if let Some(mixed_in) = mixed_in { - mixed_in - } else { - self.heap.mk_any_implicit() - }; - } - if let Some(mixed_in) = mixed_in { - return mixed_in; + if !((name == &VALUE || name == &VALUE_PROP) + && (self.field_is_inherited_from( + class.class_object(), + name, + (ModuleName::enum_().as_str(), "Enum"), + ) || self.field_is_inherited_from( + class.class_object(), + name, + (ModuleName::django_models_enums().as_str(), "Choices"), + ))) + { + return None; } - // The `_value_` annotation on `enum.Enum` is `Any`; we can infer a better type. - let enum_value_types: Vec<_> = self - .get_enum_members(class.class_object()) - .into_iter() - .filter_map(|lit| { - if let Lit::Enum(lit_enum) = lit { - let value_ty = - self.enum_literal_to_value_type(*lit_enum, enum_metadata.is_django); - if value_ty.is_implicit_literal() { - Some(value_ty.promote_implicit_literals(self.stdlib)) + if name == &VALUE { + let ty = self + .mixed_in_enum_data_type(class.class_object()) + .unwrap_or_else(|| { + if let Some(lit_enum) = enum_literal { + self.enum_literal_to_value_type(lit_enum.clone(), enum_metadata.is_django) } else { - Some(value_ty) + // The `_value_` annotation on `enum.Enum` is `Any`; we can infer a better type + let enum_value_types: Vec<_> = self + .get_enum_members(class.class_object()) + .into_iter() + .filter_map(|lit| { + if let Lit::Enum(lit_enum) = lit { + Some(self.enum_literal_to_value_type( + *lit_enum, + enum_metadata.is_django, + )) + } else { + None + } + }) + .collect(); + if enum_value_types.is_empty() { + // Assume Any, rather than Never, if there are no members because they may + // be created dynamically and we don't want downstream analysis to be incorrect. + self.heap.mk_any_implicit() + } else { + self.unions(enum_value_types) + } } - } else { - None - } - }) - .collect(); - if enum_value_types.is_empty() { - // Don't assume Never if there are no members, because they may - // be created dynamically and we don't want false-positives downstream. - self.heap.mk_any_implicit() + }); + Some(ClassAttribute::read_write(ty)) + } else if let Some(lit_enum) = enum_literal { + self.get_enum_literal_or_instance_attribute(lit_enum, metadata, &VALUE) + .map(|attr| { + // Do not allow writing `.value`, which is a property. + attr.read_only_equivalent(ReadOnlyReason::EnumMemberValue) + }) } else { - self.unions(enum_value_types) + self.get_enum_or_instance_attribute(class, metadata, &VALUE) + .map(|attr| { + // Do not allow writing `.value`, which is a property. + attr.read_only_equivalent(ReadOnlyReason::EnumMemberValue) + }) } } /// If this enum mixes in a data type by inheriting from it, return the mixed-in type. - /// Searches all bases, not just the first, to handle cases like - /// `IntegerChoices(Choices, IntEnum)` where the data type comes from `IntEnum`. fn mixed_in_enum_data_type(&self, class: &Class) -> Option { let bases = self.get_base_types_for_class(class); + let first_base = bases.iter().next()?; let enum_class = self.stdlib.enum_class(); - for base in bases.iter() { - if *base == *enum_class { - continue; - } else if self.has_superclass(base.class_object(), enum_class.class_object()) { - if let Some(ty) = self.mixed_in_enum_data_type(base.class_object()) { - return Some(ty); - } - } else { - return Some(self.heap.mk_class_type(base.clone())); - } + if first_base == enum_class { + None + } else if self.has_superclass(first_base.class_object(), enum_class.class_object()) { + self.mixed_in_enum_data_type(first_base.class_object()) + } else { + Some(self.heap.mk_class_type(first_base.clone())) } - None } - /// Convert an enum literal's raw value type to its `.value` type. fn enum_literal_to_value_type(&self, lit_enum: LitEnum, is_django: bool) -> Type { let ty = if is_django { transform_django_enum_value(lit_enum.ty, self.heap) @@ -329,6 +269,10 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { /// - Check whether the field is a member (which depends only on its type and name) /// - Validate that a member should not have an annotation, and should respect any explicit annotation on `_value_` /// + /// TODO(stroxler, yangdanny): We currently operate on promoted types, which means we do not infer `Literal[...]` + /// types for the `.value` / `._value_` attributes of literals. This is permitted in the spec although not optimal + /// for most cases; we are handling it this way in part because generic enum behavior is not yet well-specified. + /// /// We currently skip the check for `_value_` if the class defines `__new__`, since that can /// change the value of the enum member. https://docs.python.org/3/howto/enum.html#when-to-use-new-vs-init pub fn get_enum_class_field_type( diff --git a/pyrefly/lib/commands/dump_config.rs b/pyrefly/lib/commands/dump_config.rs index 3e781bc749..1c437852cc 100644 --- a/pyrefly/lib/commands/dump_config.rs +++ b/pyrefly/lib/commands/dump_config.rs @@ -129,15 +129,13 @@ fn dump_config( ConfigSource::Synthetic => { println!("Default configuration"); } - ConfigSource::PythonToolMarker(path) - | ConfigSource::Marker(path) - | ConfigSource::FailedParse(path) => { + ConfigSource::PythonToolMarker(path) | ConfigSource::Marker(path) => { println!( "Default configuration for project root marked by `{}`", path.display() ); } - ConfigSource::File(path) => { + ConfigSource::File(path) | ConfigSource::FailedParse(path) => { let config_from = if std::env::var(&config_env) .is_ok_and(|f| &Path::new(&f).absolutize() == path) { diff --git a/pyrefly/lib/commands/files.rs b/pyrefly/lib/commands/files.rs index 4432d80138..2dbb93fd56 100644 --- a/pyrefly/lib/commands/files.rs +++ b/pyrefly/lib/commands/files.rs @@ -125,15 +125,9 @@ fn get_globs_and_config_for_project( None => get_project_config_for_current_dir(args, wrapper)?, }; match &config.source { - ConfigSource::File(path) => { + ConfigSource::File(path) | ConfigSource::FailedParse(path) => { info!("Checking project configured at `{}`", path.display()); } - ConfigSource::FailedParse(path) => { - warn!( - "Config at `{}` failed to parse, checking with default configuration", - path.display() - ); - } ConfigSource::PythonToolMarker(path) | ConfigSource::Marker(path) => { info!( "Found `{}` marking project root, checking root directory with default configuration", diff --git a/pyrefly/lib/lsp/non_wasm/server.rs b/pyrefly/lib/lsp/non_wasm/server.rs index b2cae30a3f..812ce3bdbe 100644 --- a/pyrefly/lib/lsp/non_wasm/server.rs +++ b/pyrefly/lib/lsp/non_wasm/server.rs @@ -2685,11 +2685,12 @@ impl Server { // In this case, we have a config file like mypy.ini, or a pyproject.toml // with Python tool sections but no [tool.pyrefly]. We don't parse it for // pyrefly config, so treat it as if we don't have any config. - ConfigSource::PythonToolMarker(_) - | ConfigSource::Marker(_) - | ConfigSource::FailedParse(_) => TypeErrorDisplayStatus::NoConfigFile, - // We actually have a pyrefly.toml, so we can decide based on the config. - ConfigSource::File(_) => { + ConfigSource::PythonToolMarker(_) | ConfigSource::Marker(_) => { + TypeErrorDisplayStatus::NoConfigFile + } + // We actually have a pyrefly.toml (or found one but failed to parse it), so we + // can decide based on the loaded defaults tied to that path. + ConfigSource::File(_) | ConfigSource::FailedParse(_) => { if config.disable_type_errors_in_ide(path) { TypeErrorDisplayStatus::DisabledInConfigFile } else { diff --git a/pyrefly/lib/state/loader.rs b/pyrefly/lib/state/loader.rs index 19d721940e..15a037ea81 100644 --- a/pyrefly/lib/state/loader.rs +++ b/pyrefly/lib/state/loader.rs @@ -56,16 +56,18 @@ impl FindError { config_source: &ConfigSource, ) -> FindError { let config_suffix = match config_source { - ConfigSource::File(p) => format!(" (from config in `{}`)", p.display()), - ConfigSource::PythonToolMarker(p) | ConfigSource::Marker(p) => { + ConfigSource::File(p) => { + format!(" (from config in `{}`)", p.display()) + } + ConfigSource::FailedParse(p) => { format!( - " (from default config for project root marked by `{}`)", + " (from default config for `{}` which failed to parse)", p.display() ) } - ConfigSource::FailedParse(p) => { + ConfigSource::PythonToolMarker(p) | ConfigSource::Marker(p) => { format!( - " (from default config for `{}` which failed to parse)", + " (from default config for project root marked by `{}`)", p.display() ) } diff --git a/pyrefly/lib/state/lsp.rs b/pyrefly/lib/state/lsp.rs index 2859671591..a749a76706 100644 --- a/pyrefly/lib/state/lsp.rs +++ b/pyrefly/lib/state/lsp.rs @@ -52,6 +52,7 @@ use ruff_python_ast::ExprName; use ruff_python_ast::Identifier; use ruff_python_ast::Keyword; use ruff_python_ast::ModModule; +use ruff_python_ast::Stmt; use ruff_python_ast::StmtImportFrom; use ruff_python_ast::UnaryOp; use ruff_python_ast::name::Name; @@ -214,6 +215,11 @@ pub struct FindPreference { /// when callers need the raw definition (e.g., call-graph queries that /// unwrap decorators like `@lru_cache`). pub resolve_call_dunders: bool, + /// If no parameter matches a keyword name (including after checking the callee + /// signature), resolve to the callee's own definition (e.g. function or method + /// name). Enabled for go-to-definition; disabled for rename and find-refs so + /// invalid keywords do not resolve to the callee symbol. + pub keyword_argument_fallback_to_callee: bool, } impl Default for FindPreference { @@ -222,6 +228,7 @@ impl Default for FindPreference { import_behavior: ImportBehavior::JumpThroughEverything, prefer_pyi: true, resolve_call_dunders: true, + keyword_argument_fallback_to_callee: false, } } } @@ -851,19 +858,43 @@ impl<'a> Transaction<'a> { callee_range: TextRange, param_name: &Identifier, ) -> Option { + fn find_parameter_in_function( + function_def: &ruff_python_ast::StmtFunctionDef, + param_name: &Identifier, + ) -> Option { + // Include positional-only parameters: the type checker may reject + // `f(x=0)` for `def f(x, /)`, but go-to-definition should still land on `x`. + for posonly_param in function_def.parameters.posonlyargs.iter() { + if posonly_param.name().id() == param_name.id() { + return Some(posonly_param.name().range()); + } + } + for regular_param in function_def.parameters.args.iter() { + if regular_param.name().id() == param_name.id() { + return Some(regular_param.name().range()); + } + } + for kwonly_param in function_def.parameters.kwonlyargs.iter() { + if kwonly_param.name().id() == param_name.id() { + return Some(kwonly_param.name().range()); + } + } + None + } + let covering_nodes = Ast::locate_node(ast, callee_range.start()); match (covering_nodes.first(), covering_nodes.get(1)) { (Some(AnyNodeRef::Identifier(_)), Some(AnyNodeRef::StmtFunctionDef(function_def))) => { - // Only check regular and kwonly params since posonly params cannot be passed by name - // on the caller side. - for regular_param in function_def.parameters.args.iter() { - if regular_param.name().id() == param_name.id() { - return Some(regular_param.name().range()); - } - } - for kwonly_param in function_def.parameters.kwonlyargs.iter() { - if kwonly_param.name().id() == param_name.id() { - return Some(kwonly_param.name().range()); + find_parameter_in_function(function_def, param_name) + } + (Some(AnyNodeRef::Identifier(_)), Some(AnyNodeRef::StmtClassDef(class_def))) => { + // Constructor keyword args can come from class calls; prefer explicit __init__ + // parameter ranges when available. + for stmt in class_def.body.iter() { + if let Stmt::FunctionDef(function_def) = stmt + && function_def.name.id.as_str() == "__init__" + { + return find_parameter_in_function(function_def, param_name); } } None @@ -1816,14 +1847,21 @@ impl<'a> Transaction<'a> { }; for range in ranges.into_iter() { - let refined_param_range = - self.refine_param_location_for_callee(ast.as_ref(), range, identifier); - // TODO(grievejia): Should we filter out unrefinable ranges here? - results.push(FindDefinitionItem { - metadata: DefinitionMetadata::Variable(Some(SymbolKind::Variable)), - definition_range: refined_param_range.unwrap_or(range), - module: module_info.dupe(), - }) + if let Some(refined_param_range) = + self.refine_param_location_for_callee(ast.as_ref(), range, identifier) + { + results.push(FindDefinitionItem { + metadata: DefinitionMetadata::Variable(Some(SymbolKind::Variable)), + definition_range: refined_param_range, + module: module_info.dupe(), + }); + } else if preference.keyword_argument_fallback_to_callee { + results.push(FindDefinitionItem { + metadata: DefinitionMetadata::Variable(Some(SymbolKind::Variable)), + definition_range: range, + module: module_info.dupe(), + }); + } } } results @@ -2227,10 +2265,20 @@ impl<'a> Transaction<'a> { position, FindPreference { prefer_pyi: false, + keyword_argument_fallback_to_callee: true, ..Default::default() }, ) - .or_else(|_| self.find_definition(handle, position, FindPreference::default())); + .or_else(|_| { + self.find_definition( + handle, + position, + FindPreference { + keyword_argument_fallback_to_callee: true, + ..Default::default() + }, + ) + }); definitions.map(|defs| { defs.into_vec() @@ -2251,6 +2299,7 @@ impl<'a> Transaction<'a> { FindPreference { import_behavior: ImportBehavior::StopAtEverything, prefer_pyi: true, + keyword_argument_fallback_to_callee: true, ..Default::default() }, )?; @@ -2277,11 +2326,18 @@ impl<'a> Transaction<'a> { } } - self.find_definition(handle, position, FindPreference::default()) - .map(|defs| { - defs.into_vec() - .into_map(|item| TextRangeWithModule::new(item.module, item.definition_range)) - }) + self.find_definition( + handle, + position, + FindPreference { + keyword_argument_fallback_to_callee: true, + ..Default::default() + }, + ) + .map(|defs| { + defs.into_vec() + .into_map(|item| TextRangeWithModule::new(item.module, item.definition_range)) + }) } /// This function should not be used for user-facing go-to-definition. However, it is exposed to @@ -2858,23 +2914,32 @@ impl<'a> Transaction<'a> { } pub fn prepare_rename(&self, handle: &Handle, position: TextSize) -> Option { - let identifier_context = self.identifier_at(handle, position); + let identifier_context = self.identifier_at(handle, position)?; let definitions = self .find_definition(handle, position, FindPreference::default()) .map(Vec1::into_vec) .unwrap_or_default(); - for FindDefinitionItemWithDocstring { module, .. } in definitions { + for FindDefinitionItemWithDocstring { module, .. } in &definitions { // Block rename only if it's third-party AND not an editable install/source file. - if self.is_third_party_module(&module, handle) && !self.is_source_file(&module, handle) - { + if self.is_third_party_module(module, handle) && !self.is_source_file(module, handle) { return None; } } - Some(identifier_context?.identifier.range) + // If we can't resolve a keyword argument to a concrete parameter symbol, reject rename + // instead of advertising a rename target that will later produce no edits. + if matches!( + identifier_context.context, + IdentifierContext::KeywordArgument(_) + ) && definitions.is_empty() + { + return None; + } + + Some(identifier_context.identifier.range) } pub fn find_local_references( diff --git a/pyrefly/lib/test/django/enums.rs b/pyrefly/lib/test/django/enums.rs index c925917ef7..0738842def 100644 --- a/pyrefly/lib/test/django/enums.rs +++ b/pyrefly/lib/test/django/enums.rs @@ -119,7 +119,7 @@ django_testcase!( test_overwrite_value, r#" from django.db.models import Choices -from typing import Any, Literal, assert_type +from typing import Any, assert_type class A(Choices): X = 1 @@ -133,7 +133,7 @@ class B(Choices): assert_type(A.X._value_, str) assert_type(A.X.value, str) assert_type(A.values, list[str]) -assert_type(B.X._value_, Literal[1]) +assert_type(B.X._value_, int) assert_type(B.X.value, str) assert_type(B.values, list[str]) "#, diff --git a/pyrefly/lib/test/enums.rs b/pyrefly/lib/test/enums.rs index 45abae0873..ba542c6a52 100644 --- a/pyrefly/lib/test/enums.rs +++ b/pyrefly/lib/test/enums.rs @@ -50,8 +50,8 @@ assert_type(MyEnum["X"], Literal[MyEnum.X]) assert_type(MyEnum.__PRIVATE, int) # E: Private attribute `__PRIVATE` cannot be accessed outside of its defining class assert_type(MyEnum.X.name, Literal["X"]) assert_type(MyEnum.X._name_, Literal["X"]) -assert_type(MyEnum.X.value, Literal[1]) -assert_type(MyEnum.X._value_, Literal[1]) +assert_type(MyEnum.X.value, int) +assert_type(MyEnum.X._value_, int) MyEnum["FOO"] # E: Enum `MyEnum` does not have a member named `FOO` @@ -83,7 +83,8 @@ def f(x: Literal[E.X]) -> int: ... def f(x: E) -> int | str | None: ... def f(x) -> int | str | None: ... -assert_type(f(E.X), int) +# Without narrowing enum members to literal types, `E.X` matches the `E` overload. +assert_type(f(E.X), int | str | None) "#, ); @@ -183,7 +184,7 @@ class MyEnum(Enum): V = member(1) W = auto() X = 1 - Y = "FOO" # E: Enum member `Y` has type `Literal['FOO']`, must match the `_value_` attribute annotation of `int` + Y = "FOO" # E: Enum member `Y` has type `str`, must match the `_value_` attribute annotation of `int` Z = member("FOO") # E: Enum member `Z` has type `str`, must match the `_value_` attribute annotation of `int` def get_value(self) -> int: @@ -549,7 +550,7 @@ from enum import IntEnum class Color(IntEnum): RED = ... # E: Enum member `RED` has type `Ellipsis`, must match the `_value_` attribute annotation of `int` - GREEN = "wrong" # E: Enum member `GREEN` has type `Literal['wrong']`, must match the `_value_` attribute annotation of `int` + GREEN = "wrong" # E: Enum member `GREEN` has type `str`, must match the `_value_` attribute annotation of `int` "# ); env.add_with_path("pyi", "pyi.pyi", r#" @@ -557,7 +558,7 @@ from enum import IntEnum class Color(IntEnum): RED = ... - GREEN = "wrong" # E: Enum member `GREEN` has type `Literal['wrong']`, must match the `_value_` attribute annotation of `int` + GREEN = "wrong" # E: Enum member `GREEN` has type `str`, must match the `_value_` attribute annotation of `int` "# ); env @@ -666,7 +667,7 @@ testcase!( test_mixin_datatype, r#" from enum import Enum -from typing import assert_type, Literal +from typing import assert_type class A(float, Enum): X = 1 @@ -685,12 +686,12 @@ testcase!( test_override_value_prop, r#" from enum import Enum -from typing import assert_type, Literal +from typing import assert_type class E(Enum): X = 1 @property def value(self) -> str: ... -assert_type(E.X._value_, Literal[1]) +assert_type(E.X._value_, int) assert_type(E.X.value, str) "#, ); @@ -907,111 +908,3 @@ class Foo(str, Enum): Foo.from_dict({}) "#, ); - -// Regression test for https://github.com/facebook/pyrefly/issues/2583 -// StrEnum defines its own `_value_: str`, so the guard on the special-case `_value_` path -// must include StrEnum (and IntEnum) in addition to `enum.Enum`. -testcase!( - test_strenum_value_literal_type, - r#" -from enum import StrEnum, Enum -from typing import assert_type, Literal - -class Foo(StrEnum): - X = "x" - -class Bar(str, Enum): - Y = "y" - -def take_literal(z: Literal["x", "y"]) -> None: ... - -# StrEnum: specific literal gives literal type -assert_type(Foo.X.value, Literal["x"]) -# str, Enum mixin: specific literal correctly gives literal type -assert_type(Bar.Y.value, Literal["y"]) - -# Generic instance access should give the mixed-in type -def test(foo: Foo, bar: Bar) -> None: - assert_type(foo.value, str) - assert_type(bar.value, str) - take_literal(Foo.X.value) - take_literal(Bar.Y.value) - "#, -); - -// When a member's value type doesn't match the mixin (e.g. int value in a str-mixin enum), -// the mixin's `__new__` coerces the value at runtime (e.g. `str(42)` → `"42"`), -// so `.value` returns the mixin type. -testcase!( - test_mixin_value_type_mismatch, - r#" -from enum import Enum -from typing import assert_type, Literal - -class Bad(str, Enum): - X = 42 - -assert_type(Bad.X.value, str) - "#, -); - -// When `__new__` converts the value type (e.g. int → str), the literal is only -// preserved if it's a subtype of the mixin. Otherwise we fall back to the mixin type. -testcase!( - test_mixin_new_converts_type, - r#" -from enum import Enum -from typing import assert_type, Literal - -class E(str, Enum): - # String literal is a subtype of str, so the literal is preserved. - A = "hello" - # Int literal is NOT a subtype of str; str.__new__ coerces it at runtime, - # so `.value` falls back to the mixin type. - B = 42 - -assert_type(E.A.value, Literal["hello"]) -assert_type(E.B.value, str) - "#, -); - -// When `__new__` is defined, it can rewrite `_value_` at runtime, so the raw RHS type -// is unreliable. Without a mixin or explicit `_value_` annotation, we fall back to `Any` -// since we can't infer what `__new__` assigns. -testcase!( - test_new_rewrites_value, - r#" -from enum import Enum -from typing import assert_type, Literal, Any - -class SeFileType(Enum): - ALL = ("a", "all files") - REGULAR = ("f", "regular file") - - def __new__(cls, code: str, description: str) -> "SeFileType": - obj = object.__new__(cls) - obj._value_ = code - return obj - -assert_type(SeFileType.ALL.value, Any) - "#, -); - -// When `__new__` is defined AND a mixin is present, fall back to the mixin type. -testcase!( - test_new_with_mixin, - r#" -from enum import Enum -from typing import assert_type - -class Planet(float, Enum): - MERCURY = (3.303e+23, 2.4397e6) - - def __new__(cls, mass: float, radius: float) -> "Planet": - obj = float.__new__(cls, mass) - obj._value_ = mass - return obj - -assert_type(Planet.MERCURY.value, float) - "#, -); diff --git a/pyrefly/lib/test/lsp/definition.rs b/pyrefly/lib/test/lsp/definition.rs index ed712237a0..63939d3eaf 100644 --- a/pyrefly/lib/test/lsp/definition.rs +++ b/pyrefly/lib/test/lsp/definition.rs @@ -439,7 +439,7 @@ Definition Result: ^ Definition Result: 4 | def baz(x: int, /) -> None: pass - ^^^ + ^ "# .trim(), report.trim(), diff --git a/pyrefly/lib/test/lsp/lsp_interaction/rename.rs b/pyrefly/lib/test/lsp/lsp_interaction/rename.rs index 8527c7d91f..913e0a1c2c 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/rename.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/rename.rs @@ -47,6 +47,37 @@ fn test_prepare_rename() { interaction.shutdown().unwrap(); } +#[test] +fn test_prepare_rename_dataclass_keyword_argument_returns_null() { + let root = get_test_files_root(); + let root_path = root.path().join("rename_dataclass_keyword"); + + let mut interaction = LspInteraction::new(); + interaction.set_root(root_path.clone()); + interaction + .initialize(InitializeSettings::default()) + .unwrap(); + + let user_code = root_path.join("user_code.py"); + interaction.client.did_open("user_code.py"); + + interaction + .client + .send_request::(json!({ + "textDocument": { + "uri": Url::from_file_path(&user_code).unwrap().to_string() + }, + "position": { + "line": 10, + "character": 4 + } + })) + .expect_response(serde_json::Value::Null) + .unwrap(); + + interaction.shutdown().unwrap(); +} + #[test] fn test_rename_third_party_symbols_in_venv_is_not_allowed() { let root = get_test_files_root(); diff --git a/pyrefly/lib/test/lsp/lsp_interaction/test_files/rename_dataclass_keyword/user_code.py b/pyrefly/lib/test/lsp/lsp_interaction/test_files/rename_dataclass_keyword/user_code.py new file mode 100644 index 0000000000..a49d6a0a95 --- /dev/null +++ b/pyrefly/lib/test/lsp/lsp_interaction/test_files/rename_dataclass_keyword/user_code.py @@ -0,0 +1,13 @@ +from dataclasses import dataclass + + +@dataclass +class Foo: + a: int + b: str + + +Foo( + a=123, + b="abc", +) diff --git a/pyrefly/lib/test/lsp/rename.rs b/pyrefly/lib/test/lsp/rename.rs index a05f1fa815..1d5503299f 100644 --- a/pyrefly/lib/test/lsp/rename.rs +++ b/pyrefly/lib/test/lsp/rename.rs @@ -140,3 +140,61 @@ Rename locations: report.trim(), ); } + +#[test] +fn test_rename_dataclass_keyword_argument_does_not_rename_class() { + let code = r#" +from dataclasses import dataclass + +@dataclass +class Foo: + a: int + b: str + +Foo( + a=123, +# ^ + b="abc", +) +"#; + let report = get_batched_lsp_operations_report(&[("main", code)], get_test_report); + assert_eq!( + r#" +# main.py +10 | a=123, + ^ +Rename locations: +"# + .trim(), + report.trim(), + ); +} + +#[test] +fn test_rename_init_parameter_updates_constructor_keyword_argument() { + let code = r#" +class Foo: + def __init__(self, value): + pass + +Foo( + value=1, +# ^ +) +"#; + let report = get_batched_lsp_operations_report(&[("main", code)], get_test_report); + assert_eq!( + r#" +# main.py +7 | value=1, + ^ +Rename locations: +3 | def __init__(self, value): + ^^^^^ +7 | value=1, + ^^^^^ +"# + .trim(), + report.trim(), + ); +} diff --git a/test/config.md b/test/config.md index 0b165f3041..72916484e5 100644 --- a/test/config.md +++ b/test/config.md @@ -50,7 +50,7 @@ $ echo "x: str = 0" > $TMPDIR/oops.py && echo "errors = { bad-assignment = false ```scrut {output_stream: stderr} $ mkdir $TMPDIR/bad_config && touch $TMPDIR/bad_config/empty.py && echo "oops oops" > $TMPDIR/bad_config/pyrefly.toml && cd $TMPDIR/bad_config && $PYREFLY check - WARN Config at `*/pyrefly.toml` failed to parse, checking with default configuration (glob) + INFO Checking project configured at `*/pyrefly.toml` (glob) ERROR */pyrefly.toml: TOML parse error* (glob) | 1 | oops oops @@ -83,7 +83,7 @@ Fatal configuration error ```scrut {output_stream: stderr} $ $PYREFLY check -c $TMPDIR/bad_config/pyrefly.toml - WARN Config at `*/pyrefly.toml` failed to parse, checking with default configuration (glob) + INFO Checking project configured at `*/pyrefly.toml` (glob) ERROR */pyrefly.toml: TOML parse error* (glob) | 1 | oops oops