Skip to content

Conversation

@ShoyuVanilla
Copy link
Member

Fixes #18308

The original issue is quite complicated.
polar-rs monorepo has the following structure;

// crate polars-core
pub mod prelude {
    pub use crate::datatype::DataType;
}

// crate polars-ops
pub mod frame {
    pub use join::*;

    pub mod join {
        pub use hash_join::*;
        use polars_core::prelude::*;

        mod hash_join {
            pub use super::*;
        }
    }
}
  1. The glob import pub use super::* inside mod hash_join reexports the visibility of the glob import use polars_core::prelude::* inside the mod join as "public", which was previously "module(join)"
  2. The glob import pub use hash_join::* inside mod join overrides visibility of the previous visibility of the glob import use polars_core::prelude::* with "public" in 1., which was previously "module(join)"

The problematic step is 1., as rustc doesn't allow such re-export with E0603.
This PR prevent such "more visible" re-export by taking minimum of resolved item visibility and the re-export visibility

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2024
mod bar {
mod foo { pub(super) struct S; }
mod foo { pub(crate) struct S; }
pub(crate) use foo::S as U;
Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case was wrong. It emits E0364 on rustc

mod bar {
    mod foo { pub(super) struct S; }
    pub(crate) use foo::S as U;
}

fn main() {}
error[E0364]: `S` is private, and cannot be re-exported
 --> src/main.rs:3:20
  |
3 |     pub(crate) use foo::S as U;
  |                    ^^^^^^^^^^^
  |
note: consider marking `S` as `pub` in the imported module
 --> src/main.rs:3:20
  |
3 |     pub(crate) use foo::S as U;
  |                    ^^^^^^^^^^^

@ShoyuVanilla
Copy link
Member Author

BTW I spent most time on making a MCVE, as the original issue was quite hard to reproduce outside the pola-rs repository😅
Both pub use super::* and the importing order mattered

@Veykril Veykril added this pull request to the merge queue Oct 24, 2024
Merged via the queue into rust-lang:master with commit b12fead Oct 24, 2024
9 checks passed
@orlp
Copy link

orlp commented Oct 24, 2024

Great work! Sorry for the large reproduction, I tried a bit to make it minimal but wasn't successful 😓.

@ShoyuVanilla
Copy link
Member Author

Great work! Sorry for the large reproduction, I tried a bit to make it minimal but wasn't successful 😓.

No worries 😅 The root causes were quite interwoven with each other and I just managed to made a MVCE with custom r-a build with extra tracings.
Thank you for maintaining polars, a great library for data manipulations

@ShoyuVanilla ShoyuVanilla deleted the issue-18308 branch October 24, 2024 09:58
@lnicola
Copy link
Member

lnicola commented Oct 25, 2024

This seems to have caused a regression in pattern type inference:

image

And in unknown types:

image

@ShoyuVanilla
Copy link
Member Author

This seems to have caused a regression in pattern type inference:

Oh, I'll check about this

@ShoyuVanilla
Copy link
Member Author

I ran cargo run --release -p rust-analyzer -- unresolved-references . and it seems that r-a isn't resolving the following layout things since this PR 🤔

layout::IntegerType::Pointer(sign) => match sign {

@ShoyuVanilla
Copy link
Member Author

MCVE:

//- /main.rs crate:main deps:test_crate
use test_crate::foo; // not resolving this!

use foo::Foo;

//- /lib.rs crate:test_crate deps:test_crate2
extern crate test_crate2;

pub use test_crate2 as foo;

//- /lib2.rs crate:test_crate2
pub struct Foo;

It seems that I should exempt min visibility for extern crate, as it seems to be exceptional in rustc 🤔

ChayimFriedman2 added a commit to ChayimFriedman2/rust-analyzer that referenced this pull request Dec 4, 2024
When a glob import overriding the visibility of a previous glob import was not properly resolved when the items are only available in the next fixpoint iteration.

The bug was hidden until rust-lang#18390.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import suggestions do not take into account visibility

5 participants