-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Check that self
can be used in a macro without having to be specified as an argument.
#34317
Conversation
… to be specified as an argument.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
||
fn main() { | ||
assert_eq!(format!("Pretty: {}", A { v: 3 }.pretty()), "Pretty: <A:3>"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there should be a newline at the end of this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we just need to test that this compiles, so this could be a compile-fail
test, which would run >2x faster:
#![feature(rustc_attrs)]
...
#[rustc_error]
fn main() {} //~ ERROR compilation successful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it, this test is supposed to succeed. I added:
#![feature(rustc_attrs)]
...
#[rustc_error]
fn main() {} //~ ERROR compilation successful
Then ran make check-stage1-cfail
and the test failed. Which I thought means compilation succeeded which it should (at least it does in the latest master). You can see my changes in this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compile-fail
test failed due to "unused struct" warnings. If you add #![allow(unused)]
, it will pass.
(also, there's two trailing newlines in the compile-fail
test)
(finally, feel free to squash/--amend
commits to keep the history clean)
@joelself For example, fn f(x: i32, y: i32) {
my_macro!(x); // Because of hygiene, we know that `my_macro!` cannot access `y`.
} This is also true for struct S;
impl S {
fn f(&self, x: i32) {
my_macro!(x); // Because of hygiene, we know that `my_macro!` cannot access `self`.
}
} #33485 would have allowed Macros can introduce bindings, but they can only be used by the macro itself. For example, macro_rules! m {
() => {
let x = 1;
println!("{}", x); // This is OK since `x` was declared in the macro.
}
}
fn main() {
m!(); // this expands to `let x = 1; println!("{}", x);` and prints "1".
x; // This is a unresolved name error, since the binding can only be used by the macro
} Again, this is true of macro_rules! m {
() => {
struct S;
impl S {
fn f(&self) { // Since this binding was introduced by the macro,
self; // it can be used here
}
}
}
}
m!(); // This compiles on stable, beta, and nightly |
Also, macro_rules! m {
($x:ident) => {
let x = 1; // This binding can only be used by the macro, so
x; // this is OK, but
$x; // this is a name resolution error (since it came from outside the macro)
}
}
fn main() {
m!(x); // (in this expansion)
} Similarly, macro_rules! m {
($x:ident) => {
struct S;
impl S {
fn f(&self) {
self; // this is OK, but
$x; // this is a name resolution error (since it came from outside the macro)
}
}
}
}
fn main() {
m!(self); // (in this expansion)
} |
Ok, now I'm thinking I've made everyone pass in macro_rules! method (
($name:ident<$a:ty>, $self_:ident, $submac:ident!( $($args:tt)* )) => (
fn $name( $self_: $a, i: &[u8] ) -> ($a, $crate::IResult<&[u8], &[u8], u32>) { // instead of this
let result = $submac!(i, $($args)*);
($self_, result)
}
);
); if instead I had this: fn $name( self: $a, i: &[u8] ) -> ($a, $crate::IResult<&[u8], &[u8], u32>) { It should work since the binding of self is introduced by the macro. I gotta go try it out when I get the chance, but man I swear it didn't work. I'm going to close this because clearly I misunderstood the situation. Thanks for taking all of the time to explain stuff to me though! |
Or in other words: check that
self
is allowed to be 'unhygienic'. I don't like that word because it has such a negative connotation where this change breaks nothing, but makes method-making macros so much more sensible. Without it you have to tell users to always placeself
right there, no matter what. You cannot put any other variable there ever or you'll get a cryptic error message. I've already had one user stumble over this.Better arguments have been made, I'll let them speak for themselves: #1606, #33485.
I was in the process of making the change to make
self
'unhygienic' when I discovered that it has already been done. So here's a test that verifies thatself
isreasonableunhygienic. If the change was intentional then you should merge it.cc @jseyfried
r? @nrc