fix Duplicate errors for undefined name in __all__ #2655

[asukaminato0721]

Summary

Fixes #2655

If a module explicitly declares __all__ and that list contains a missing name, Pyrefly now reports the error on the defining module’s __all__, not again on every from foo import * importer.

The importer still gets the wildcard binding as Any(Error), but the duplicate Could not import ... from foo diagnostic is suppressed.

Test Plan

update test

[Copilot]

Pull request overview

Suppresses duplicate diagnostics for missing names listed in a module’s explicit __all__ by reporting the error only on the defining module (not on every from foo import * importer), while keeping the imported binding as Any(Error).

Changes:

• Updates star-import binding to suppress MissingModuleAttribute diagnostics when the exporting module has an explicit __all__.
• Adds a has_explicit_dunder_all query to the export lookup API and implements it for transaction/export providers.
• Updates/imports incremental and imports tests to expect no importer-side diagnostic.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pyrefly/lib/test/incremental.rs	Adjusts incremental test to assert no error is emitted in the star-importing module.
pyrefly/lib/test/imports.rs	Updates import test to stop expecting duplicate importer-side error.
pyrefly/lib/state/state.rs	Implements has_explicit_dunder_all for the transaction lookup layer.
pyrefly/lib/export/exports.rs	Extends LookupExport trait + adds Exports::has_explicit_dunder_all.
pyrefly/lib/binding/stmt.rs	Suppresses importer-side missing-name errors for explicit __all__ star imports.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

check_ignoring_expectations doesn’t assert that the defining-module __all__ error is actually cleared after adding y. This can let regressions slip in where the importer stays clean but foo continues to report an error. Consider explicitly collecting errors for foo (and optionally main) after the update and asserting they’re empty, or use a check path that validates “no diagnostics” for updated modules.

[Copilot]

This test no longer asserts that the missing-name-in-__all__ diagnostic is emitted on the defining module; it only asserts the duplicate importer-side diagnostic is gone. To better lock in the intended behavior, add/ensure an expectation in the exporting module fixture (e.g., on the __all__ assignment) so the test validates “error moved to exporter” rather than “error disappeared.”

[Copilot]

get_wildcard(m) and has_explicit_dunder_all(m) likely both consult the same exports data, so this can introduce an extra lookup per star import. If feasible, consider fetching exports once (or extending the wildcard lookup to also return whether __all__ is explicit) to avoid repeated work on import-heavy modules.

[yangdanny97]

Thanks for the PR,

So I think we need to be careful about differentiating these two possibilities, and there should be tests verifying that they behave differently.

1. __all__ contains some bad name, we try to import that bad name -> error on the file w/ __all__, do not error in the importing file
2. __all__ contains some bad name, we try to import a different name that's not in __all__ at all -> we should error in both files

I believe this PR handles case 2 incorrectly, since it suppresses the error only based on whether there's an explicit __all__, not based on what name we were actually importing.

To fix the issue, we might need to update the way we represent exports.

[meta-codesync]

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

[kinto0]

Review automatically exported from Phabricator review in Meta.

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

scipy (https://github.com/scipy/scipy)
- ERROR scipy/special/tests/test_spfun_stats.py:6:36-48: Could not import `multigammaln` from `scipy.special` [missing-module-attribute]
- ERROR scipy/stats/_multivariate.py:11:42-54: Could not import `multigammaln` from `scipy.special` [missing-module-attribute]
- ERROR scipy/stats/tests/test_multivariate.py:38:27-39: Could not import `multigammaln` from `scipy.special` [missing-module-attribute]

[github-actions]

Primer Diff Classification

✅ 1 improvement(s) | 1 project(s) total | -3 errors

1 improvement(s) across scipy.

Project	Verdict	Changes	Error Kinds	Root Cause
scipy	✅ Improvement	-3	missing-module-attribute	The changes to binding/stmt.rs (handling `ExportLocatio...

Detailed analysis

✅ Improvement (1)

scipy (-3)

This is an improvement in error reporting quality. Instead of reporting 'Could not import X' at every import site when X is listed in __all__ but missing, pyrefly now reports the error once at the __all__ definition. This reduces noise and points to where the fix is needed. The PR test case test_dunder_all_star_import_missing_definition_no_import_error confirms this is the intended behavior.
Attribution: The changes to binding/stmt.rs (handling ExportLocation::InvalidDunderAll) and export/exports.rs (tracking invalid __all__ entries) caused import sites to no longer report errors for names that are in __all__ but missing from the module

---

Was this helpful? React with 👍 or 👎

_{Classification by primer-classifier (1 LLM)}

[yangdanny97]

sorry for the churn here, Kyle's review comments weren't exported here - I've patched some stuff on top to simplify things but the behavior is different from this impl, will try to reconcile it before the merge.