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

rustdoc: make doctest span tweak a 2024 edition change #132210

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

notriddle
Copy link
Contributor

Fixes #132203

This is a compatibility hack, because I think the new behavior is better. When an A include_str! B, and B include_str! C, the path to C should be resolved relative to B, not A. That's how include! itself works, so that's how include_str! with should work.

Fixes rust-lang#132203

This is a compatibility hack, because I think the new behavior is better.
When an A `include_str!` B, and B `include_str!` C, the path to C should
be resolved relative to B, not A. That's how `include!` itself works,
so that's how `include_str!` with should work.
@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2024

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 27, 2024
@notriddle notriddle added the A-edition-2024 Area: The 2024 edition label Oct 27, 2024
@GuillaumeGomez
Copy link
Member

Nice, thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 27, 2024

📌 Commit 1819b4f has been approved by GuillaumeGomez

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 Oct 27, 2024
@compiler-errors
Copy link
Member

Should this not be treated like any other edition 2024 change then, with a migration lint (or at least a best-effort attempt at detecting cases where users may be affected) and a section in the edition 2024 book? This seems a somewhat quick decision to gate this behind edition 2024, and presumably we shouldn't leave users in the dark when they're upgrading and their code suddenly breaks. cc @traviscross

@notriddle
Copy link
Contributor Author

Should this not be treated like any other edition 2024 change then, with a migration lint (or at least a best-effort attempt at detecting cases where users may be affected) and a section in the edition 2024 book?

This should definitely be in the edition 2024 book.

As for the lint, I couldn't think of a good way to suggest a fix. There's no way to fix it before you migrate, and after you migrate, there already is a suggestion, as you can see in the crater reports.

Rustdoc doctest nested include_str! change

🚧 The 2024 Edition has not yet been released and hence this section is still "under construction".
More information may be found at #132210.

Summary

When a doctest is included with include_str!, if that doctest itself
also uses include! / include_str! / include_bytes!, the path is
resolved relative to the Markdown file, rather than the Rust
source file.

Details

Prior to the 2024 edition, adding documentation with
#[doc=include_str!("path/file.md")] didn't carry span information
into the doctests. As a result, if the Markdown file is in a different
directory than the source, any include!ed paths need to be relative
to the Rust file.

For example, consider a library crate with these files:

  • Cargo.toml
  • README.md
  • src/
    • lib.rs
  • examples/
    • data.bin

Let lib.rs contain this:

#![doc=include_str!("../README.md")]

And assume this README.md file:

```
let _ = include_bytes!("../examples/data.bin");
//                      ^^^ notice this
```

Prior to the 2024 edition, the path in README.md needed to be
relative to the lib.rs file. In 2024 and later, it is now relative
to README.md iself.

Migration

After migrating, you'll see the following error:

error: couldn't read `../examples/data.bin`: No such file or directory (os error 2)
 --> src/../README.md:2:24
  |
2 | let _ = include_bytes!("../examples/data.bin");
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = note: this error originates in the macro `include_bytes` (in Nightly builds, run with -Z macro-backtrace for more info)
help: there is a file with the same name in a different directory
  |
2 | let _ = include_bytes!("examples/data.bin");
  |                        ~~~~~~~~~~~~~~~~~~~

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2024
…k, r=GuillaumeGomez

rustdoc: make doctest span tweak a 2024 edition change

Fixes rust-lang#132203

This is a compatibility hack, because I think the new behavior is better. When an A `include_str!` B, and B `include_str!` C, the path to C should be resolved relative to B, not A. That's how `include!` itself works, so that's how `include_str!` with should work.
@bors
Copy link
Contributor

bors commented Oct 27, 2024

⌛ Testing commit 1819b4f with merge 466a6fa...

@traviscross
Copy link
Contributor

@bors r-

(Edition-related. Comment to follow.)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 27, 2024
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 27, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2024
@traviscross
Copy link
Contributor

traviscross commented Oct 27, 2024

