Conversation
…ot-visible, not ambiguous
When resolving an identifier through `use *` with no visible candidate,
`resolve_path_using_use_star` folded the non-pub candidates back into
`module_items_found` and hit its `len() > 1` check, reporting E2087
("Ambiguous path") — but nothing is visible, so the user can't
disambiguate it. This is a visibility error.
Report E2099 (not visible) instead. Since several distinct non-visible
items may share only the name, `ItemNotVisible`'s item becomes
`Option<ModuleItemId>`: a lone candidate is still named, while multiple
candidates are reported without a specific item (just the modules).
Single/direct imports keep their exact message via `Some`.
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryLow Risk Overview Diagnostic formatting was updated to omit a concrete item path when Reviewed by Cursor Bugbot for commit 04b5ed3. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 04b5ed3. Configure here.
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware and orizi).
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware).
Summary
When resolving identifiers through
use *(glob) imports, multiple distinct items from different modules can share the same name. Previously, theItemNotVisiblediagnostic variant required a concreteModuleItemId, which forced the resolver to pick a single item even when multiple non-visible candidates existed. This PR changesItemNotVisibleto holdOption<ModuleItemId>so that ambiguous invisible candidates can be reported without pointing to a specific item.The resolver now tracks non-visible candidates separately (
non_pub_module_items_found) and usesexactly_one()to determine whether a single invisible item or multiple invisible items were found. When exactly one non-visible item exists, it is reported by name. When multiple non-visible items share the name, the diagnostic omits the item name and reports"Item is not visible in this context through any of the modules: ..."instead of"Item \...` is not visible..."`.Type of change
Please check one:
Why is this change needed?
When two glob imports (
use a::*; use b::*;) both bring in a name that is not publicly visible, the old code would attempt to resolve the item in each non-visible module sequentially and report whichever it found first. This could produce a misleading diagnostic naming one specific item when in fact multiple invisible candidates existed. The fix correctly handles the ambiguous-invisible case.What was the behavior or documentation before?
When multiple non-visible items shared the same name via glob imports, the diagnostic would arbitrarily name one of them:
Item \some::path` is not visible in this context...`.What is the behavior or documentation after?
When multiple non-visible items share the same name via glob imports, the diagnostic now reads:
Item is not visible in this context through any of the modules: \test::a`, `test::b`.` — omitting the specific item path since no single item can be unambiguously identified.Related issue or discussion (if any)
N/A
Additional context
A new test case (
Testing use star with only invisible candidates) was added todiagnostic_test_data/teststo cover the scenario where all glob-imported candidates for a name are non-visible.