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

gfx_device_gl 0.16.1 broken on latest nightly #63687

Closed
bjorn3 opened this issue Aug 18, 2019 · 10 comments · Fixed by #64060
Closed

gfx_device_gl 0.16.1 broken on latest nightly #63687

bjorn3 opened this issue Aug 18, 2019 · 10 comments · Fixed by #64060
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Path resolution C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bjorn3
Copy link
Member

bjorn3 commented Aug 18, 2019

Caused by #63613. To be precise the https://github.com/rust-lang/rust/pull/63613/files#diff-c915449964a130718bd732c4f035b2cfR101 change.

It basically does:

extern crate gfx_core as core;
format!(...);

Because format! uses ::core::format_args to refer to format_args!, it will try to resolve it in gfx_core, which doen't contain format_args!.

   Compiling gfx_device_gl v0.16.1
error[E0433]: failed to resolve: could not find `format_args` in `core`
   --> /Users/bjorn/.cargo/registry/src/github.com-1ecc6299db9ec823/gfx_device_gl-0.16.1/src/shade.rs:434:24
    |
434 |                 name = format!("Target{}", index);
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `format_args` in `core`
    |
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0433]: failed to resolve: could not find `format_args` in `core`
   --> /Users/bjorn/.cargo/registry/src/github.com-1ecc6299db9ec823/gfx_device_gl-0.16.1/src/shade.rs:487:30
    |
487 |             let color_name = format!("Target{}\0", i);
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^ could not find `format_args` in `core`
    |
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0433`.
@jonas-schievink jonas-schievink added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Path resolution I-nominated regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Aug 18, 2019
@Centril
Copy link
Contributor

Centril commented Aug 18, 2019

cc @petrochenkov

@jaynus
Copy link

jaynus commented Aug 23, 2019

This regression affects another project I am working on as well. is there any status on this?

@nikomatsakis
Copy link
Contributor

I'm not sure what the details of #63613 were, but this feels like a bug to me -- in particular it feels like the behavior here is less hygienic, not more? @petrochenkov perhaps you can clarify your view on the situation, that'd be great.

@petrochenkov
Copy link
Contributor

Well, it's more hygienic than before because previously format used whatever format_args was closest in scope at the call site, while now it certainly uses format_args from the crate named core.
(That doesn't make the change less breaking.)

What macro_rules! format really wants is to use def-site hygiene so that "the crate named core" is searched from the macro_rules! format's definition point of view. (Not possible right now due to def-site hygiene for crates not being implemented.)
In the meantime the change to macro_rules! format can be reverted to fix the regression.

@nikomatsakis
Copy link
Contributor

@petrochenkov

Well, it's more hygienic than before because previously format used whatever format_args was closest in scope at the call site, while now it certainly uses format_args from the crate named core.

I see -- just not necessarily the core that was in scope at the point of definition.

In the meantime the change to macro_rules! format can be reverted to fix the regression.

Seems right. Is there a tracking issue to cc or something?

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 29, 2019

That said, the constructions

extern crate some_crate as core;
extern crate some_crate as std;

or (same, but from command line)

--extern core=/path/to/some_crate
--extern std=/path/to/some_crate

really require some responsibility since they effectively override std and core.

If you override them you should be able to provide all the necessary services expected from std and core, since for any third party macro ::std::foo is the only reliable way to refer to the standard library, at least until def-site hygiene is fully implemented and stabilized.

@petrochenkov
Copy link
Contributor

@nikomatsakis

Is there a tracking issue to cc or something?

This is supposed to be the tracking issue for the regression?

@est31
Copy link
Member

est31 commented Aug 29, 2019

While most of the changes in @petrochenkov 's PR have increased hygiene, the specific change linked by @bjorn3 didn't do any increase (or decrease) as both times the macro is being resolved in an unhygienic way.

From a pragmatic point of view, lots of people seem to shadow the core crate but few people want to (saying "want to" and not "do" because of selection bias) shadow the format_args! macro while wanting to use the builtin one for format! which is what the change now allows you to do. Furthermore, not breaking downstream users should always be a priority. It's not their fault that the format macro uses something in an unhygienic way.

IMO the specific change should be reverted.

since for any third party macro ::std::foo is the only reliable way to refer to the standard library, at least until def-site hygiene is fully implemented and stabilized.

There is a pattern to use std or any other third party crate in a hygienic way. You define a #[doc(hidden)] pub mod __reexports { pub use std; } in lib.rs and then use $crate::__reexports::std::... to refer to std items in the macro.

@petrochenkov
Copy link
Contributor

Fixed in #64060.

@nikomatsakis
Copy link
Contributor

check-in from compiler triage:

marking as p-high, but @petrochenkov has a pending fix already 🎉

tmandry added a commit to tmandry/rust that referenced this issue Sep 10, 2019
Improve hygiene of `alloc::format!`

`format` now uses `format_args` though a `__export` module, as described in rust-lang#63687 (comment).

Fixes rust-lang#63687
Centril added a commit to Centril/rust that referenced this issue Sep 10, 2019
Improve hygiene of `alloc::format!`

`format` now uses `format_args` though a `__export` module, as described in rust-lang#63687 (comment).

Fixes rust-lang#63687
@bors bors closed this as completed in e757d33 Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Path resolution C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants