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 #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 rust-lang/rust#67105 and rust-lang/rust#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: a5585ef . 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 #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]

x86_64/src/structures/paging/mapper/mod.rs

Line 113 in a5585ef

pub trait Mapper<S: PageSize + DebugOutput> {

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]

x86_64/src/structures/paging/mapper/mod.rs

Line 113 in a5585ef

pub trait Mapper<S: PageSize + DebugOutput> {

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. [...]

[Freax13]

x86_64/src/structures/paging/mapper/mod.rs

Line 113 in a5585ef

pub trait Mapper<S: PageSize + DebugOutput> {

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 a5585ef, it would be impossible for users to create generic implementations of Mapper<S>.