Skip to content
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

unexpected behavior of pub(in other_module) #109657

Open
lcnr opened this issue Mar 27, 2023 · 7 comments
Open

unexpected behavior of pub(in other_module) #109657

lcnr opened this issue Mar 27, 2023 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Mar 27, 2023

mod parent {
    mod this {
        pub(in super::sibling) fn foo() {}
    }
    
    mod sibling {
        fn bar() {
            super::this::foo();
        }
    }
}

results in

error[E0433]: failed to resolve: could not find `sibling` in `super`
 --> src/lib.rs:3:23
  |
3 |         pub(in super::sibling) fn foo() {}
  |                       ^^^^^^^ could not find `sibling` in `super`

this error feels very weird 🤔

I see the following possible fixes here:

  • this is a bug with name resolution and should compile
  • the in super::sibling error should be changed to explain what's going on here and why that doesn't work

found via #109511 (comment)

cc @petrochenkov in case you know more about what's going on here.

@lcnr lcnr added the C-bug Category: This is a bug. label Mar 27, 2023
@mu001999
Copy link
Contributor

The Rust Reference says:

pub(in path) makes an item visible within the provided path. path must be an ancestor module of the item whose visibility is being declared.

@lcnr
Copy link
Contributor Author

lcnr commented Mar 27, 2023

so I guess the fix is

  • the in super::sibling error should be changed to explain what's going on here and why that doesn't work

1 similar comment
@lcnr

This comment was marked as duplicate.

@nandesu-utils
Copy link

so I guess the fix is

  • the in super::sibling error should be changed to explain what's going on here and why that doesn't work

Shouldn't it change to error of declaring an item with pub(in super::sibling) where the path is surely not an ancestor? Otherwise it doesn't really make sense to delay the error until trying to use the item?

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 27, 2023

Resolution for paths inside pub(...) happens very early (similarly to macro and import paths) and mod sibling is just not yet put into the name resolution structures when pub(in super::sibling) is resolved.

For paths in macro calls and reexports we would put such unresolved path into a queue to try resolving it later, because for macros and imports it's important for correctness.

For paths in visibilities nobody cared to organize such a queue with retries, because it would complicate things and would only affect diagnostics but not correctness, and non crate or super paths are rare there anyway.
So the diagnostics are suboptimal.

@petrochenkov
Copy link
Contributor

I remember that there's an older duplicate for this issue, but can't find it right now.

@petrochenkov
Copy link
Contributor

mod sibling is just not yet put into the name resolution structures when pub(in super::sibling) is resolved

You could notice the difference in diagnostics if you swap the order between mod this and mod sibling.

@lcnr lcnr added A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. and removed C-bug Category: This is a bug. labels Mar 29, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants