Take includeDeclaration into account

[DanielNoord]

Summary

Closes #2126

Not sure if just passing the bool around is the best, but I guess it works? 😄

The logic for references might also not be fully idiomatic. So very open to feedback and suggestions.

Test Plan

I copied test_references_cross_file_no_config from the test file as it seems to use a module that is pretty simple. From the expected test output I removed the declaration, and that test now passes.

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

[kinto0]

looks great! now we just need to let people set this

[kinto0]

we probably need to allow this to be configured via the lsp config workspace_configuration_response

[DanielNoord]

Sorry for the late response. I haven't touched the configuration code before.

I think you mean to add it to so it can be set:

pyrefly/pyrefly/lib/lsp/non_wasm/workspace.rs

Lines 283 to 291 in aeb2b10

	struct LspConfig {
	/// Settings we share with the Pylance extension for backwards compatibility
	/// See LspAnalysisConfig's docstring for more details
	#[serde(default, deserialize_with = "deserialize_analysis")]
	analysis: Option<LspAnalysisConfig>,
	python_path: Option<String>,
	/// Settings we've added that are specific to Pyrefly
	pyrefly: Option<PyreflyClientConfig>,
	}

But I'm not quite sure in which section it should be?
Or should it be in Workspace?

Happy to make the change, just don't really know where I should make it 😅

[kinto0]

if we put it in the PyreflyClientConfig, it'll be accessible by LSP via configuration and initializationOptions. see the tip in our wiki here (we probably should make this info easier to understand and find)

[DanielNoord]

@kinto0 Added it, but I'm not 100% confident in the change 😅

Not sure where the config belongs to, and I think with the current change it will default to false whereas main uses true considering it doesn't even look at the argument.

Any pointers are welcome! :)

[kinto0]

I'm sorry, I misunderstood this part! includeDeclaration is only part of references params, so the question is what to do with rename. I think the old behavior is the only correct behavior, since renaming must include declarations otherwise the references would reference the wrong thing

[kinto0]

approving the first commit before I suggested changes. let me try importing + resolving merge conflicts

[meta-codesync]

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

[kinto0]

now that i'm looking at it more directly to resolve conflicts, I'm noticing a few other things:

• local references still include declarations (I added tests for this + fixed it by adding the bool to local references function)
• global references calls local references. we only need this logic in local references functionality. and we don't even have to filter, we just need to avoid explicitly adding the declaration here

i'm just fixing these up since they're quick and i've already resolved the conflicts

[stroxler]

Review automatically exported from Phabricator review in Meta.

[DanielNoord]

Thanks @kinto0

Still getting used to the codebase. Happy to see they trigger further improvements :)

[meta-codesync]

@kinto0 merged this pull request in df10434.