seal off the PageSize trait

[Freax13]

Users should never implement this trait themselves. This also allows us to add more supertraits to PageSize and NotGiantPageSize without introducing a major breaking change.

This is a breaking change (though I really hope users haven't been doing this previously).

[josephlr]

We already have a sealed module here:

x86_64/src/instructions/port.rs

Lines 69 to 73 in a143355

	mod sealed {
	pub trait Access {
	const DEBUG_NAME: &'static str;
	}
	}

Could we have a common place to put all sealed traits or try to reuse stuff?

[josephlr]

Given that both Access and PageSize have need for some sort of &'static str (Access::DEBUG_NAME and PageSize::SIZE_AS_DEBUG_STR) for implementing Debug, we could do something like:

In lib.rs:

pub(crate) mod sealed {
    pub trait Sealed {
        const DEBUG_STR: &'static str;
    }
}

Then both structures::paging::PageSize and instructions::port::{PortReadAccess, PortWriteAccess} could use the same sealed::Sealed trait.

[Freax13]

Given that both Access and PageSize have need for some sort of &'static str (Access::DEBUG_NAME and PageSize::SIZE_AS_DEBUG_STR) for implementing Debug, we could do something like:

In lib.rs:

pub(crate) mod sealed {
    pub trait Sealed {
        const DEBUG_STR: &'static str;
    }
}

Then both structures::paging::PageSize and instructions::port::{PortReadAccess, PortWriteAccess} could use the same sealed::Sealed trait.

This would also make DEBUG_STR inaccessible to users. Do we consider DEBUG_STR to be an implementation detail or part of the public API? A quick search on github code search doesn't turn up any code actually using this, so we probably won't break much code if we decide to hide this.

[josephlr]

This would also make DEBUG_STR inaccessible to users. Do we consider DEBUG_STR to be an implementation detail or part of the public API? A quick search on github code search doesn't turn up any code actually using this, so we probably won't break much code if we decide to hide this.

I definitely would prefer it being an implementation detail rather than something that we commit to as part of our stable API.

[phil-opp]

A few thoughts on this:

• I don't think that there is much value in using a unified sealed module
  • It doesn't show up in the docs anyway since it's a private module
  • There is no complexity in the Sealed trait, it's just an empty trait
  • (This is not a strong opinion though, I'm fine with a unified sealed module too.)
• I'm fine with hiding the DEBUG string constants from the public API if no one is using them
• Sealing the PageSize trait sounds reasonable to me. However, I'm curious what kind of misuse we prevent this way. Is there a way to cause UB with the current implementation? (If so, we should fix this on the current version too.)
• What about the NotGiantPageSize trait? We should probably seal it too, right?

[Freax13]

I definitely would prefer it being an implementation detail rather than something that we commit to as part of our stable API.

That's fine with me.

• Sealing the PageSize trait sounds reasonable to me. However, I'm curious what kind of misuse we prevent this way. Is there a way to cause UB with the current implementation? (If so, we should fix this on the current version too.)

I'm not aware of any UB that can be caused by faking a PageSize in this crate, but I've come across this in my own code.

• What about the NotGiantPageSize trait? We should probably seal it too, right?

NotGiantPageSize wouldn't need to be sealed off because its parent trait PageSize is already sealed off.

[josephlr]

Looks reasonable to me modulo minor nits.

[Qix-]

Any idea on a release for this change? I'd rather not have to worry about non-4kib aligned page size types 😅 since generic_const_exprs is a minefield feature, compile-time checks for making sure that particular invariant is true is resulting in some hairy code whereby sealing this trait would prevent any misuse.