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

Possible regression at nightly-2016-06-15 for include_str! macro #34431

Closed
kbknapp opened this issue Jun 23, 2016 · 11 comments
Closed

Possible regression at nightly-2016-06-15 for include_str! macro #34431

kbknapp opened this issue Jun 23, 2016 · 11 comments
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@kbknapp
Copy link

kbknapp commented Jun 23, 2016

I have an example which internally uses include_str! to pull in the contents of a file in the same directory. This example fails to buid on all nightlies after 2016-06-14 (ab0b87458), yet passes on ab0b87458 and all prior (including stable).

The error message I get is:

error: couldn't read 17_yaml.yml: No such file or directory (os error 2)
 --> <clap macros>:2:44
  |>
2 |> & :: clap :: YamlLoader :: load_from_str ( include_str ! ( $ yml ) ) . expect
  |>                                            ^^^^^^^^^^^^^^^^^^^^^^^
examples/17_yaml.rs:32:15: 32:40: note: in this expansion of load_yaml! (defined in <clap macros>)

error: aborting due to previous error
error: Could not compile `clap`.

To learn more, run the command again with --verbose.

The only commit that seems even somewhat related is 4259fba7e6?

@brson brson added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Jun 23, 2016
@TimNN
Copy link
Contributor

TimNN commented Jun 23, 2016

I believe this may have been caused by #34187.

Edit: Apparently not, testing with that commit reverted did not fix the problem.

@TimNN
Copy link
Contributor

TimNN commented Jun 24, 2016

I can confirm that reverting 4259fba fixes the problem.

cc @jseyfried

The following code reproduces the issue:

bar.rs:

#![crate_type = "lib"]

#[macro_use]
mod foo;

load_data!("data.txt");

data.txt: Any file, contents don't matter.

foo/mod.rs:

macro_rules! load_data { ($file:expr) => {
    static DATA: &'static str = include_str!($file);
}}

@jseyfried
Copy link
Contributor

jseyfried commented Jun 24, 2016

@TimNN Thanks for the cc and the minimal example! This was caused by #33749.

I think this is a bug-fix -- include_str! is now searching in the directory foo, where it was written, rather than ., where it ended up getting expanded. This allows macro authors to use include_str! in macro definitions without worrying about where users will use the macros.

That being said, if this is causing significant breakage in practice, we should fix it (not that hard) or do a warning cycle (also not that hard). Maybe we could do a crater run to determine breakage?

cc @nrc

@TimNN
Copy link
Contributor

TimNN commented Jun 24, 2016

@jseyfried: Will the new behaviour ever work with macros imported from extern crates? I imagine not, since the extern crate's sources will not necessarily be available.

@jseyfried
Copy link
Contributor

jseyfried commented Jun 24, 2016

@TimNN Good point, macros from extern crates that use include_str wouldn't work.

Perhaps we should just keep the old behaviour -- the new behaviour may not be enough of an improvement to justify potential breakage or a warning cycle.

@sfackler
Copy link
Member

A mild ergonomic improvement seems far from what I'd want to see to justify breaking significant amounts of code in the real world, regardless of the existence of a warning period.

@jseyfried
Copy link
Contributor

@stacker agreed -- we would only consider the change if crater found virtually no breakage.

@Ericson2314
Copy link
Contributor

I think this is the correct semantics, anywhere to file an issue for macros 2.0?

@Ericson2314
Copy link
Contributor

For the extern crate perhaps something like scheme's let-syntax could be used to evaluate the macro outside the macro body, and the substitute it's evaluation via an identifier inside?

@jseyfried
Copy link
Contributor

Maybe comment on one if @nrc''s RFCs -- probably RFC 1584.

@jseyfried
Copy link
Contributor

jseyfried commented Jun 26, 2016

Hmm, I'll have to look into let-syntax a bit more to understand that.
Another idea for extern crates could be to have the search path be based on the source location of the file name itself, not the include! macro invocation. This would alow someone to define my_include!().

bors added a commit that referenced this issue Jun 26, 2016
Revert a change to the relative path for macro-expanded `include!`s

Fixes #34431 (c.f. discussion in that issue).
r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

No branches or pull requests

6 participants