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

Improve assert_matches! documentation #121732

Merged

Conversation

Voultapher
Copy link
Contributor

@Voultapher Voultapher commented Feb 28, 2024

This new documentation tries to limit the impact of the conceptual pitfall, that the if guard relaxes the constraint, when really it tightens it. This is achieved by changing the text and examples. The previous documentation also chose a rather weird and non-representative example for the if guard, that made it needlessly complicated to understand.

This new documentation tries to avoid to limit the impact of the
conceptual pitfall, that the if guard relaxes the constraint, when
really it tightens it. This is achieved by changing the text and
examples. The previous documentation also chose a rather weird and
non-representative example for the if guard, that made it needlessly
complicated to understand.
@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 28, 2024
@rust-log-analyzer

This comment has been minimized.

@Voultapher
Copy link
Contributor Author

Not sure I understand the CI error:

macro_rules named matches exists in this crate, but it is not in scope at this link's location

It's the same module, and previously

[\`assert!`]

worked, which is also in the same module. Is this a feature thing?

@jieyouxu
Copy link
Member

jieyouxu commented Feb 28, 2024

Not sure I understand the CI error:

macro_rules named matches exists in this crate, but it is not in scope at this link's location

It's the same module, and previously

[\`assert!`]

worked, which is also in the same module. Is this a feature thing?

Is it because of declarative macro definition rules? As in, declarative macro matches is defined after you reference it in assert_matches's docs (although that would be a surprising behavior in terms of docs...). Does it work if you write something like [`matches`](crate::matches)

@Voultapher
Copy link
Contributor Author

Voultapher commented Feb 28, 2024

Indeed that's it. Didn't know the declaration order also matters for the doc comments. Here is small reproducer:

macro_rules! start {
    () => {};
}

/// [`start!`]
/// [`end!`]
macro_rules! mid {
    () => {};
}

macro_rules! end {
    () => {};
}

fn main() {}
$ cargo doc
[...]
note: `macro_rules` named `end` exists in this crate, but it is not in scope at this link's location

Kind of sad, because it means there is no general way to cross link macros :/ I tried [matches](crate::matches) but that also doesn't work.

@jieyouxu
Copy link
Member

Kind of sad, because it means there is no general way to cross link macros :/ I tried matches but that also doesn't work.

Maybe submit an enhancement request issue for this to the rustdoc team, because I feel like it probably should work? Although I can see not wanting to change the behavior to stay consistent with how name resolution works in rustc as well.

@cuviper
Copy link
Member

cuviper commented Feb 28, 2024

Here is small reproducer:

FWIW, that does work if you add #[macro_export] on end!.

@Voultapher
Copy link
Contributor Author

FWIW, that does work if you add #[macro_export] on end!.

The stdlib matches macro is already marked #[macro_export], so unfortunately it doesn't work there. Maybe something to do with the new macro syntax for the other macros?

@kadiwa4
Copy link
Contributor

kadiwa4 commented Feb 29, 2024

#[macro_export] adds the macro to the crate root as an item, but the doc comment you edited is in macros and not the crate root. IIUC, that's why you have to use a qualified path in your PR but not in the small reproducer.

Another way to make a macro an item (but not in the crate root) is this:

macro_rules! end {
    () => {};
}
use end;

I'm not suggesting that for your PR; but that's what people should do in other crates where they want to have a macro be an item in the local module.

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Looks good -- I just have some comma suggestions.

library/core/src/macros/mod.rs Outdated Show resolved Hide resolved
library/core/src/macros/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

You missed the commas that I removed, which do not belong before the preposition. Also, now that I look again, the phrase "meet expectations" should be plural, or else we could more directly say "did not match the pattern."

library/core/src/macros/mod.rs Outdated Show resolved Hide resolved
library/core/src/macros/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Josh Stone <cuviper@gmail.com>
@Voultapher
Copy link
Contributor Author

Good catch. Should be fixed now.

@cuviper
Copy link
Member

cuviper commented Mar 3, 2024

@bors r+ rollup

@cuviper
Copy link
Member

cuviper commented Mar 4, 2024

Was the bot asleep?

@bors r+ rollup

@cuviper
Copy link
Member

cuviper commented Mar 4, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2024

📌 Commit c45f0a9 has been approved by cuviper

It is now in the queue for this repository.

@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 Mar 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 4, 2024
…documentation, r=cuviper

Improve assert_matches! documentation

This new documentation tries to limit the impact of the conceptual pitfall, that the if guard relaxes the constraint, when really it tightens it. This is achieved by changing the text and examples. The previous documentation also chose a rather weird and non-representative example for the if guard, that made it needlessly complicated to understand.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#120976 (constify a couple thread_local statics)
 - rust-lang#121683 (Fix LVI tests after frame pointers are enabled by default)
 - rust-lang#121703 (Add a way to add constructors for `rustc_type_ir` types)
 - rust-lang#121732 (Improve assert_matches! documentation)
 - rust-lang#121928 (Extract an arguments struct for `Builder::then_else_break`)
 - rust-lang#121939 (Small enhancement to description of From trait)
 - rust-lang#121968 (Don't run test_get_os_named_thread on win7)
 - rust-lang#121969 (`ParseSess` cleanups)
 - rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.)
 - rust-lang#121994 (Update platform-support.md with supported musl version)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#120976 (constify a couple thread_local statics)
 - rust-lang#121683 (Fix LVI tests after frame pointers are enabled by default)
 - rust-lang#121703 (Add a way to add constructors for `rustc_type_ir` types)
 - rust-lang#121732 (Improve assert_matches! documentation)
 - rust-lang#121928 (Extract an arguments struct for `Builder::then_else_break`)
 - rust-lang#121939 (Small enhancement to description of From trait)
 - rust-lang#121968 (Don't run test_get_os_named_thread on win7)
 - rust-lang#121969 (`ParseSess` cleanups)
 - rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.)
 - rust-lang#121994 (Update platform-support.md with supported musl version)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 008ab33 into rust-lang:master Mar 5, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
Rollup merge of rust-lang#121732 - Voultapher:improve-assert_matches-documentation, r=cuviper

Improve assert_matches! documentation

This new documentation tries to limit the impact of the conceptual pitfall, that the if guard relaxes the constraint, when really it tightens it. This is achieved by changing the text and examples. The previous documentation also chose a rather weird and non-representative example for the if guard, that made it needlessly complicated to understand.
@Voultapher Voultapher deleted the improve-assert_matches-documentation branch March 5, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants