Remove unneeded ItemId::Primitive variant

[GuillaumeGomez]

As I mentioned here, I wondered if ItemId::Primitive was actually used for anything. Apparently, it seems so because removing it led to no changes as far as I and tests could see.

r? @notriddle

[rustbot]

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

[GuillaumeGomez]

This change is very ugly though. I wondered if making item_id an Option would have been better but in the end I just left as is. If you have a better idea, I'm all for it. Otherwise I can add a comment.

[bjorn3]

Maybe use index: 0xFFFF_FFFFu32.into() to prevent potential confusion with the crate root?

[GuillaumeGomez]

Wouldn't u32::MAX be better?

[bjorn3]

That would work too :)

[GuillaumeGomez]

Ah so it's u32::MAX - 1.

[bjorn3]

I think it needs to be 0xFFFF_FF00 or less:

rust/compiler/rustc_macros/src/newtype.rs

Lines 84 to 85 in 89e0576

	// shave off 256 indices at the end to allow space for packing these indices into enums
	let max = max.unwrap_or_else(|| Lit::Int(LitInt::new("0xFFFF_FF00", Span::call_site())));

[GuillaumeGomez]

Oh that's a shame. I think I'll go back to 0 since we don't use the DefId for primitives in any case...

[notriddle]

Sentinel DefIds seem like a nasty hack. Rustdoc used to use them sometimes, and stopped because it's a nasty hack.

If the PrimitiveType field of ItemId::Primitive isn't being used, would it make more sense to keep the Primitive ItemId, but have it only contain the CrateNum, like this?

#[derive(Debug, Clone, PartialEq, Eq, Hash, Copy)]
pub(crate) enum ItemId {
    /// A "normal" item that uses a [`DefId`] for identification.
    DefId(DefId),
    /// Identifier that is used for auto traits.
    Auto { trait_: DefId, for_: DefId },
    /// Identifier that is used for blanket implementations.
    Blanket { impl_id: DefId, for_: DefId },
    /// Identifier for primitive types.
    Primitive(CrateNum),
}

[GuillaumeGomez]

At this point we could just keep Primitive, but it was mostly to get rid of an unused variant considering ItemId::Primitive is never used.

[notriddle]

The other option, like you already mentioned, would be to wrap the item_id in an Option. I either one would be cleaner than generating a DefId with false information inside.

[GuillaumeGomez]

Which one do you prefer?

[notriddle]

I don't really like Option for a case like this. It's better than forging a DefId, but it can still be unclear what the reason for its absence is, while ItemId::Primitive is self-explanatory.

I guess I would prefer to keep Primitive, even if it has no member values.

[GuillaumeGomez]

Let's go with that then! I'll make the update tomorrow.

[GuillaumeGomez]

Actually the solution was much simpler: we can use the item's DefId directly since only its krate field is ever used in the case of a primitive. That makes things much simpler. :)

[GuillaumeGomez]

@bors r=notriddle rollup

[bors]

📌 Commit 36c9b49 has been approved by notriddle

It is now in the queue for this repository.

[bors]

🌲 The tree is currently closed for pull requests below priority 999. This pull request will be tested once the tree is reopened.