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

feat: Map tokens from include! expansion to the included file #14561

Merged
merged 1 commit into from
Apr 13, 2023
Merged

feat: Map tokens from include! expansion to the included file #14561

merged 1 commit into from
Apr 13, 2023

Conversation

jonas-schievink
Copy link
Contributor

Fixes #3767

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2023
@jonas-schievink jonas-schievink changed the title Map tokens from include! expansion to the included file feat: Map tokens from include! expansion to the included file Apr 13, 2023
@Veykril
Copy link
Member

Veykril commented Apr 13, 2023

@bors r+
That was a lot simpler than I would've expected it to be

@bors
Copy link
Contributor

bors commented Apr 13, 2023

📌 Commit 901c8a4 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 13, 2023

⌛ Testing commit 901c8a4 with merge 08ce44e...

@davidbarsky
Copy link
Contributor

just have to say: this is awesome and i'm really thankful you managed to make this change.

@bors
Copy link
Contributor

bors commented Apr 13, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 08ce44e to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Apr 13, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 08ce44e to master...

@bors bors merged commit 08ce44e into rust-lang:master Apr 13, 2023
@bors
Copy link
Contributor

bors commented Apr 13, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@jonas-schievink jonas-schievink deleted the goto-included-file branch April 13, 2023 16:40
@NickLarsenNZ
Copy link

NickLarsenNZ commented Apr 14, 2023

I'm running the nightly version (I believe, based on version: 0.4.1477-standalone (10e0aaf28 2023-04-14) in the output, and having switched to the release version of the extension and reloading vscode).

I am getting the following error when trying to jump to the code:

thread 'Worker' panicked at 'Bad offset: range 0..319 offset 153598', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rowan-0.15.11/src/cursor.rs:751:9
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: rowan::cursor::SyntaxNode::token_at_offset
   3: ide::goto_implementation::goto_implementation
   4: std::panicking::try
   5: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut
   6: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter
   7: rust_analyzer::handlers::handle_hover
   8: std::panicking::try
   9: <F as threadpool::FnBox>::call_box
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
[Error - 22:24:15] Request textDocument/hover failed.
  Message: request handler panicked: Bad offset: range 0..319 offset 153598
  Code: -32603 

This happens when I hover over dapr_v1::TopicEventResponse

And when I CMD+Click to follow it anyway, it jumps to the end of the file where the tonic::include_proto! is called.

I have tried with the following setting in vscode too (to no avail):

{
    "rust-analyzer.cargo.buildScripts.enable": true,
}

I'm unsure if the build.rs is of any significance in this issue.

@lnicola
Copy link
Member

lnicola commented Apr 17, 2023

include-go-to-def.mp4

@zhouzq-thu
Copy link

With opencv-rust crate, goto definition works fine for the functions like dnn::get_available_backends, but can't goto the right place for the traits (member functions) like model.set_preferable_backend. Maybe the rust-analyzer still has some bugs.

@lnicola
Copy link
Member

lnicola commented Apr 18, 2023

Maybe the rust-analyzer still has some bugs.

Yeah, about 1233 open ones 😬.

There are some known situations where we pick up the wrong trait member. If you could minimize it (outside of opencv-rust), that would be nice, but I don't think it's related to the handling of include!.

@TroyKomodo
Copy link

How do we enable this feature?
It does not seem to work with tonic.

@lnicola
Copy link
Member

lnicola commented Apr 22, 2023

@TroyKomodo Go to definition should work, as seen in the recording above. If it doesn't work for you and it's not #14611, please file a new issue.

@TroyKomodo
Copy link

@lnicola Does it expected with submacros?

mod generated {
    include!(concat!(env!("OUT_DIR"), "/generated.rs"))
}

@lnicola
Copy link
Member

lnicola commented Apr 22, 2023

That's what the gdal-sys crate (from the recording) does: https://github.com/georust/gdal/blob/master/gdal-sys/src/lib.rs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Goto Definition into include!
9 participants