`Sealed::DEBUG_STR` is publicly accessible

[mkroening]

Background: We use v0.14s PageSize::SIZE_AS_DEBUG_STR in the Hermit loader (src/arch/x86_64/paging.rs#L31), which can be easily replaced.

I interpret the discussion in https://github.com/rust-osdev/x86_64/pull/404 so that the intention was to remove Sealed::DEBUG_STR from the public API.

This issue is to inform you that while the Sealed trait is not nameable, you can still use DEBUG_STR when using generics:

use x86_64::structures::paging::{PageSize, Size4KiB};

/// Compiles just fine.
pub fn generic<S: PageSize>() -> &'static str {
    S::DEBUG_STR
}

/// Compiles just fine.
pub fn concrete() -> &'static str {
    generic::<Size4KiB>()
}

/// Does not compile because `Sealed` is not in scope.
pub fn not_generic() -> &'static str {
    Size4KiB::DEBUG_STR
}

I am not sure if this is actually a rustc issue or not.

[phil-opp]

Thanks for reporting! I definitely see that this is confusing.

I am not sure if this is actually a rustc issue or not.

See https://github.com/rust-lang/rust/issues/67105 and https://github.com/rust-lang/rust/issues/83882. It looks like the is no consensus on what should be the expected behavior yet (forbid access in generic code or allow access in non-generic code too).

[Freax13]

I can't think of a good way to prevent this, other than by making the Sealed trait private. Unfortunately, private supertraits are only allowed starting with Rust 1.74 following RFC 2145. Our current MSRV is 1.59, so that's not an option.

[phil-opp]

I think it should be possible to move the DEBUG_STR constant to a separate, private trait. Looking into it right now.

[phil-opp]

It kind of works, but it leads to a lot of additional bounds on the new private trait: https://github.com/rust-osdev/x86_64/commit/a5585ef240c553a2a13b19dc34fe864f76a12180 . I don't think that this is an improvement...

[mkroening]

I guess the alternative would be to commit to having this part of the public API even though that was not what we wanted in https://github.com/rust-osdev/x86_64/pull/404, right?

[phil-opp]

Yes, but in that case we should probably redefine the DEBUG_STR constant in the respective child traits. Otherwise it's not included in the docs and only accessible using generics.

[Freax13]

https://github.com/rust-osdev/x86_64/blob/a5585ef240c553a2a13b19dc34fe864f76a12180/src/structures/paging/mapper/mod.rs#L113

This makes it impossible for users to add their own generic implementations because they can't name DebugOutput, doesn't it?

[phil-opp]

Yes it does, at least in the current implementation. Maybe it's possible to avoid this somehow, but it's probably not worth the trouble. So exposing DEBUG_STR is probably the better solution.

[mkroening]

https://github.com/rust-osdev/x86_64/blob/a5585ef240c553a2a13b19dc34fe864f76a12180/src/structures/paging/mapper/mod.rs#L113

This makes it impossible for users to add their own generic implementations because they can't name DebugOutput, doesn't it?

Isn't that what you meant with https://github.com/rust-osdev/x86_64/pull/404 anyway?

Users should never implement this trait themselves. [...]

[Freax13]

https://github.com/rust-osdev/x86_64/blob/a5585ef240c553a2a13b19dc34fe864f76a12180/src/structures/paging/mapper/mod.rs#L113

This makes it impossible for users to add their own generic implementations because they can't name DebugOutput, doesn't it?

Isn't that what you meant with #404 anyway?

Users should never implement this trait themselves. [...]

404 is meant to prevent users from implementing PageSize on their own types. What I meant with my comment was that with the changes in a5585ef240c553a2a13b19dc34fe864f76a12180, it would be impossible for users to create generic implementations of Mapper<S>.