MBE: more checks at definition time #61053
Comments
|
Could someone tag this with EDIT: and |
jonas-schievink
added
A-macros
|
@mark-i-m you can apply labels yourself by using |
|
Oh, that's good to know. Thanks :) @rustbot modify labels: +C-cleanup |
rustbot
added
the
C-cleanup
Centril
added
T-compiler
|
Just some a quick note for anyone who wants to implement this: the checks should probably be inserted here, where there are already some other checks: rust/src/libsyntax/ext/tt/macro_rules.rs Lines 356 to 369 in dec4c52 You can look at the existing checks to see what they are like. Also, anything implemented will probably need a crater run. I think breakage here is acceptable, since any code that does the things in the OP is almost certainly wrong. EDIT: also, we would probably need to follow a procedure like #57742 and its PRs. |
|
@rustbot modify labels: +E-medium +E-mentor |
rustbot
added
E-medium
|
I am interested into implementing those checks. I would need a bit of mentoring though. Who can I ask questions and through which medium? For example, I implemented the check that variables in rhs are bound in lhs and it already prevents building stage 1 (with |
|
@ia0 Thanks! You can ask me on this thread :) But I am out of town this week, so I might not be super responsive. Hmm... I am not super familiar with quick-error. I believe you can change the Cargo.toml for the faulty crate to use a path dependency. Does this help: https://internals.rust-lang.org/t/how-to-specify-a-private-repository-for-one-library-like-libc/9779 ? |
|
Thanks, I was indeed looking for
|
|
I think most of the work is done and is ready for review. Should I create a pull request? Here is a description of the commits available in https://github.com/ia0/rust/commits/issues_61053:
I will be offline for the next 4 days (leaving Thursday noon, back Sunday night). |
|
Actually, there's an interesting problem:
|
|
Ah, that's a great example. |
|
Ok, I created #62008. Should I migrate the errors to an allow-by-default before running crater? Or maybe I can migrate to a deny-by-default lint, run crater, and then switch the lint to allow-by-default. |
|
@ia0 Thanks! I will take a look soon. Let I think we should make it an allow-by-default lint of the crater run. The goal will be to make sure we aren't breaking any existing known crates. If we want we could also try a run with the deny-by-default lint just to see how prevalent the errors are and what the false-positive rate is. |
|
@petrochenkov raised a good point in #62008 that we don't need to specify the repetition in RHS. It is entirely determined by the LHS. macro_rules! current { ($($x:tt)+) => { $($x)+ }; }
macro_rules! proposed { ($($x:tt)+) => { $($x) }; }Although less noisy, I see 2 possible issues:
|
Add meta-variable checks in macro definitions This is an implementation of #61053. It is not sound (some errors are not reported) and not complete (reports may not be actual errors). This is due to the possibility to define macros in macros in indirect ways. See module documentation of `macro_check` for more details. What remains to be done: - [x] Migrate from an error to an allow-by-default lint. - [x] Add more comments in particular for the handling of nested macros. - [x] Add more tests if needed. - [x] Try to avoid cloning too much (one idea is to use lists on the stack). - [ ] Run crater with deny-by-default lint (measure rate of false positives). - [ ] Remove extra commit for deny-by-default lint - [x] Create a PR to remove the old `question_mark_macro_sep` lint #62160
|
It might be a good thing to do for decl_macros |
I would expect |
So you mean that I think it is preferable to have a breaking change (new compiler does not expect a Kleene operator after a sequence in the RHS) than to have the Kleene operator be optional in the RHS. |
|
@petrochenkov That would be ambiguous unless we denied the use of ?/*/+ in matchers or required the first such character to be a Kleene op, as @ia0 pointed out. |
Add meta-variable checks in macro definitions This is an implementation of #61053. It is not sound (some errors are not reported) and not complete (reports may not be actual errors). This is due to the possibility to define macros in macros in indirect ways. See module documentation of `macro_check` for more details. What remains to be done: - [x] Migrate from an error to an allow-by-default lint. - [x] Add more comments in particular for the handling of nested macros. - [x] Add more tests if needed. - [x] Try to avoid cloning too much (one idea is to use lists on the stack). - [ ] Run crater with deny-by-default lint (measure rate of false positives). - [ ] Remove extra commit for deny-by-default lint - [x] Create a PR to remove the old `question_mark_macro_sep` lint #62160
Add meta-variable checks in macro definitions This is an implementation of #61053. It is not sound (some errors are not reported) and not complete (reports may not be actual errors). This is due to the possibility to define macros in macros in indirect ways. See module documentation of `macro_check` for more details. What remains to be done: - [x] Migrate from an error to an allow-by-default lint. - [x] Add more comments in particular for the handling of nested macros. - [x] Add more tests if needed. - [x] Try to avoid cloning too much (one idea is to use lists on the stack). - [ ] Run crater with deny-by-default lint (measure rate of false positives). - [ ] Remove extra commit for deny-by-default lint - [x] Create a PR to remove the old `question_mark_macro_sep` lint #62160
StatusImplementation#62008 implements the
The main issue with #62008 is that it may have false negatives and false positives. This is due to the possibility to define macros in macros in ways that depend on the actual macro instantiation. The current lint tries to detect nested macro definitions by looking for one of the following patterns:
In case a nested macro definition is detected, its body is analyzed as long as it matches a sequence of Here are a few problematic examples classified by root cause. Behavior depends on actual macro instantiationWe need a check at macro instantiation to be sound and complete. macro_rules! foo {
($n:tt) => {
// The lint does not recognize the possible nested macro definition.
$n! bar {
// $x is free unless $n is `macro_rules`
($x:tt) => { $x };
}
};
}macro_rules! foo {
($d:tt) => {
macro_rules! bar {
// The lint does not recognize the possible meta-variable occurrence:
// `$d z` is a free meta-variable if $d is `$`.
($y:tt) => { $d z };
}
};
}Behavior depends on other macro definitionsWe need some kind of abstract macro instantiation to unfold macro calls. macro_rules! foo {
() => {
// The lint thinks bar is a nested macro definition but it's not.
stringify!(macro_rules! bar { () => { $x }; })
};
}macro_rules! foo {
() => {
// The lint does not look at the definition of `def`, thus does not recognize
// the nested macro definition, and thus reports $x as free.
def!({ ($x:tt) => { $x }; });
};
}
macro_rules! def { ($body:tt) => { macro_rules! bar $body }; }Arbitrary limitationsThe way we try to guess nested macro definitions and macro bodies can be improved. macro_rules! foo {
($($k:tt $v:tt)*) => {
macro_rules! bar {
// The lint does not recognize the repetition as a body, thus falls back
// to checking as if outside a nested macro definition, and thus reports
// $f as free.
$(($f:tt $k) => { $f($v) };)*
}
};
}DiscussionOmit Kleene operator for repetition occurrencesAs noticed by @petrochenkov, the Kleene operator on sequence occurrences is redundant. It is entirely defined by its binder. As such, we could consider a language change to omit the Kleene operator in those cases: macro_rules! current { ($($x:tt)+) => { $($x)+ }; }
macro_rules! proposed { ($($x:tt)+) => { $($x) }; }As noted by @mark-i-m, this may be easier to apply for |
Add meta-variable checks in macro definitions This is an implementation of #61053. It is not sound (some errors are not reported) and not complete (reports may not be actual errors). This is due to the possibility to define macros in macros in indirect ways. See module documentation of `macro_check` for more details. What remains to be done: - [x] Migrate from an error to an allow-by-default lint. - [x] Add more comments in particular for the handling of nested macros. - [x] Add more tests if needed. - [x] Try to avoid cloning too much (one idea is to use lists on the stack). - [ ] Run crater with deny-by-default lint (measure rate of false positives). - [ ] Remove extra commit for deny-by-default lint - [x] Create a PR to remove the old `question_mark_macro_sep` lint #62160
|
To measure the ratio of false positives of #62008 we ran a crater with deny-by-default: https://crater-reports.s3.amazonaws.com/pr-62008/downloads.html. In this post, we go over regressed crates to see if the regression is expected (actual errors in the crate) or if it is due to false positives. I went over all regressed crates that have a non-transitive error which is not a "different Kleene operator" (since those are most probably an actual error and it removes 355 crates). From those 53 crates, 6 have false positives:
The 47 remaining crates only have expected errors:
False positivesWe mostly have 3 types of false positives:
cluColor-0.1.5On this line, we don't see that dyplugin-0.2.0On this line, we don't see that easy_ffi-0.1.0On this line, we think pmutil-0.3.0On this line, we don't see that seed-0.3.7On this line, we think webrtc-sctp-0.0.0On this line, we don't see that |
|
How should we handle macro_rules! bar { () => { stringify!($x) }; }This is not an error apparently (but the lint would report it). Note that this is specific to macro_rules! foo { ($($x:tt)*) => {}; }
macro_rules! bar { () => { foo!($x) }; }I'm currently marking those "errors" as expected when finding false positives, because I don't think this is really an issue of the lint, because the crates that have this issue seem to not rely on this deliberately. |
|
@ia0 Thanks! Sorry for the long delay. Things have been busy...
|
|
Also, thanks for all the work you have put into this! |
|
No problem :-) And by the way, I presented this lint at the Rust meetup in Zürich today (to try to get some people using it), and a few interesting questions came up:
Also, I would like to know the following:
Thanks a lot! |
|
To respond to (3), I was wrong. It doesn't run when proc-macro outputs a macro definition. # Cargo.toml
[package]
name = "proc_macro_experiments"
version = "0.1.0"
edition = "2018"
[lib]
proc-macro = true// src/lib.rs
#![deny(meta_variable_misuse)]
extern crate proc_macro;
use proc_macro::TokenStream;
#[proc_macro]
pub fn make_foo(_: TokenStream) -> TokenStream {
format!("macro_rules! foo {{ () => {{ $x }}; }}")
.parse()
.unwrap()
}// examples/main.rs
#![feature(proc_macro_hygiene)]
#![deny(meta_variable_misuse)]
proc_macro_experiments::make_foo!();
fn main() {
// foo!();
}
|
|
Sorry! It's been quite a while!
At the time I proposed the lint, I didn't realize it would have false positives. Also, I don't know enough about clippy to know whether it would be able to implement these.
Personally, I think they should be enabled by default (at least some of them can be). However, there is a bit more discussion on #68836 ... At the very least, I think this work has been really useful in giving us an understanding of some of the downsides of the current MBEs implementation and behavior. |
No problem :-) I wasn't waiting active follow-up. I guess this is a subject that may take quite some time to settle and depends on the future of MBE and proc-macro.
I'll follow that. Thanks for the link!
Yes, it was also very interesting to me to dig a bit into a small part of the Rust compiler :-) |
|
Visiting this in T-lang backlog bonanza, we think the majority of the checks here are currently implemented, and it makes sense to file new issues for concrete additions (perhaps like #95943). Closing this issue for now since it seems like this is no longer actively tracking something. |
|
This issue is still referenced to by these docs: https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#meta-variable-misuse |
There are a few checks that are currently only done at macro invocation time (or are not done at all) that really would make more sense to do a macro definition time.
This is a tracking issue to implement these checks.
Checking that the Kleene operator used in the body is the same as that in the matcher (Add meta-variable checks in macro definitions #62008)
Checking that all meta-variables used in the body are defined and in scope (Add meta-variable checks in macro definitions #62008)
Checking that all meta-variables used in the body are repeating at the right depth (Add meta-variable checks in macro definitions #62008)
Checking that the body of a macro is actually valid rust syntax (to the extent that we can, anyway)Maybe fix false positives/negatives
Possibly propose not using kleene op on RHS
The text was updated successfully, but these errors were encountered: