Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions pyrefly/lib/state/lsp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1248,15 +1248,26 @@ impl<'a> Transaction<'a> {
///
/// TSP prefers raw bound types of identifiers since it re-resolves declarations.
pub fn get_computed_type_at_range(&self, handle: &Handle, range: TextRange) -> Option<Type> {
// Bare names need declaration-preserving lookup to avoid coercing
// overloaded functions into synthesized callables.
if range.is_empty()
|| self.get_ast(handle).is_some_and(|module| {
Ast::locate_node(&module, range.start())
.into_iter()
.any(|node| node.range() == range && matches!(node, AnyNodeRef::ExprName(_)))
})
{
// Bare names and definition-name ranges need declaration-preserving
// lookup to avoid coercing overloaded functions into synthesized
// callables, and to preserve inferred returns for unannotated defs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked issue diagnoses Key::Definition as yielding the pre-inference function type, yet this fix relies on get_type_at_preserving_declaration -> the FunctionDef/MethodDef arm of get_type_at_impl_with_options -> Key::Definition to actually carry the inferred return. So the effective change is get_type_trace (the stored trace type) vs. the Key::Definition binding. Since that distinction is the crux of the fix, could you add a sentence here (or in the commit message) spelling out the real mechanism? It'll save future readers who go back to the issue and see the seemingly-contradictory claim about Key::Definition.

let is_identifier_name_range = !range.is_empty()
&& self.identifier_at(handle, range.start()).is_some_and(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (optional, non-blocking): identifier_at runs here and then again inside get_type_at_preserving_declaration, so the identifier is located twice per call. This isn't introduced by the PR (the old code also walked the AST twice), so fine to leave as-is — just flagging in case a variant that accepts the already-resolved identifier feels worthwhile.

|IdentifierWithContext {
identifier,
context,
}| {
identifier.range == range
&& matches!(
context,
IdentifierContext::Expr(_)
| IdentifierContext::FunctionDef { .. }
| IdentifierContext::MethodDef { .. }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClassDef is intentionally excluded here, but it reads like it could be an oversight. Could you add a one-line note explaining why? My understanding is class def-sites don't have the inferred-return problem and their get_type_trace type is already correct, so they don't need the declaration-preserving path — but a comment would make that explicit.

)
},
);

if range.is_empty() || is_identifier_name_range {
return self.get_type_at_preserving_declaration(handle, range.start());
}
self.get_type_trace(handle, range)
Expand Down
66 changes: 66 additions & 0 deletions pyrefly/lib/test/tsp/tsp_interaction/get_type_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,26 @@ use crate::test::tsp::tsp_interaction::object_model::write_pyproject;
fn setup_project(file_content: &str) -> (TspInteraction, String, i32) {
let temp_dir = TempDir::new().unwrap();
write_pyproject(temp_dir.path());
setup_project_in_dir(temp_dir, file_content)
}

fn setup_project_with_pyrefly_config(
file_content: &str,
pyrefly_config: &str,
) -> (TspInteraction, String, i32) {
let temp_dir = TempDir::new().unwrap();
write_pyproject(temp_dir.path());

let pyproject = temp_dir.path().join("pyproject.toml");
let mut content = std::fs::read_to_string(&pyproject).unwrap();
content.push_str("\n[tool.pyrefly]\n");
content.push_str(pyrefly_config);
std::fs::write(pyproject, content).unwrap();

setup_project_in_dir(temp_dir, file_content)
}

fn setup_project_in_dir(temp_dir: TempDir, file_content: &str) -> (TspInteraction, String, i32) {
let test_file = temp_dir.path().join("main.py");
std::fs::write(&test_file, file_content).unwrap();

Expand Down Expand Up @@ -787,6 +806,53 @@ fn test_get_computed_type_function_has_return_type() {
tsp.shutdown();
}

#[test]
fn test_get_computed_type_definition_site_uses_inferred_return_type() {
let code = "\
def plain():
return True

class C:
def method(self):
return 1

@property
def ok(self):
return True

plain
";
let (mut tsp, file_uri, snapshot) = setup_project_with_pyrefly_config(
code,
"untyped-def-behavior = \"check-and-infer-return-type\"\n",
);

for (start_line, start_character, end_line, end_character, label) in [
(0, 4, 0, 9, "plain function definition"),
(4, 8, 4, 14, "method definition"),
(8, 8, 8, 10, "property getter definition"),
(11, 0, 11, 5, "plain function reference"),
] {
let result = get_computed_type_range_ok(
&mut tsp,
&file_uri,
start_line,
start_character,
end_line,
end_character,
snapshot,
);
assert_kind(&result, TypeKind::Function);

let return_type = result
.get("returnType")
.unwrap_or_else(|| panic!("Expected returnType for {label}: {result}"));
assert_kind(return_type, TypeKind::Class);
}

tsp.shutdown();
}

#[test]
fn test_get_computed_type_function_has_declaration() {
// Verify that a def-function has a Regular declaration with name
Expand Down
Loading