Uh oh!
There was an error while loading. Please reload this page.
Add meta-variable checks in macro definitions - #62008
Conversation
rust-highfive
commented
Jun 20, 2019
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
ia0
commented
Jun 20, 2019
r? @mark-i-m (not sure it's ready for final review, but the functionality is there) |
rust-highfive
commented
Jun 20, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Uh oh!
There was an error while loading. Please reload this page.
mark-i-m
commented
Jun 21, 2019
Thanks @ia0! I will take a look soon. For the sake of documentation: Next steps:
|
mark-i-m
left a comment
There was a problem hiding this comment.
@ia0 This is excellent! Overall I think it looks good. I left a bunch of comments, but none of them is a major issue. My comments fall into 3 categories:
- Blocking
bors tryTODOwill causetidyto fail. UseFIXMEinstead (more below).- Change to allow-by-default lint.
- Blocking
r=me- Using
SmallVecinstead ofVec. This will avoid heap allocations in a few places in the most common case. - It would be good to have more comments in some places. The handling of nested macros is a bit subtle and non-obvious. I've left notes where I think the comments could be augmented. (Overall, though, your comments were very helpful ❤️).
- Would be good to use an enum instead of an integer for the state machine in
check_nested_occurrences - Using
DUMMY_SPis more conventional thanSpan::default, I think. Perhaps @petrochenkov can correct me if I am wrong. - In the
uitests,//~ERRORshould be followed by a prefix of the error message
- Using
- Could be done in a followup PR (perhaps just leave FIXMEs for them)
- Warning where we believe we are in a nested macro (false positives)
- Reducing the amount of
cloneandto_owned. - Move
is_delimitedandis_tokento inherent methods onTokenTree.
@petrochenkov It might also be good to get a perf run after crater.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ia0
commented
Jun 22, 2019
@mark-i-m Thanks a lot for your comments! I applied the simple ones and realized a bit more how approximative the current check is. This is due to the fact that meta-variables in nested macros can be hidden by using a meta-variable for the dollar token: I'll focus on making this a lint and adding more comments explaining nested macros. |
rust-highfive
commented
Jun 22, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
mark-i-m
commented
Jun 22, 2019
@ia0 Thanks :) I think the check in its current form probably covers the vast majority of use cases. If somebody is doing Regarding the |
petrochenkov
commented
Jun 22, 2019
ia0
commented
Jun 22, 2019
I turned the errors into a lint (and removed the overridden dependencies). |
mark-i-m
commented
Jun 22, 2019
Might be easiest if you just submit that commit as another PR too. |
Fix meta-variable binding errors in macros The errors are either: - The meta-variable used in the right-hand side is not bound (or defined) in the left-hand side. - The meta-variable used in the right-hand side does not repeat with the same kleene operator as its binder in the left-hand side. Either it does not repeat enough, or it uses a different operator somewhere. This change should have no semantic impact. Found by rust-lang#62008
ia0
commented
Jun 23, 2019
Indeed, created #62070. And actually even the second commit (storing kleene operator span) can be submitted in a separate PR. All commits in this PR are orthogonal. |
a148f69 to
7d06bd0Comparecraterbot
commented
Jul 17, 2019
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
commented
Jul 17, 2019
🎉 Experiment
|
petrochenkov
commented
Jul 17, 2019
@ia0 |
The content of this post has been moved to the issue: #61053 (comment). |
emilyalbini
commented
Jul 17, 2019
Yeah, you should only look at the regressed section. |
mark-i-m
commented
Jul 19, 2019
@ia0 Is there a way to filter out the transitive failures? I would hate for you to have to look through thousands of error message. |
ia0
commented
Jul 19, 2019
What I'm doing is Also, note that the issue of transitive failures is probably because I use |
petrochenkov
commented
Jul 19, 2019
@ia0 The node ID determines the scope in which the lint can be |
petrochenkov
commented
Jul 19, 2019
"Non-root" regressions are probably from macros generating macro definitions. Issues like this are usually fixed by disabling lints for items (macro items in this case) produced by macros from other crates. |
This is needed for having complete error messages where reporting macro variable
errors. Here is what they would look like:
error: meta-variable repeats with different kleene operator
--> $DIR/issue-61053-different-kleene.rs:3:57
|
LL | ( $( $i:ident = $($j:ident),+ );* ) => { $( $( $i = $j; )* )* };
| - expected repetition ^^ - conflicting repetitionpetrochenkov
commented
Jul 20, 2019
@bors r+ |
bors
commented
Jul 20, 2019
📌 Commit 6ec4584 has been approved by |
bors
commented
Jul 20, 2019
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
bors
commented
Jul 20, 2019
☀️ Test successful - checks-azure |
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_checkfor more details.What remains to be done:
question_mark_macro_seplint Remove outdated question_mark_macro_sep lint #62160