Verify overloaded method name displays in hover trace

[aaron-ang]

Close #1416.

Apparently, the issue of the method name not displaying has been fixed. I added a test case to verify this, and also refactored OverloadedCallee to an enum to better distinguish between resolved (show one) vs candidate (show all) traces.

[kinto0]

thanks for the contribution! i'm trying to assign you to the issue but it's not letting me put your name in. maybe you need to comment no that issue?

[kinto0]

it doesn't seem like it displays overloaded method name for everything. i'd expect to see __eq__ here

[aaron-ang]

The hover_type test suite reports the type only, so the code changes made this diff.

pyrefly/pyrefly/lib/test/lsp/hover_type.rs

Lines 16 to 22 in f3d2477

	fn get_test_report(state: &State, handle: &Handle, position: TextSize) -> String {
	if let Some(t) = state.transaction().get_type_at(handle, position) {
	format!("Hover Result: `{t}`")
	} else {
	"Hover Result: None".to_owned()
	}
	}

[aaron-ang]

The test cases in hover.rs verify the display.

[kinto0]

I still think this is probably a regression. users don't want to see BoundMethod in the hover type

[aaron-ang]

pyrefly/pyrefly/lib/test/lsp/hover_type.rs

Lines 455 to 489 in ffa1a38

	fn overloaded_methods_test() {
	let code = r#"
	from typing import overload
	class Foo:
	@overload
	def overloaded_meth(self, a: str) -> bool: ...
	@overload
	def overloaded_meth(self, a: int, b: bool) -> str: ...
	def overloaded_meth(self):
	pass
	foo = Foo()
	foo.overloaded_meth("")
	# ^
	foo.overloaded_meth(1, True)
	# ^
	foo.overloaded_meth(False)
	# ^
	"#;
	let report = get_batched_lsp_operations_report_allow_error(&[("main", code)], get_test_report);
	assert_eq!(
	r#"
	# main.py
	13 | foo.overloaded_meth("")
	^
	Hover Result: `(self: Foo, a: str) -> bool`
	15 | foo.overloaded_meth(1, True)
	^
	Hover Result: `(self: Foo, a: int, b: bool) -> str`
	17 | foo.overloaded_meth(False)
	^
	Hover Result: `BoundMethod[Foo, Overload[(self: Foo, a: str) -> bool, (self: Foo, a: int, b: bool) -> str]]`

@kinto0 an existing test expects BoundMethod on hover when the method does not match any of the overloaded function signatures. I may have misinterpreted when it should be shown. Can I get some clarification here?

[aaron-ang]

@kinto0 I've changed OverloadCallee into an enum to decouple the two cases. Tracing the call stack, record_overload_trace_from_type is used to insert resolved dunder methods, so I named one of the variants as such.

[kinto0]

in general, every time we export this Type type, there's a lot of overhead. we probably want to only export the necessary part. in this case, you only need the callable + the name. in another change, we might also want "type" to make the method change but I see that as another PR to "make hover for methods say (method) instead of (attribute).

thoughts @stroxler ?

[aaron-ang]

rebased

[kinto0]

looks like some tests are still failing

[aaron-ang]

looks like some tests are still failing

@kinto0 I have fixed my test cases.

[meta-codesync]

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D86531225.

[kinto0]

hey, thanks for keeping it updated with all our changes coming in. this is a lot of work.

I still am not 100% sure we want to expose Type everywhere. It's not needed for this task, since the test passes without it. If you still want to expose type, what if we split that into another PR where we can weigh the pros/cons? fwiw, I tested the performance on the IG codebase (20+MLOC) and it didn't make a huge difference.

happy to merge this right now with just the test addition

[kinto0]

this test actually passes before your change.... maybe this feature is done in the meantime or the bug report was incorrect

what if you split this into two PRs:

• just this test which closes hover on operator missing the magic method name and highlight #1416
• follow-up + new issue for the rest of what this diff does (expose type, show (method) different from (attribute)) on hover

[aaron-ang]

After I rebased this branch onto main, the test failed the assertion:

<(method) __matmul__: def __matmul__(
>(attribute) __matmul__: (
    self: Matrix,
    other: Matrix
<) -> Matrix: ...
>) -> Matrix

[aaron-ang]

If it's ok for the method to be named as an attribute for now, I will refactor the PR to reflect this.

[aaron-ang]

I also want to add that I was able to reproduce the issue on my machine while working on it initially. So the bug report is legit, and the issue may have been fixed elsewhere afterwards.

[kinto0]

I still think this is probably a regression. users don't want to see BoundMethod in the hover type

[kinto0]

thanks for the split. I like your refactor too

[stroxler]

Review automatically exported from Phabricator review in Meta.

[meta-codesync]

@kinto0 merged this pull request in 4ee45a9.