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

the implied edition for playground needs to be 2021 in ch09-02 (at least) #3970

Closed
correabuscar opened this issue Jul 8, 2024 · 5 comments
Closed

Comments

@correabuscar
Copy link

correabuscar commented Jul 8, 2024

URL to the section(s) of the book with this problem:
https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html#recoverable-errors-with-result
https://doc.rust-lang.org/nightly/book/ch09-02-recoverable-errors-with-result.html#recoverable-errors-with-result

Description of the problem:
There's an example there, this one:

use std::fs::File;

fn main() {
    let greeting_file_result = File::open("hello.txt");

    let greeting_file = match greeting_file_result {
        Ok(file) => file,
        Err(error) => panic!("Problem opening the file: {error:?}"),
    };
}

which when you press the Play button, it clearly uses one of 2015 or 2018 editions instead of the 2021 one because the errors are these:

Compiling playground v0.0.1 (/playground)
warning: unused variable: `greeting_file`
 --> src/main.rs:6:9
  |
6 |     let greeting_file = match greeting_file_result {
  |         ^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_greeting_file`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `error`
 --> src/main.rs:8:13
  |
8 |         Err(error) => panic!("Problem opening the file: {error:?}"),
  |             ^^^^^ help: if this is intentional, prefix it with an underscore: `_error`

warning: panic message contains an unused formatting placeholder
 --> src/main.rs:8:57
  |
8 |         Err(error) => panic!("Problem opening the file: {error:?}"),
  |                                                         ^^^^^^^^^
  |
  = note: this message is not used as a format string when given without arguments, but will be in Rust 2021
  = note: `#[warn(non_fmt_panics)]` on by default
help: add the missing argument
  |
8 |         Err(error) => panic!("Problem opening the file: {error:?}", ...),
  |                                                                   +++++
help: or add a "{}" format string to use the message literally
  |
8 |         Err(error) => panic!("{}", "Problem opening the file: {error:?}"),
  |                              +++++

warning: `playground` (bin "playground") generated 3 warnings (run `cargo fix --bin "playground"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.74s
     Running `target/debug/playground`
thread 'main' panicked at src/main.rs:8:23:
Problem opening the file: {error:?}
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Also note that there's no button to open that code block in playground, there's only copy code and play button(which shows the output inline, while viewing the book), so to try it in playground, one must manually open the already-known playground url then paste the copied code there and see that it actually works (due to 2021 edition being default in playground).

Here's 2018 edition on playground
Here's 2021 edition on playground
The latter works as expected.

Suggested fix:
I don't know where this edition is enforced for playground(it doesn't seem like this knows), but, at least for this example, it should be edition=2021, because 2015 and 2018 show the above warnings and as you can see, it doesn't display correct output Problem opening the file: {error:?} which should be Problem opening the file: Os { code: 2, kind: NotFound, message: "No such file or directory" }.

I've checked the linked file and it is in a project with edition 2021, however, that doesn't mean that the play button knows to use that edition for playground.

It would seem that the workaround1 is quite possibly adding edition2021 here just like it's done in

```rust,edition2021

A built book.js has 2015 as the default edition:

        let text = playground_text(code_block);
        let classes = code_block.querySelector('code').classList;
        let edition = "2015";
        if(classes.contains("edition2018")) {
            edition = "2018";
        } else if(classes.contains("edition2021")) {
            edition = "2021";
        }

well this default is from mdBook and been there since 2021 or earlier.

Footnotes

  1. it's just a workaround because who knows in how many other places this is needed; ideally, that edition value would be gotten from Cargo.toml instead, somehow, everywhere where there's a play button...

@chriskrycho
Copy link
Contributor

Thanks for reporting this! I agree that we should make sure that all the playground links get generated using the edition in the Cargo.toml file. We can do that using the relevant mdBook config, and then we’ll also need to audit to make sure everything still works as expected.

@correabuscar

This comment was marked as outdated.

@chriskrycho
Copy link
Contributor

I merged a baseline change for this in #3974, as it passed CI so things which have a hard requirement on it all pass. We likely still want to do a further pass to check the behavior of the other listings, and we will also want to figure out why this one (and any others like it!) passed CI (which is the really 😬 bit).

@correabuscar
Copy link
Author

correabuscar commented Jul 16, 2024

we will also want to figure out why this one (and any others like it!) passed CI

it doesn't fail because they're only warnings not errors, when using edition 2015 instead of edition 2021.

It would need a #![deny(rust_2021_compatibility)] for that specific example(in OP) to fail, if edition 2015 or 2018 were used. However, I haven't found a global way to deny that for ALL examples.


I guess mdbook test is the one that's supposed to fail? and yet it does not fail even with:

diff --git a/book.toml b/book.toml
index 800adcf0f5bcd753..90f9cdebce43fff7 100644
--- a/book.toml
+++ b/book.toml
@@ -17,4 +17,4 @@ git-repository-url = "https://github.com
 output-mode = "default"
 
 [rust]
-edition = "2021"
+edition = "2015"

So we can't trust it.
A preliminary test I did shows that the edition isn't used at all by mdbook test (so it must be using whatever the default edition is for the current rustc, unsure) , I'll look more into it.

Ok so apparently mdbook test handles the {#rustdoc_include ../listings/ch09-error-handling/listing-09-04/src/main.rs} which makes the code become:

```rust,should_panic
#![allow(unused_variables)]
use std::fs::File;

fn main() {
    let greeting_file_result = File::open("hello.txt");

    let greeting_file = match greeting_file_result {
        Ok(file) => file,
        Err(error) => panic!("Problem opening the file: {error:?}"),
    };
}
\```

and then mdbook test calls out to rustdoc (as per this info) which is a call like this: /tmp/mdbook-F1QUCF/ch09-02-recoverable-errors-with-result.md --test --edition 2021 (the 2021 is the one from book.toml),
this seems to be enough in theory, but for some reason rustdoc doesn't fail because they're only warnings not errors, when using edition 2015 instead of edition 2021.

@chriskrycho
Copy link
Contributor

I just circled back to this and checked on things, and I think the fix I landed to handle this did indeed handle everything we needed here. We will need to remember to hit that on future updates, like when the 2024 Edition lands in a few months, so I opened #4064 to help us remember! Going to go ahead and close this out now. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants