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

Fix ICE when rustdoc is scraping examples inside of a proc macro #90583

Merged
merged 4 commits into from Nov 5, 2021

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented Nov 4, 2021

This PR provides a clearer semantics for how --scrape-examples interacts with macros. If an expression's span AND it's enclosing item's span both are not from_expansion, then the example will be scraped. The added test case rustdoc-scrape-examples-macros shows a variety of situations.

  • A macro-rules macro that takes a function call as input: good
  • A macro-rules macro that generates a function call as output: bad
  • A proc-macro that generates a function call as output: bad
  • An attribute macro that generates a function call as output: bad
  • An attribute macro that takes a function call as input: good, if the proc macro is designed to propagate the input spans

I ran this updated rustdoc on pyo3 and confirmed that it successfully scrapes examples from inside a proc macro, eg

Screen Shot 2021-11-04 at 1 11 28 PM

(cc @mejrs)

Additionally, this PR fixes an ordering bug in the highlighting logic.

Fixes #90567.

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2021
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 4, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 4, 2021

An attribute macro that takes a function call as input: good, if the proc macro is designed to propagate the input spans

Can you also test what happens if it's buggy and doesn't propagate spans? It seems fine to discard that example, I just want to make sure it doesn't cause an ICE.

@camelid
Copy link
Member

camelid commented Nov 4, 2021

Additionally, this PR fixes an ordering bug in the highlighting logic.

Could you separate that into another commit? It'll make it easier to understand why code was changed later on.

And thanks for adding a test ❤️

@rust-log-analyzer

This comment has been minimized.

src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/scrape_examples.rs Outdated Show resolved Hide resolved
src/librustdoc/scrape_examples.rs Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. requires-nightly This issue requires a nightly compiler in some way. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 4, 2021

For posterity, this fixes the following ICE when running on Pyo3 (reported on discord):

$ cargo +nightly doc -Z unstable-options -Z rustdoc-scrape-examples=examples
   Compiling pyo3 v0.14.5 (C:\Users\user\rust\pyo3)
thread 'rustc' panicked at 'assertion failed: enclosing_item_span.contains(expr_span)', src\librustdoc\scrape_examples.rs:93:9
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

error: internal compiler error: unexpected panic

error: Unrecognized option: 'scrape-examples-output-path'

