Skip to content

Fix __unknown__ module names for files without a config#4136

Open
jorenham wants to merge 1 commit into
facebook:mainfrom
jorenham:gh-4132
Open

Fix __unknown__ module names for files without a config#4136
jorenham wants to merge 1 commit into
facebook:mainfrom
jorenham:gh-4132

Conversation

@jorenham

Copy link
Copy Markdown
Contributor

Summary

CLI handles were named without checking the config's fallback_search_path, so in a directory without a config every file was __unknown__, and no symbol FQN could match the public FQNs in pyrefly coverage {check, report} --public-only.
ConfigFile::handle_from_module_path now applies its own fallback_search_path (i.e. what the LSP already did), so that handle_from_module_path_with_fallback is now no longer needed.

Fixes #4132

Test Plan

Regression test added, and verified that pyrefly coverage check --public-only in a directory without pyrefly config now reports the same as without --public-only.

@github-actions

Copy link
Copy Markdown

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

@meta-codesync

meta-codesync Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

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

@stroxler stroxler left a comment

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.

Review automatically exported from Phabricator review in Meta.

@connernilsen connernilsen left a comment

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.

Review automatically exported from Phabricator review in Meta.

@kinto0 kinto0 self-assigned this Jul 14, 2026
@kinto0

kinto0 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

one more question (and an alternate approach suggestion):

what project doesn't work with coverage? I expect projects that don't work on coverage to also have name issues with pyrefly check. can I test it?

We wanted to keep __unknown__ as a means to detect when imports couldn't be determined. by adding fallback_search_path, we support imports that don't work in normal python (for example, subdir1 using a different import root than subdir2).

If we find that this is an issue in coverage and not check, I'd like to limit this change to coverage doing something like #4150

@jorenham

Copy link
Copy Markdown
Contributor Author

what project doesn't work with coverage? I expect projects that don't work on coverage to also have name issues with pyrefly check. can I test it?

Any directory without a config reproduces it:

mkdir demo && cd demo
printf 'def f(x: int) -> int:\n    return x\n\ndef g(x, y):\n    return x + y\n' > a.py
pyrefly coverage check --public-only

On main this reports 0 of 0 typable (with this PR: 3 of 6). Plain pyrefly coverage check and pyrefly check are unaffected.
With pyrefly check it never matches the module name of the handle against anything, and imports already resolve through fallback_search_path either way. So the name only matters in coverage, where the symbol FQNs are matched against the public FQNs.

We wanted to keep __unknown__ as a means to detect when imports couldn't be determined. by adding fallback_search_path, we support imports that don't work in normal python (for example, subdir1 using a different import root than subdir2).

Well, this PR doesn't remove it, it just moves it to ModuleNameWithKind. The names derived from the fallback are tagged fallback rather than guaranteed, and __unknown__ still shows up when even the ancestors can't form a valid module name. That also motivates this general fix here, because the LSP already names handles this way, so CLI and IDE currently disagree.

If we find that this is an issue in coverage and not check, I'd like to limit this change to coverage doing something like #4150

So this is indeed a coverage-only thing. So if check should keep the literal __unknown__, then #4150 is also fine as far as I'm concerned.

@kinto0

kinto0 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

interesting. I had a gap in my understanding related to the downstream effect of this module name. looks like it doesn't change check.

I'll merge your PR here and add a follow-up to remove the conditional on __unknown__ in coverage

@kinto0

kinto0 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

#4157 if you have thoughts. it looks like you added the ModuleName::unknown gate in the first place

(note the PR also includes these changes. you can go to a single commit to see just the new part)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

running coverage check --public-only reports 0 typables in a dir without config

4 participants