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

Breaking macro name resolution change in 1.29 #54783

Closed
Diggsey opened this issue Oct 3, 2018 · 7 comments
Closed

Breaking macro name resolution change in 1.29 #54783

Diggsey opened this issue Oct 3, 2018 · 7 comments
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Diggsey
Copy link
Contributor

Diggsey commented Oct 3, 2018

Minimal test case: https://github.com/Diggsey/macro_regression_example

This code compiles on 1.28, but fails on 1.29

error[E0433]: failed to resolve. Use of undeclared type or module `TestEnum`
 --> src\main.rs:6:10
  |
6 | #[derive(DbEnum, Debug)]
  |          ^^^^^^ Use of undeclared type or module `TestEnum`

error[E0412]: cannot find type `TestEnum` in this scope
 --> src\main.rs:6:10
  |
6 | #[derive(DbEnum, Debug)]
  |          ^^^^^^ not found in this scope

warning: unused `#[macro_use]` import
 --> src\main.rs:1:1
  |
1 | #[macro_use]
  | ^^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

warning: cannot find type `TestEnum` in this scope
 --> src\main.rs:6:10
  |
6 | #[derive(DbEnum, Debug)]
  |          ^^^^^^ names from parent modules are not accessible without an explicit import
  |
  = note: #[warn(proc_macro_derive_resolution_fallback)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #50504 <https://github.com/rust-lang/rust/issues/50504>

warning: cannot find type `TestEnum` in this scope
 --> src\main.rs:6:10
  |
6 | #[derive(DbEnum, Debug)]
  |          ^^^^^^ names from parent modules are not accessible without an explicit import
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #50504 <https://github.com/rust-lang/rust/issues/50504>

error: aborting due to 2 previous errors

Some errors occurred: E0412, E0433.
For more information about an error, try `rustc --explain E0412`.
error: Could not compile `macro_regression`.

Note that I do not have #![deny(warnings)] or anything like that enabled. This gives quite a bad impression when stability is advertised so frequently.

@Diggsey
Copy link
Contributor Author

Diggsey commented Oct 3, 2018

For anyone else running into this issue with diesel_derive_enum, you can bump to at least version 1.4.4 and it will fix the issue. For other crates it may not be so simple...

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 3, 2018

Hmm, this is #51952, but it was supposed to be a warning for derives (#[warn(proc_macro_derive_resolution_fallback)]).
Apparently #[derive(DbEnum)] does something unexpected so it didn't fall under the lint conditions somehow.
(Regarding stability, this would certainly be fixed it it was reported or found by crater before 1.29 stable release.)

@petrochenkov petrochenkov added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. A-resolve Area: Name resolution A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Oct 3, 2018
@petrochenkov petrochenkov self-assigned this Oct 3, 2018
@pnkfelix
Copy link
Member

triage: I don't know what priority to assign here. Nominating for discussion at T-compiler meeting.

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Nov 29, 2018
@pnkfelix
Copy link
Member

discussed at T-compiler meeting. Tagging as P-high and leaving assigned to @petrochenkov

@pnkfelix pnkfelix added P-high High priority and removed I-nominated labels Nov 29, 2018
@petrochenkov
Copy link
Contributor

Tentatively wontfix, but I haven't looked why exactly it is reported as an error and not as a warning.

If it's going to be fixed, then it will still be under compatibility lint, and still turned into an error later.
Given that it's already an error on stable 1.29, 1.30 and soon 1.31, it doesn't make much sense to go through deprecation warning again.

@nikomatsakis
Copy link
Contributor

@petrochenkov that logic makes sense to me.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 6, 2018

closing as wontfix.

(if anyone feels inspired by the conversation to try to find out why the warning-cycle code was subverted by DbEnum, I whole-heartedly support them channeling that inspiration into an investigation. but the point here is that we are not going to attempt to backport a fix to the warning-cycle.)

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: Name resolution P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants