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.

Sign up for GitHub

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

Add diagnostic item for std::iter::Iterator::enumerate #124542

Merged
merged 1 commit into from
May 1, 2024
Merged

Add diagnostic item for std::iter::Iterator::enumerate #124542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,7 @@ symbols! {
enable,
encode,
end,
enumerate_method,
env,
env_CFG_RELEASE: env!("CFG_RELEASE"),
eprint_macro,
Expand Down
1 change: 1 addition & 0 deletions library/core/src/iter/traits/iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,7 @@ pub trait Iterator {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_do_not_const_check]
#[cfg_attr(not(test), rustc_diagnostic_item = "enumerate_method")]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a silly question, but why not(test)?

A quick rg rustc_diagnostic_item library shows lots with, but also lots without, and I don't understand why.

Copy link
Contributor Author

@CBSpeir CBSpeir Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The Rust Compiler Development Guide explains, not including not(test) can cause compilation errors while running some tests. It goes on to say it's okay to add it as a preventative measure for all diagnostic items.

I did not test whether this specific diagnostic item causes any testing compilation errors. I will look into this further to determine if it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran ./x test, which I believe runs all tests given my current configuration. There were no failures with not(test) removed.

Copy link
Member

@lukas-code lukas-code Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The cfg(not(test)) is required in alloc and std, but not in core.

This is because alloc and std contain some unit #[test]s as part of the library crates and the crates therefore have to be compiled twice: Once as a library as a dependency of the test crate (which contains the default test runner) and once as an executable that imports the test crate.

When the crates are compiled twice and the diagnostic items don't have cfg(not(test)), then we get an error that the diagnostic items are defined multiple times. (The same also happens for lang items.)

The core package explicitly doesn't test its library crate and only has integration tests, so the cfg(not(test)) is not required there.

rust/library/core/Cargo.toml

Lines 14 to 16 in 20aa2d8

[lib]
test = false
bench = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since cfg(not(test)) is not required for this diagnostic item, should I remove it? Or is it okay to leave it on as a preventative measure?

fn enumerate(self) -> Enumerate<Self>
where
Self: Sized,
Expand Down
Loading