Change in behavior of include! in doctests on nightly

[sgrif]

I haven't had a chance to create a minimal repro case, but I'm seeing these in Diesel's builds: https://travis-ci.org/diesel-rs/diesel/jobs/252088111#L824. The issue only occurs on nightly, and it seems to only affect doctests which are more than 2 directories deep.

[sgrif]

Any chance I can get some eyes on this? It's going to become a regression in beta soon.

[Eijebong]

I tried bisecting but ran in some issues with rust not being able to compile anything that had a proc_macro in it after the first iteration.

error: libproc_macro-19bb163a3bc62cba.so: cannot open shared object file: No such file or directory
  --> /home/eijebong/.cargo/registry/src/github.com-1ecc6299db9ec823/dotenv-0.10.1/src/lib.rs:11:1
   |
11 | extern crate derive_error_chain;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: Could not compile `dotenv`.

EDIT:
Oh and if I fix this by using LD_LIBRARY_PATH, I get

error: proc-macro derive panicked
  --> /home/eijebong/.cargo/registry/src/github.com-1ecc6299db9ec823/dotenv-0.10.1/src/lib.rs:22:17
   |
22 | #[derive(Debug, error_chain)]
   |                 ^^^^^^^^^^^
   |
   = help: message: index out of bounds: the len is 0 but the index is 4294967180

error: Could not compile `dotenv`.

[Mark-Simulacrum]

@sgrif Is there a chance you or someone else could provide a more minimal example? Preferably something that would work with straightforward rustdoc invocation...

[Eijebong]

@Mark-Simulacrum Here is a repo https://github.com/Eijebong/repro_rust_43153

cargo test --doc passes on stable but doesn't on nightly (and beta for some reasons, I have no idea why diesel tests are passing...)

EDIT: They're not passing on beta anymore.

[sgrif]

Thanks for getting a minimal repro @Eijebong

[Mark-Simulacrum]

Bisection shows 4d526e0 - #40939, cc @jseyfried, @alexcrichton -- I'm not sure if this bisection is entirely accurate, but it does seem potentially related.... I'm going to attempt to look into fixing, but don't count on me.

[jdm]

This looks like a similar symptom ("No such file or directory") to the warning I reported in #43371.

[alexcrichton]

@nrc would you be able to help out with investigation here?

[nrc]

I can try. I don't think it is the same as #43371 (I'll comment over there about the cause, and it doesn't explain this), though it might well be the same PR that regressed things here.

[Mark-Simulacrum]

Nominated for prioritization and investigation. Seems bad.

[nrc]

Observation, from the minimal test case, the error reported is error: couldn't read "src/src/setup.rs" - note the double src, the actual file is not there, it is at src/setup.rs as specified in the include!. However, in the Travis links, that is not the case (just a single src and pointing in the right place).

[nrc]

I think what is happening here is that res_rel_file (see

rust/src/libsyntax/ext/source_util.rs

Line 185 in 5dfcd85

fn res_rel_file(cx: &mut ExtCtxt, sp: syntax_pos::Span, arg: &Path) -> PathBuf {

) creates an absolute path from a relative one, using the location of the filename as the 'working directory'. To do so, it uses the span. However, #40939 changed how spans are stored for some macro expansions. So when we have a double include or an include inside some other macro expansion, we calculate the relative path differently because the span for the nested include is different.

I don't know how to fix this - its 6pm on Friday and I just worked out the problem. If it is easy for someone to take a look at over the weekend, that would be awesome. Otherwise I'll come up with a fix on Monday (I have no idea whether this will be easy or hard to fix).

[sgrif]

Thanks for looking into it @nrc. Have a good weekend! ❤️

[nrc]

I am confused. Both the comment in the code I linked in the previous comment and the docs for include! (https://doc.rust-lang.org/nightly/std/macro.include.html) seem to be wrong in different ways. What do people expect the path to include! to be resolved relative to? Given that the examples in the mini repo here and the examples I found in Diesel are all of the form src/foo.rs, I assume they are relative to Cargo.toml. Does that mean they are relative to the working directory? It does not seem to be relative to the compilation unit (which I would expect to be the src directory, not its parent) or the current file.

[sgrif]

I've always just gone with "whatever worked". Certainly the behavior seems to be working directory, which cargo should always be enforcing is the manifest directory. Relative to the current file would definitely be problematic if it's ever used from inside a macro.

[nrc]

Nominating for lang team discussion. This was a breaking change, but I think it is also a bug fix. I'm not sure if we should revert to the old behaviour and then warning cycle, or leave it as is, or revert to the old behaviour and leave there. So, nominating for lang team discussion.

Problem summary

Note that this only affects include! in code in doc comments which gets executed in doc tests (yes I know that is in the title, but just to be clear that include! in normal code is working fine).

For a 'normal' include, the file included is relative to the current file (there has been previous discussion about what that means wrt macro expansion, but basically it is unhygienic for back compat reasons). However, for include! used in doc tests, the path has been considered relative to the working directory. Looking at the implementation, that was clearly unintentional, but I assume we never thought about what should happen.

I see three solutions:

• restore the previous behaviour (paths are relative to working directory), i.e., just fix this issue
• leave this as is (paths are relative to the current file), i.e., include! in doc tests matches include! outside. Though this is a breaking change, so ideally we'd warning cycle, etc.
• ban include! in doc tests because they don't have a clear idea of where they are in the filesystem so it doesn't make sense (also a breaking change).

Implementation note

The implementation takes the path from a span, does a pop then pushes the string from include, which would give a path relative to the file in the span. However, for doc tests, the filename wass <anon>, so the pop/push gave just the supplied path (which is taken as relative to the WD). Now we get a proper filename in the span, so we get the file-relative behaviour.

[sgrif]

If we have a defined story for what include! resolves to in doctests I'm fine with changing Diesel's codebase to match (obviously I don't speak for everybody, but I'd wager few others are doing this). Banning include! in doctests would make our life a lot harder. When demonstrating an API requires a database with some schema to exist, there's a lot of setup code that would otherwise have to be duplicated.

[nrc]

We discussed this at today's lang team meeting. Conclusion was that we should fix the behaviour of include! in doc tests so that the path is relative to the file where the doctest is written (i.e., match include! outside doc tests). Note that this changes behaviour for include if written by the user in a doc test, but means the behaviour is not changed if it is macro-included. We also decided that this use of include was probably niche enough that it did not require a warning cycle.

I'll do the implementation.

[sgrif]

👍 Just let me know when I can point at a nightly/beta release to fix our use in Diesel. 😄

[nrc]

@sgrif this is fixed in #43782, which is waiting for review. Once that lands, you'll need to change the include!s in doc comments

[nrc]

@sgrif @Eijebong: #43782 has landed and should be in today's nightly. Could you let me know if it fixes the problems you've had once the source code is fixed up please?

[Eijebong]

Is the nightly out yet ? rustup update isn't updating anything

[Eijebong]

@nrc diesel-rs/diesel#1101

It's fixed, now waiting for it to be in a release :) thanks

[briansmith]

This broke ring's build. I need to remove the "src/" prefix on beta & nightly Rust but I need to keep it for stable Rust. How should one accomplish this? That is, for a project affected by this change, how do we write code that bridges the change in behavior (without adding additional dependencies)?

Also, this needs to be in the release notes.

[nrc]

Sorry for the breakage. We assumed that since the original issue did not cause problems, neither would the fix. It should indeed be in the release notes. This patch was uplifted to beta, so the easiest option is to just wait a week and then all channels will need the fixed version. If you don't want to wait then mark the examples with ignore.