Skip to content

Checker: report environment before checking namespace #18609

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

Merged
merged 6 commits into from
May 28, 2025

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented May 26, 2025

A follow up for #18519.

It turns out features like the code completion relied on the own environment being reported when checking top level declarations in some cases. This is not an entirely correct approach, but it's likely that there are many similar places where things assumed something close enough being reported.

#18519 has broken cases where a name resolution environment just before checking a namespace was lost (i.e. including previously checked namespaces). This PR adds reporting the environment before checking the current namespace fragment. This may break some other things but it seems it's a step in the right direction.

Copy link
Contributor

github-actions bot commented May 26, 2025

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/10.0.100.md

@auduchinok auduchinok force-pushed the tc-decl-ns-env-sink branch from bcad6fa to cb8e4e9 Compare May 26, 2025 12:35
@auduchinok
Copy link
Member Author

auduchinok commented May 26, 2025

It turns out the same captured environments are used in different kinds of features, even though their requirements are different:

  • code completion requires an environment captured before the symbol checking, i.e. it should include previously checked symbols but it should not be able to resolve a symbol name inside its declaration (unless it's recursive)
  • tooltips may require an environment that was captured after the checking, so it would be possible to resolve a symbol when hovering its declaration with a mouse

These requirements contradict each other. Previously we ignored some of the related small bugs here and there, and #18519 was an attempt at fixing some of them.

What can be done here is one these options:

  • optionally mark the captured environments as pre/post-check and add additional checks when getting environments
    • provides some control to the features
  • capture additional environments at the ranges of the symbol declaration names
    • this is already done for some declarations (e.g. for local values), but not consistently
    • code completion is normally unavailable at these ranges
  • revert Checker: don't capture environment for checked modules #18519 (i.e. report the post-check env again), keep this PR and modify the pre-check env ranges, so they don't capture the name part
    • this is a trade-off that could seemingly allow using pre-check envs in code completion and would fallback to the post-check ones at the name ranges
    • requires calculating body ranges

It's likely that all of these approaches will require fixing features that (accidentally) rely on the current behavior. It may also turn out to be more work to do it consistently across different kinds of declarations.

@auduchinok
Copy link
Member Author

auduchinok commented May 26, 2025

There's another interesting detail. Features like tooltips try to get previously reported symbols at the given range first, so sticking with environments tailored for code completion may be a good (and simple!) approach after all.

The reason some of the FCS tests fail is they provide incorrect arguments like the line text or coordinates, which happened to work with the previous implementation (which seems incorrect).
Updating the arguments in these tests fixes them. I've reused the same {caret} approach as in the code completion tests, so it's now easier to provide the correct data in these tests. We should use it in more places, it makes it much better.

Let's see what does CI say for the rest of the tests.

@auduchinok
Copy link
Member Author

Yay, it's green 🙂

@T-Gro
Copy link
Member

T-Gro commented May 28, 2025

Thanks @auduchinok for the detailed analysis and targeted fix here!

@github-project-automation github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling May 28, 2025
@T-Gro T-Gro merged commit ec503f9 into dotnet:main May 28, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in F# Compiler and Tooling May 28, 2025
@auduchinok auduchinok deleted the tc-decl-ns-env-sink branch May 29, 2025 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants