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

macros: Remove matching on "complex" nonterminals requiring AST comparisons #49326

Merged
merged 1 commit into from Apr 14, 2018

Conversation

Projects
None yet
@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 24, 2018

So, you can actually use nonterminals from outer macros in left hand side of nested macros and invocations of nested macros will try to match passed arguments to them.

macro outer($nt_item: item) {
    macro inner($nt_item) {
        struct S;
    }

    inner!($nt_item); // OK, `$nt_item` matches `$nt_item`
}

Why this is bad:

  • We can't do this matching correctly. When two nonterminals are compared, the original tokens are lost and we have to compare AST fragments instead. Right now the comparison is done by PartialEq impls derived on AST structures.
    • On one hand, AST loses information compared to original tokens (e.g. trailing separators and other simplifications done during parsing to AST), so we can produce matches that are not actually correct.
    • On another hand derived PartialEq impls for AST structures don't make much sense in general and compare various auxiliary garbage like spans. For the argument nonterminal to match we should use literally the same token (possibly cloned) as was used in the macro LHS (as in the example above). So we can reject matches that are actually correct.
    • Support for nonterminal matching is the only thing that forces us to derive PartialEq for all (!) AST structures. As I mentioned these impls are also mostly nonsensical.

This PR removes support for matching on all nonterminals except for "simple" ones like ident, lifetime and tt for which we have original tokens that can be compared.
After this is done I'll submit another PR removing huge number of PartialEq impls from AST and HIR structures.

This is an arcane feature and I don't personally know why would anyone use it, but the change should ideally go through crater.
We'll be able to support this feature again in the future when all nonterminals have original token streams attached to them in addition to (or instead of) AST fragments.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 24, 2018

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 24, 2018

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Mar 24, 2018

There's one pattern I know of that this could break, though it's fine if :tt is still allowed. It's where you use an inner macro to make an equality check:

macro_rules! eq {
    ($a:tt $b:tt) => {
        macro_rules! __eq {
            ($a $a) => { /* equal */ };
            ($a $b) => { /* not equal */ }
        }
        
        __eq!($a $b);
    }
}

Arcane, yes, but useful. This trick is the basis of my crate static-cond (and used transitively in named-block), though I learned/stole it from @sdleffler.

@sdleffler

This comment has been minimized.

Copy link
Contributor

sdleffler commented Mar 25, 2018

@durka notably, iirc, this trick no longer works if you call that macro more than once: last time I tried it, I got a "macro shadows another macro" error :(

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Mar 25, 2018

@sdleffler static-cond uses an inner scope so it can be called multiple times, however it only works as an expression macro, not an item macro. It works fine. Not sure how the RFC 1560 restriction actually applies or whether it's supposed to. @jseyfried?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Mar 26, 2018

We'll want crater artifacts eventually presumably.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 26, 2018

⌛️ Trying commit 8eb9aa4 with merge 0b4b23e...

bors added a commit that referenced this pull request Mar 26, 2018

Auto merge of #49326 - petrochenkov:nteq, r=<try>
macros: Remove matching on "complex" nonterminals requiring AST comparisons

So, you can actually use nonterminals from outer macros in left hand side of nested macros and invocations of nested macros will try to match passed arguments to them.

```rust
macro outer($nt_item: item) {
    macro inner($nt_item) {
        struct S;
    }

    inner!($nt_item); // OK, `$nt_item` matches `$nt_item`
}
```

Why this is bad:
- We can't do this matching correctly. When two nonterminals are compared, the original tokens are lost and we have to compare AST fragments instead. Right now the comparison is done by `PartialEq` impls derived on AST structures.
    - On one hand, AST loses information compared to original tokens (e.g. trailing separators and other simplifications done during parsing to AST), so we can produce matches that are not actually correct.
    - On another hand derived `PartialEq` impls for AST structures don't make much sense in general and compare various auxiliary garbage like spans. For the argument nonterminal to match we should use literally the same token (possibly cloned) as was used in the macro LHS (as in the example above). So we can reject matches that are actually correct.
    - Support for nonterminal matching is the only thing that forces us to derive `PartialEq` for all (!) AST structures. As I mentioned these impls are also mostly nonsensical.

This PR removes support for matching on all nonterminals except for "simple" ones like `ident`, `lifetime` and `tt` for which we have original tokens that can be compared.
After this is done I'll submit another PR removing huge number of `PartialEq` impls from AST and HIR structures.

This is an arcane feature and I don't personally know why would anyone use it, but the change should ideally go through crater.
We'll be able to support this feature again in the future when all nonterminals have original token streams attached to them in addition to (or instead of) AST fragments.
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Mar 26, 2018

We should probably compare the underlying tokens the fragment was parsed from, eventually.
But for now this is probably a good step forward.

Another weird thing happening with nonterminals is we expand macros in them (#47358 (comment)).

@eddyb

eddyb approved these changes Mar 26, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 26, 2018

☀️ Test successful - status-travis
State: approved= try=True

@aidanhs

This comment has been minimized.

Copy link
Member

aidanhs commented Mar 31, 2018

Crater run started.

@aidanhs

This comment has been minimized.

Copy link
Member

aidanhs commented Apr 8, 2018

(apologies for the delay, I fell ill last week)

Hi @petrochenkov (crater requester), @eddyb (PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49326/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Apr 8, 2018

All the regressions are spurious.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 12, 2018

Discussed in the @rust-lang/lang meeting and we agreed to go forward with this change. It seems like this was never intentional and we can't really be exposing the details of how PartialEq on our internal AST nodes is implemented.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 12, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 12, 2018

📌 Commit 8eb9aa4 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 13, 2018

⌛️ Testing commit 8eb9aa4 with merge e6410f5...

bors added a commit that referenced this pull request Apr 13, 2018

Auto merge of #49326 - petrochenkov:nteq, r=eddyb
macros: Remove matching on "complex" nonterminals requiring AST comparisons

So, you can actually use nonterminals from outer macros in left hand side of nested macros and invocations of nested macros will try to match passed arguments to them.

```rust
macro outer($nt_item: item) {
    macro inner($nt_item) {
        struct S;
    }

    inner!($nt_item); // OK, `$nt_item` matches `$nt_item`
}
```

Why this is bad:
- We can't do this matching correctly. When two nonterminals are compared, the original tokens are lost and we have to compare AST fragments instead. Right now the comparison is done by `PartialEq` impls derived on AST structures.
    - On one hand, AST loses information compared to original tokens (e.g. trailing separators and other simplifications done during parsing to AST), so we can produce matches that are not actually correct.
    - On another hand derived `PartialEq` impls for AST structures don't make much sense in general and compare various auxiliary garbage like spans. For the argument nonterminal to match we should use literally the same token (possibly cloned) as was used in the macro LHS (as in the example above). So we can reject matches that are actually correct.
    - Support for nonterminal matching is the only thing that forces us to derive `PartialEq` for all (!) AST structures. As I mentioned these impls are also mostly nonsensical.

This PR removes support for matching on all nonterminals except for "simple" ones like `ident`, `lifetime` and `tt` for which we have original tokens that can be compared.
After this is done I'll submit another PR removing huge number of `PartialEq` impls from AST and HIR structures.

This is an arcane feature and I don't personally know why would anyone use it, but the change should ideally go through crater.
We'll be able to support this feature again in the future when all nonterminals have original token streams attached to them in addition to (or instead of) AST fragments.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 13, 2018

💔 Test failed - status-appveyor

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Apr 13, 2018

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:06:06] 598 |                 ident_lhs.node == ident_rhs.node && is_raw_lhs == is_raw_rhs,
[00:06:06]     |                           ^^^^
[00:06:06]
[00:06:06] error[E0609]: no field `node` on type `&syntax_pos::symbol::Ident`
[00:06:06]    --> libsyntax/parse/token.rs:598:45
[00:06:06]     |
[00:06:06] 598 |                 ident_lhs.node == ident_rhs.node && is_raw_lhs == is_raw_rhs,
[00:06:06]     |                                             ^^^^
[00:06:06]
[00:06:06] error[E0609]: no field `ident` on type `&syntax_pos::symbol::Ident`
[00:06:06]    --> libsyntax/parse/token.rs:599:64
[00:06:06]     |
[00:06:06] 599 |             (NtLifetime(lt_lhs), NtLifetime(lt_rhs)) => lt_lhs.ident == lt_rhs.ident,
[00:06:06]     |                                                                ^^^^^
[00:06:06]
[00:06:06] error[E0609]: no field `ident` on type `&syntax_pos::symbol::Ident`
[00:06:06]    --> libsyntax/parse/token.rs:599:80
[00:06:06]     |
[00:06:06] 599 |             (NtLifetime(lt_lhs), NtLifetime(lt_rhs)) => lt_lhs.ident == lt_rhs.ident,
---
[00:06:07]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name syntax libsyntax/lib.rs --color always --error-format json --crate-type dylib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 -C metadata=d3b6fcf798f7d22a -C extra-filename=-d3b6fcf798f7d22a --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/release/deps --extern bitflags=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libbitflags-9866e194db82a141.rlib --extern serialize=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libserialize-c04ded78717d5d67.so --extern serialize=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libserialize-c04ded78717d5d67.rlib --extern log=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/liblog-3e35efb9e5bc7a9a.rlib --extern rustc_cratesio_shim=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_cratesio_shim-296054b03e3b45fe.so --extern syntax_pos=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libsyntax_pos-9f3518d56a01456f.so --extern scoped_tls=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libscoped_tls-efdb8dd1548942a4.rlib --extern rustc_errors=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_errors-609e2421d03f9c9a.so --extern rustc_data_structures=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_data_structures-93cb1ddd29ab61a4.so` (exit code: 101)
[00:06:07] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:06:07] expected success, got: exit code: 101
[00:06:07] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1085:9
[00:06:07] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:06:07] travis_fold:end:stage0-rustc
[00:06:07] travis_time:end:stage0-rustc:start=1523594442600078449,finish=1523594498617288755,duration=56017210306
[00:06:07] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:06:07] Build completed unsuccessfully in 0:01:09
[00:06:07] Makefile:28: recipe for target 'all' failed
[00:06:07] make: *** [all] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:193e06d8:start=1523594499096262374,finish=1523594499116036039,duration=19773665
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:234d2762
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:234d2762:start=1523594499120806784,finish=1523594499126966360,duration=6159576
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:05bb5c16
$ dmesg | grep -i kill
[   10.120576] init: failsafe main process (1093) killed by TERM signal

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 @TimNN. (Feature Requests)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Apr 13, 2018

Needs rebase.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Apr 13, 2018

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 13, 2018

📌 Commit 5bbfd67 has been approved by eddyb

@petrochenkov petrochenkov force-pushed the petrochenkov:nteq branch from 5bbfd67 to 7e1f73b Apr 13, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Apr 13, 2018

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 13, 2018

📌 Commit 7e1f73b has been approved by eddyb

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Apr 13, 2018

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:46] configure: rust.quiet-tests     := True
---
[00:06:14] 598 |                 ident_lhs.node == ident_rhs.node && is_raw_lhs == is_raw_rhs,
[00:06:14]     |                           ^^^^
[00:06:14]
[00:06:14] error[E0609]: no field `node` on type `&syntax_pos::symbol::Ident`
[00:06:14]    --> libsyntax/parse/token.rs:598:45
[00:06:14]     |
[00:06:14] 598 |                 ident_lhs.node == ident_rhs.node && is_raw_lhs == is_raw_rhs,
[00:06:14]     |                                             ^^^^
[00:06:14]
[00:06:14] error[E0609]: no field `ident` on type `&syntax_pos::symbol::Ident`
[00:06:14]    --> libsyntax/parse/token.rs:599:64
[00:06:14]     |
[00:06:14] 599 |             (NtLifetime(lt_lhs), NtLifetime(lt_rhs)) => lt_lhs.ident == lt_rhs.ident,
[00:06:14]     |                                                                ^^^^^
[00:06:14]
[00:06:14] error[E0609]: no field `ident` on type `&syntax_pos::symbol::Ident`
[00:06:14]    --> libsyntax/parse/token.rs:599:80
[00:06:14]     |
[00:06:14] 599 |             (NtLifetime(lt_lhs), NtLifetime(lt_rhs)) => lt_lhs.ident == lt_rhs.ident,
---
[00:06:16]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name syntax libsyntax/lib.rs --color always --error-format json --crate-type dylib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 -C metadata=d3b6fcf798f7d22a -C extra-filename=-d3b6fcf798f7d22a --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/release/deps --extern rustc_errors=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_errors-609e2421d03f9c9a.so --extern log=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/liblog-3e35efb9e5bc7a9a.rlib --extern syntax_pos=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libsyntax_pos-9f3518d56a01456f.so --extern rustc_data_structures=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_data_structures-93cb1ddd29ab61a4.so --extern rustc_cratesio_shim=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-g
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:0caa2c80:start=1523662195618621066,finish=1523662195626682150,duration=8061084
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:22222a40
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:22222a40:start=1523662195633213946,finish=1523662195641322438,duration=8108492
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1bff6996
$ dmesg | grep -i kill
[   12.076477] init: failsafe main process (1096) killed by TERM signal

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 @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 14, 2018

⌛️ Testing commit 7e1f73b with merge 5724462...

bors added a commit that referenced this pull request Apr 14, 2018

Auto merge of #49326 - petrochenkov:nteq, r=eddyb
macros: Remove matching on "complex" nonterminals requiring AST comparisons

So, you can actually use nonterminals from outer macros in left hand side of nested macros and invocations of nested macros will try to match passed arguments to them.

```rust
macro outer($nt_item: item) {
    macro inner($nt_item) {
        struct S;
    }

    inner!($nt_item); // OK, `$nt_item` matches `$nt_item`
}
```

Why this is bad:
- We can't do this matching correctly. When two nonterminals are compared, the original tokens are lost and we have to compare AST fragments instead. Right now the comparison is done by `PartialEq` impls derived on AST structures.
    - On one hand, AST loses information compared to original tokens (e.g. trailing separators and other simplifications done during parsing to AST), so we can produce matches that are not actually correct.
    - On another hand derived `PartialEq` impls for AST structures don't make much sense in general and compare various auxiliary garbage like spans. For the argument nonterminal to match we should use literally the same token (possibly cloned) as was used in the macro LHS (as in the example above). So we can reject matches that are actually correct.
    - Support for nonterminal matching is the only thing that forces us to derive `PartialEq` for all (!) AST structures. As I mentioned these impls are also mostly nonsensical.

This PR removes support for matching on all nonterminals except for "simple" ones like `ident`, `lifetime` and `tt` for which we have original tokens that can be compared.
After this is done I'll submit another PR removing huge number of `PartialEq` impls from AST and HIR structures.

This is an arcane feature and I don't personally know why would anyone use it, but the change should ideally go through crater.
We'll be able to support this feature again in the future when all nonterminals have original token streams attached to them in addition to (or instead of) AST fragments.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 14, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 5724462 to master...

@bors bors merged commit 7e1f73b into rust-lang:master Apr 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors added a commit that referenced this pull request Jul 14, 2018

Auto merge of #51829 - petrochenkov:noideq, r=eddyb
Remove most of `PartialEq` and `Hash` impls from AST and HIR structures

Continuation of #49326, prerequisite for removing `PartialEq` for `Ident`.

bors added a commit that referenced this pull request Jul 14, 2018

Auto merge of #51829 - petrochenkov:noideq, r=eddyb
Remove most of `PartialEq` and `Hash` impls from AST and HIR structures

Continuation of #49326, prerequisite for removing `PartialEq` for `Ident`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.