While this seems a sensible change, it is very late to be adding an edition item. If this is to go into Rust 2024 at all, we need to follow some process to ensure this is ready for the edition before the change itself lands. Here's what needs to happen:

  • File an edition tracking issue for the change. I've done this:
  • Complete the other required steps for an edition item. In particular:
    • Confirm whether or not there is some edition migration lint needed here, and if there is, implement it.
    • Write up a chapter for the edition guide and have it approved.
    • Confirm whether there are any changes needed to the Reference, and if so, put up that PR.
    • Confirm that this is a decision by the @rust-lang/rustdoc team to request a Rust 2024 edition item.
      • I don't see an FCP or other similar consensus decision, but maybe this was discussed elsewhere.
      • Alternatively, if this is an executive decision by the team lead acting on behalf of the team, that's fine too; it should just be stated explicitly.
    • Confirm with the edition team this can be accepted for the edition at the point these things are ready.
      • In particular, I want sign-off from @ehuss that this late change does not worry him too much with respect to how we test the edition with crater.
    • Then merge this PR landing the change in Rust 2024.

Hopefully this list doesn't seem too daunting. On net, I probably would like to see this happen. But there's some catch-up needed here if this is going to happen, and these items will need to all happen within a very short time -- days, not weeks.

Looking at the breakage, one alternative that the rustdoc team might want consider here is whether this bug might be fixed without making this an edition item at all. Perhaps there is a way to issue a FCW if it seems a crate is relying on the bug? Then PRs could be submitted to the affected crates. After some time, then perhaps the change could be landed (in all editions).

Please let us know what you want to do here, in particular if you do plan to quickly complete the steps needed to make this an edition item.

@traviscross
Copy link
Contributor

The edition guide change is merged. There are no edition migration lints for this. @GuillaumeGomez has confirmed the rustdoc team wants this as an edition item.

On the edition side, we've discussed and can fit this into our testing with it merging now, so let's get it in.

I would r+ this, but it looks like from the logs above that some tests here still need to be fixed. When that's resolved, this is good to go, r=me,GuillaumeGomez.

@notriddle
Copy link
Contributor Author

Okay, ignoring windows on the test that cares about path syntax.

@GuillaumeGomez
Copy link
Member

Issue was approved so I'm planning on r+ing this PR by this evening. CI seems to have been cancelled so I'll wait for @notriddle to take another look here before doing anything.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

CI is failing. Once fixed, please r=me. :)

@notriddle
Copy link
Contributor Author

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Oct 30, 2024

📌 Commit ac7de1a has been approved by GuillaumeGomez

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 Oct 30, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 30, 2024
…ack, r=GuillaumeGomez

rustdoc: make doctest span tweak a 2024 edition change

Fixes rust-lang#132203

This is a compatibility hack, because I think the new behavior is better. When an A `include_str!` B, and B `include_str!` C, the path to C should be resolved relative to B, not A. That's how `include!` itself works, so that's how `include_str!` with should work.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2024
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#129383 (Remap impl-trait lifetimes on HIR instead of AST lowering)
 - rust-lang#132210 (rustdoc: make doctest span tweak a 2024 edition change)
 - rust-lang#132246 (Rename `rustc_abi::Abi` to `BackendRepr`)
 - rust-lang#132267 (force-recompile library changes on download-rustc="if-unchanged")
 - rust-lang#132344 (Merge `HostPolarity` and `BoundConstness`)

Failed merges:

 - rust-lang#132347 (Remove `ValueAnalysis` and `ValueAnalysisWrapper`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 62ba25d into rust-lang:master Oct 31, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 31, 2024
Rollup merge of rust-lang#132210 - notriddle:notriddle/doctest-span-hack, r=GuillaumeGomez

rustdoc: make doctest span tweak a 2024 edition change

Fixes rust-lang#132203

This is a compatibility hack, because I think the new behavior is better. When an A `include_str!` B, and B `include_str!` C, the path to C should be resolved relative to B, not A. That's how `include!` itself works, so that's how `include_str!` with should work.
@notriddle notriddle deleted the notriddle/doctest-span-hack branch October 31, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition 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.

regression: change in paths/CWD for rustdoc tests
7 participants