error: could not document `pyo3

@camelid camelid added the A-rustdoc-scrape-examples Area: The (unstable) rustdoc scrape-examples feature described in RFC 3123 label Nov 4, 2021
@camelid
Copy link
Member

camelid commented Nov 4, 2021

(FYI, I just created an A-rustdoc-scrape-examples label to use in tracking this feature. I'll add it to some other PRs and issues too.)

@willcrichton
Copy link
Contributor Author

Could you separate that into another commit? It'll make it easier to understand why code was changed later on.

@camelid I've separated this into two commits.

Can you also test what happens if it's buggy and doesn't propagate spans? It seems fine to discard that example, I just want to make sure it doesn't cause an ICE.

@jyn514 the rustdoc-scrape-examples-macro test already does this, if I'm understanding you correctly. The #[an_attr_macro] macro is used twice, the second time with #[an_attr_macro(with_span)].

@jyn514
Copy link
Member

jyn514 commented Nov 4, 2021

r=me with CI passing :)

@bors delegate=willcrichton

@bors
Copy link
Contributor

bors commented Nov 4, 2021

✌️ @willcrichton can now approve this pull request

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2021
@willcrichton
Copy link
Contributor Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Nov 4, 2021

📌 Commit 4846d10 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 4, 2021
…n514

Fix ICE when rustdoc is scraping examples inside of a proc macro

This PR provides a clearer semantics for how --scrape-examples interacts with macros. If an expression's span AND it's enclosing item's span both are not `from_expansion`, then the example will be scraped. The added test case `rustdoc-scrape-examples-macros` shows a variety of situations.

* A macro-rules macro that takes a function call as input: good
* A macro-rules macro that generates a function call as output: bad
* A proc-macro that generates a function call as output: bad
* An attribute macro that generates a function call as output: bad
* An attribute macro that takes a function call as input: good, if the proc macro is designed to propagate the input spans

I ran this updated rustdoc on pyo3 and confirmed that it successfully scrapes examples from inside a proc macro, eg

<img width="1013" alt="Screen Shot 2021-11-04 at 1 11 28 PM" src="https://user-images.githubusercontent.com/663326/140412691-81a3bb6b-a448-4a1b-a293-f7a795553634.png">

(cc `@mejrs)`

Additionally, this PR fixes an ordering bug in the highlighting logic.

Fixes rust-lang#90567.

r? `@jyn514`
@mejrs
Copy link
Contributor

mejrs commented Nov 4, 2021

Is this supposed to draw code examples from /tests? That doesn't sound right.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 4, 2021
…n514

Fix ICE when rustdoc is scraping examples inside of a proc macro

This PR provides a clearer semantics for how --scrape-examples interacts with macros. If an expression's span AND it's enclosing item's span both are not `from_expansion`, then the example will be scraped. The added test case `rustdoc-scrape-examples-macros` shows a variety of situations.

* A macro-rules macro that takes a function call as input: good
* A macro-rules macro that generates a function call as output: bad
* A proc-macro that generates a function call as output: bad
* An attribute macro that generates a function call as output: bad
* An attribute macro that takes a function call as input: good, if the proc macro is designed to propagate the input spans

I ran this updated rustdoc on pyo3 and confirmed that it successfully scrapes examples from inside a proc macro, eg

<img width="1013" alt="Screen Shot 2021-11-04 at 1 11 28 PM" src="https://user-images.githubusercontent.com/663326/140412691-81a3bb6b-a448-4a1b-a293-f7a795553634.png">

(cc `@mejrs)`

Additionally, this PR fixes an ordering bug in the highlighting logic.

Fixes rust-lang#90567.

r? `@jyn514`
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 5, 2021
@bors
Copy link
Contributor

bors commented Nov 5, 2021

⌛ Testing commit c62817b with merge 86e2b3bd0cb2eff1a3502fd66296a3f3d8e480da...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 5, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 5, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 5, 2021

@willcrichton every platform names the suffix for dynamic libraries something different - you can either try to make it work in the makefile (with $(RUSTC) --crate-name foo --crate-type dylib --print file-names, see the trouble I've been having in https://github.com/rust-lang/rust/pull/88368/files#diff-33be197db3e3ddb089ba3380ea3d6ce10944ff5517531547039f0fa6c06216e3) or just make it linux-only.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
@willcrichton
Copy link
Contributor Author

Ok I did your suggested change @jyn514.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 5, 2021

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Nov 5, 2021

📌 Commit 82b23be has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
@bors
Copy link
Contributor

bors commented Nov 5, 2021

⌛ Testing commit 82b23be with merge 0d1754e...

@bors
Copy link
Contributor

bors commented Nov 5, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 0d1754e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2021
@bors bors merged commit 0d1754e into rust-lang:master Nov 5, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 5, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0d1754e): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -0.7% on full builds of webrender)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@jyn514 jyn514 deleted the example-analyzer branch November 5, 2021 23:56
@jyn514 jyn514 restored the example-analyzer branch November 5, 2021 23:56
@jyn514 jyn514 deleted the example-analyzer branch November 5, 2021 23:56
@jyn514 jyn514 restored the example-analyzer branch November 5, 2021 23:58
@camelid
Copy link
Member

camelid commented Nov 6, 2021

I guess fetching the span was quite slow and moving it to after the expansion check (and other checks) improved performance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-scrape-examples Area: The (unstable) rustdoc scrape-examples feature described in RFC 3123 merged-by-bors This PR was explicitly merged by bors. requires-nightly This issue requires a nightly compiler in some way. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ice with rustdoc scrape_examples
10 participants