Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unneeded ItemId::Primitive variant #106628

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

GuillaumeGomez
Copy link
Member

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 rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2023

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

cc @camelid

@@ -600,7 +600,7 @@ fn build_module_items(
items.push(clean::Item {
name: None,
attrs: Box::new(clean::Attributes::default()),
item_id: ItemId::Primitive(prim_ty, did.krate),
item_id: ItemId::DefId(DefId { index: 0u32.into(), krate: did.krate }),
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't u32::MAX be better?

Copy link
Member

Choose a reason for hiding this comment

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

That would work too :)

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

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

@bjorn3
Copy link
Member

bjorn3 commented Jan 9, 2023

I think it needs to be 0xFFFF_FF00 or less:

// 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
Copy link
Member Author

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
Copy link
Contributor

notriddle commented Jan 9, 2023

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

Which one do you prefer?

@notriddle
Copy link
Contributor

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
Copy link
Member Author

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

@GuillaumeGomez
Copy link
Member Author

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
Copy link
Member Author

@bors r=notriddle rollup

@bors
Copy link
Contributor

bors commented Jan 10, 2023

📌 Commit 36c9b49 has been approved by notriddle

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 10, 2023

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

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2023
Rollup of 14 pull requests

Successful merges:

 - rust-lang#105194 (Add comment to cleanup_kinds)
 - rust-lang#106521 (remove E0280)
 - rust-lang#106628 (Remove unneeded ItemId::Primitive variant)
 - rust-lang#106635 (std sync tests: better type name, clarifying comment)
 - rust-lang#106642 (Add test for rust-lang#106062)
 - rust-lang#106645 ([RFC 2397] Initial implementation)
 - rust-lang#106653 (Fix help docs for -Zallow-features)
 - rust-lang#106657 (Remove myself from rust-lang/rust reviewers)
 - rust-lang#106662 (specialize impl of `ToString` on `bool`)
 - rust-lang#106669 (create helper function for `rustc_lint_defs::Level` and remove it's duplicated code)
 - rust-lang#106671 (Change flags with a fixed default value from Option<bool> to bool)
 - rust-lang#106689 (Fix invalid files array re-creation in rustdoc-gui tester)
 - rust-lang#106690 (Fix scrolling for item declaration block)
 - rust-lang#106698 (Add compiler-errors to some trait system notification groups)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b0ffc11 into rust-lang:master Jan 11, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 11, 2023
@GuillaumeGomez GuillaumeGomez deleted the rm-itemid-primitive branch January 11, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants