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

Tricky rustdoc warning messages when using include_str #118549

Closed
shnewto opened this issue Dec 2, 2023 · 5 comments · Fixed by #123204
Closed

Tricky rustdoc warning messages when using include_str #118549

shnewto opened this issue Dec 2, 2023 · 5 comments · Fixed by #123204
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@shnewto
Copy link

shnewto commented Dec 2, 2023

I wanted to share something that made me scratch my head in case it makes anyone else scratch their head too. But I'm not sure I'd call this a bug or an error. It just stumped me for a minute and thought I'd bring it up and let y'all decide whether it's either.

(and in case it helps set the tone of my comments below, I love rustdoc, I love the include_str feature, and I'm grateful for all the work and care that goes into it all!)

When using #![doc = include_str!("../README.md")], if the contents of README.md generate warnings, it's not immediately clear what's to be done.

Here's a screenshot of output from cargo doc when the badge urls in my README.md were bare rather than atomatic urls.

rustdoc-warning

I guess my misguided thought process went something like, "hmm it must be thinking about the path ../README.md as a url?" and I think that derailed me really seeing that the urls provided in the output were the root of the issue and not just "helpful" examples.

...I don't really know what "better" would look like either, I can imagine myself being similarly confused if the terminal output showed me lines from a .md file 😅

Thanks for taking a look! And if you decide that I'm just too easily confused, please feel free to close this issue 😄

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 2, 2023
@fmease fmease added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-diagnostics Area: Messages for errors, warnings, and lints D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 2, 2023
@fmease
Copy link
Member

fmease commented Dec 2, 2023

This is definitely a bug. Ideally, the lint diagnostic would highlight the lines in the included file (here README.md).

Addressing other rustdoc devs: I can't tell how easy it would be to fix this without looking at the relevant code first. I remember that rustc does map included files to SourceFiles and thus assigns a Span distinct from the including file (which would allow a Diagnostic to point into the README.md) but there might be some peculiarities I don't know of. cc #95189 (comment).

@connortsui20
Copy link
Contributor

connortsui20 commented Dec 20, 2023

I actually just ran into something similar, and it is similar enough that I won't make a new issue.

It's common to have code blocks in README.md, and I think that people can forget to annotate the code blocks with syntax highlighting. For example:

This is annotated with "sh".
This is not annotated with anything. (no "sh")

The difference is subtle, but for documentation tests this is pretty important. When you have #![doc = include_str!("../README.md")] and you run cargo test, it will run documentation tests on top of all the other tests. The issue here is that if the code block is not annotated, cargo will treat it as a documentation test.

Example:

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

And then in README.md

# Testing code block in README

A few words

```
This is going run as a test (and not compile)!
```

Running cargo doc is fine, but when I run cargo test, I get this error output:

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests test_include_str_doc

running 1 test
test src\lib.rs - (line 5) ... FAILED

failures:

---- src\lib.rs - (line 5) stdout ----
error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `is`
 --> src\lib.rs:6:6
  |
3 | This is going to be run as a test!
  |      ^^ expected one of 8 possible tokens

error: aborting due to previous error

Couldn't compile the test.

failures:
    src\lib.rs - (line 5)

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.10s

error: doctest failed, to rerun pass `--doc`

I think there are a couple of things that are confusing here:

  • The line where it failed doesn't actually exist, lib.rs should only have 2 lines
  • The error message itself can be cryptic

In this case, it is pretty obvious where we want to find the bug. But when I ran into this issue the first time, I had this in my README.md:

tar -cvf something.tar somethingelse.txt ...

I had forgot to put the $ in front, and I was also calling this in my build script, so it was actually not immediately obvious where the source of this bug was (I actually thought it was something wrong with build.rs).

I think these issues would probably be solved by what @fmease suggested, highlighting the lines in the included file rather than in the file that brought it in. Though it seems like it might be non-trivial to implement this, maybe it could be a lint instead? Make sure that if a doc test is in an included file that it compiles immediately?

Thanks for all the great work!

@GuillaumeGomez
Copy link
Member

I'll try to take a look in the coming weeks if someone didn't already as I've been bitten by this issue quite a few times already.

@fmease
Copy link
Member

fmease commented Dec 20, 2023

I'll try to take a look in the coming weeks

Nice, thanks! If you are short on time, I can squeeze this into my ever-growing TODO list, too ;)

@GuillaumeGomez
Copy link
Member

For now I'll put into my "oh my god how can I have enough time to do all that????" list. 🤣

@bors bors closed this as completed in ffea7e2 Apr 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 12, 2024
Rollup merge of rust-lang#123204 - notriddle:notriddle/include-str-span, r=pnkfelix

rustdoc: point at span in `include_str!`-ed md file

Fixes rust-lang#118549
flip1995 pushed a commit to flip1995/rust that referenced this issue Apr 18, 2024
…an, r=pnkfelix

rustdoc: point at span in `include_str!`-ed md file

Fixes rust-lang#118549
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants