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

rustc_span: return an impl Iterator instead of a Vec from macro_backtrace. #68407

Merged
merged 4 commits into from Jan 27, 2020

Conversation

@eddyb
Copy link
Member

eddyb commented Jan 20, 2020

Having Span::macro_backtrace produce an impl Iterator<Item = ExpnData> allows #67359 to use it instead of rolling its own similar functionality.

The move from MacroBacktrace to ExpnData (which the first two commits are prerequisites for) both eliminates unnecessary allocations, and is strictly more flexible (exposes more information).

r? @petrochenkov

src/librustc_span/hygiene.rs Outdated Show resolved Hide resolved
src/librustc_span/lib.rs Outdated Show resolved Hide resolved
src/librustc_span/hygiene.rs Outdated Show resolved Hide resolved
@eddyb eddyb force-pushed the eddyb:iter-macro-backtrace branch from cd5eaf9 to 50fc4cd Jan 21, 2020
@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Jan 21, 2020

@petrochenkov Addressed your comments, does it look good now?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 22, 2020

r=me with the clone removed

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Jan 24, 2020

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 24, 2020

📌 Commit 43b46ac has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 24, 2020

⌛️ Testing commit 43b46ac with merge d47dac1...

bors added a commit that referenced this pull request Jan 24, 2020
rustc_span: return an impl Iterator instead of a Vec from macro_backtrace.

Having `Span::macro_backtrace` produce an `impl Iterator<Item = ExpnData>` allows #67359 to use it instead of rolling its own similar functionality.

The move from `MacroBacktrace` to `ExpnData` (which the first two commits are prerequisites for) both eliminates unnecessary allocations, and is strictly more flexible (exposes more information).

r? @petrochenkov
@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Jan 24, 2020

💔 Test failed - checks-azure

@eddyb

This comment was marked as resolved.

Copy link
Member Author

eddyb commented Jan 24, 2020

Looks like a Cargo test hardcodes this error message:

"[..]\"message\":\"recursion limit reached while expanding the macro `m`\"[..]",

Not sure how to proceed with Cargo, the diagnostic diff is roughly like this:

-recursion limit reached while expanding the macro `m`
+recursion limit reached while expanding `m!`

The change was made for consistency and simpler implementation (!, attribute syntax, etc. are added in ExpnKind::descr to generate e.g. foo!, #[bar], #[derive(Baz)], so that different diagnostics can use the same style), so I'd prefer keeping it.

I assume one suboptimal fix would be removing "the macro m" from the Cargo test.
A slightly better one could be to use a regex (old|new) pattern (if that is available).

EDIT: opened rust-lang/cargo#7826 for a simple workaround.

bors added a commit to rust-lang/cargo that referenced this pull request Jan 24, 2020
test: allow some flexibility in check::error_from_deep_recursion's expected diagnostic.

This should unblock rust-lang/rust#68407, by loosening the expected output pattern.

As per rust-lang/rust#68407 (comment), this is the change in the diagnostic:
```diff
-recursion limit reached while expanding the macro `m`
+recursion limit reached while expanding `m!`
```

Ideally I would use something like this regex:
```
recursion limit reached while expanding (the macro `m`|`m!`)
```
but AFAIK these tests don't support regexes.
@eddyb eddyb force-pushed the eddyb:iter-macro-backtrace branch from 43b46ac to 9dec58f Jan 24, 2020
@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Jan 24, 2020

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 24, 2020

📌 Commit 9dec58f has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 24, 2020

⌛️ Testing commit 9dec58f with merge db5e6d5...

bors added a commit that referenced this pull request Jan 24, 2020
rustc_span: return an impl Iterator instead of a Vec from macro_backtrace.

Having `Span::macro_backtrace` produce an `impl Iterator<Item = ExpnData>` allows #67359 to use it instead of rolling its own similar functionality.

The move from `MacroBacktrace` to `ExpnData` (which the first two commits are prerequisites for) both eliminates unnecessary allocations, and is strictly more flexible (exposes more information).

r? @petrochenkov
@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Jan 24, 2020

💔 Test failed - checks-azure

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 25, 2020

Some tests in clippy failed and it's a no tool breakage week.

Marking as waiting on author because some regressions in rustfix tests there look legitimate.

@eddyb

This comment was marked as resolved.

Copy link
Member Author

eddyb commented Jan 25, 2020

Looks like it also has the same unidiom I found inside rustc_lint:

clippy_lints/src/utils/mod.rs:760-764

            let data = span.ctxt().outer_expn_data();
            let mac_name = data.kind.descr();
            let new_span = data.call_site;

            if mac_name.as_str() == name {

clippy_lints/src/utils/mod.rs:787-791

        let data = span.ctxt().outer_expn_data();
        let mac_name = data.kind.descr();
        let new_span = data.call_site;

        if mac_name.as_str() == name {

This can be fixed before this PR (as they can just compare/match on data.kind), I'll try to do that.
EDIT: opened rust-lang/rust-clippy#5096.

bors added a commit to rust-lang/rust-clippy that referenced this pull request Jan 26, 2020
Don't use ExpnKind::descr to get the name of a bang macro.

This is the same change as the first commit in rust-lang/rust#68407, but applied to clippy.
The new code should work both before and after the changes in rust-lang/rust#68407.
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jan 26, 2020
Don't use ExpnKind::descr to get the name of a bang macro.

This is the same change as the first commit in rust-lang/rust#68407, but applied to clippy.
The new code should work both before and after the changes in rust-lang/rust#68407.

changelog: none
@eddyb eddyb force-pushed the eddyb:iter-macro-backtrace branch from 9dec58f to 6980f82 Jan 26, 2020
@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Jan 26, 2020

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 26, 2020

📌 Commit 6980f82 has been approved by petrochenkov

@matthiaskrgr

This comment has been minimized.

Copy link
Member

matthiaskrgr commented Jan 26, 2020

according to the diff, this PR also changes submodules, is this intended?

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Jan 26, 2020

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 26, 2020

⌛️ Testing commit 6980f82 with merge a237641...

bors added a commit that referenced this pull request Jan 26, 2020
rustc_span: return an impl Iterator instead of a Vec from macro_backtrace.

Having `Span::macro_backtrace` produce an `impl Iterator<Item = ExpnData>` allows #67359 to use it instead of rolling its own similar functionality.

The move from `MacroBacktrace` to `ExpnData` (which the first two commits are prerequisites for) both eliminates unnecessary allocations, and is strictly more flexible (exposes more information).

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing a237641 to master...

@bors bors added the merged-by-bors label Jan 27, 2020
@bors bors merged commit 6980f82 into rust-lang:master Jan 27, 2020
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20200126.13 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 27, 2020

📣 Toolstate changed by #68407!

Tested on commit a237641.
Direct link to PR: #68407

🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 27, 2020
Tested on commit rust-lang/rust@a237641.
Direct link to PR: <rust-lang/rust#68407>

🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).
@eddyb eddyb deleted the eddyb:iter-macro-backtrace branch Jan 27, 2020
@ehuss ehuss mentioned this pull request Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.