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

feature request: xref jump to C source for function if src installed #499

Open
nverno opened this issue Mar 9, 2018 · 17 comments
Open

feature request: xref jump to C source for function if src installed #499

nverno opened this issue Mar 9, 2018 · 17 comments

Comments

@nverno
Copy link
Contributor

nverno commented Mar 9, 2018

The new support for xref is great. It would be cool if xref would jump to C source as well, as it does in emacs-lisp. For example if an equivalent variable like C-source-directory was available to search from.

@nverno
Copy link
Contributor Author

nverno commented Mar 9, 2018

I might be able to take a stab at this next week if it was deemed worthy.

@lionel-
Copy link
Member

lionel- commented Mar 9, 2018

how would you find out which do_ function you need to jump to?

@lionel-
Copy link
Member

lionel- commented Mar 9, 2018

I guess https://github.com/jimhester/lookup is able to do it so it should be possible. Then you just need a TAGS file in the R source and use xref to jump to the right definition.

@nverno
Copy link
Contributor Author

nverno commented Mar 9, 2018

you're right, I currently use a TAGS file generated with universal-ctags and it doesn't always jump to the correct locations to be sure. It does seem to do a pretty decent job though, and the xref interface to list possible jump points is pretty helpful. Honestly, I haven't really thought to through to much, I just thought it would be a pretty sweet feature.

The R source follows a pretty standardized naming scheme right? Would it be good enough to use the naming scheme to determine jump points?

@lionel-
Copy link
Member

lionel- commented Mar 9, 2018

the R source follows a pretty standardized naming scheme right?

I don't think so.

I just thought it would be a pretty sweet feature.

Agreed it would be nice. I guess for internal functions you'd first jump to the closure wrapper in src/library/pkg and then jump to the C code with the cursor placed on the .Internal() first arg.

@nverno
Copy link
Contributor Author

nverno commented Mar 9, 2018

Ok that's a bummer, I had thought there was a convention as you mentioned previously like do_lapply, do_* etc for exported functions. I hadn't known about that lookup package, Ill try to look more deeply into it in a few days. Universal ctags does a decent job, but highly doubt youd want another dependency.

@vspinu
Copy link
Member

vspinu commented Mar 10, 2018

I think this might be a huge complication. I would prefer to keep tags separately from our xref backend. TAGS could be anywhere and multiple tag files (R sources + package sources) could be active at the same time. Users might have several versions of R source repo lying around etc. We really don't want to get into tag file management.

There have been endless discussions on emacs devel about how to properly aggregate backends and the crux is that it's not possible to have a satisfactory solution. Add to this that either functions could be named identically in R and C code or, there is no perfect mapping between R and C level symbols and various indirections exists (core R do_ scheme or exports automatically generated by Rcpp) and one quickly ends up in a mess.

So my suggestion would be to stick with different shortcuts for different lookup mechanisms (I use M-g M-t for tags, M-. for ide specific backend, M-o for imenu-anywhere etc). Usually you know exactly what you want, a C-symbol or an R symbol, so why mix these two for the sake of having one messy shortcut? Why would you want to go through the xref disambiguation menu if you already know from the start that you want the C symbol?

@lionel-
Copy link
Member

lionel- commented Mar 10, 2018

There should be only one kind of TAGS file in the R-source (for C files) so I don't think that's where the difficulty lies. There would be no xref disambiguation menu in the scheme I'm imagining. But how do you map the base function symbols to their R source, and how do you find out about the relevant C symbol is the hard part. As for compiled code in packages that's another separate messy issue, but here again the difficulty is not xref but finding out about relevant symbols.

@vspinu
Copy link
Member

vspinu commented Mar 10, 2018

There should be only one kind of TAGS file in the R-source (for C files)

One for R itself, one for each package. I have always multiple tag files loaded at the same time.

There would be no xref disambiguation menu in the scheme I'm imagining.

I use same function names in C files and R wrappers. So does Rcpp::compileAttributes when generating exports. So, for 99% of the users who develop with Rcpp the disambiguation menu will pop up.

When you are on print.default in

    .Internal(print.default(x, digits, quote, na.print, print.gap, 

do you want to src of R's print.default or do_printdefault? The user could want either, so menu should be shown. Having separate shortcuts for these two options is uniformly better UI and, I believe, we should encourage it.

R source, and how do you find out about the relevant C symbol is the hard part

"Relevant" in what sense? Do you have in mind jumping from R code from .Call or .Primitive invocations? If so, then I could see how current xref etags backend could be improved a bit by falling back on apropos and searching for the partial match at point (not always works, as with print.default example). Currently one has to type the partial match, but that doesn't bother me that much.

code in packages that's another separate messy issue,

Most of the users will be interested in C/C++ locations of their packages and not not in R sources. So, if implemented, it should first work within current package as the baseline.

again the difficulty is not xref

It's not a problem with xref as long as you use it for one type of lookup, but once you start using it for a bunch of things which are impossible to disambiguate from context then you quickly end up with problems or, in best case scenario, with a disambiguation menu.

@lionel-
Copy link
Member

lionel- commented Mar 10, 2018

When you are on print.default in

We'd detect .Internal() and jump right away to the relevant definition based on R's own TAGS file. Same if we are on the first argument of .Call(). This way there'd be no disambiguation.

Given how good srcrefs lookup is, I'm thinking typical workflow would never rely on R symbols in a TAGS file ad there'd only be one such files relevant at any moment. I no longer think we need to combine the xref backends to have a good workflow.

So, if implemented, it should first work within current package as the baseline.

I agree package lookup is more useful but if someone wants to implement R lookup they're welcome to do it.

@lionel-
Copy link
Member

lionel- commented Mar 10, 2018

We could also detect .Call() and .Internal() when jumping to an R function and position the cursor on their first arg to make it easy to jump to compiled code.

Edit: And then we can have an option to automatically rejump from there. And if you wanted to see the closure wrapper you'd run xref-pop-marker-stack to go back one step.

@lionel-
Copy link
Member

lionel- commented Mar 10, 2018

Also jumping on the right entry of the definition table in names.c (for R) or init.c (for packages) based on a regexp search would already be a very good step, it's easy to manually jump on the relevant C/C++ symbol afterwards. For packages we'd need to search within src to detect where the table is defined.

@vspinu
Copy link
Member

vspinu commented Mar 10, 2018

g typical workflow would never rely on R symbols in a TAGS file ad there'd only be one such files relevant at any moment.

This could be doable if we could identify package from where the symbol originates. PACKAGE arg is optional in .Call.

Please take into account what happens when xref-prompt-for-identifier is set to t if you decide to take a stab at this.

I no longer think we need to combine the xref backends to have a good workflow.

Combining backends or doing it ourselves, we have to solve same issue - how to combine and disambiguate. By "disambiguation menu" I meant how do we make distinction between R symbol and C symbol with the same name within our own backend. I might want to jump to do_printdefault from other locations, not just from within .Call (I do have xref-prompt-for-identifier set to t).

e cursor on their first arg to make it easy to jump to compiled code

Not all functions using .Call are wrappers. Some might use multiple .Calls.

I agree package lookup is more useful but if someone wants to implement R lookup they're welcome to do it.

Then let's stick to this minimal plan. Let's make C symbol lookup within current package work first. Ideally with automatic TAGS generation whenever possible.

@vspinu
Copy link
Member

vspinu commented Mar 10, 2018

Also jumping on the right entry of the definition table in names.c (for R) or init.c (for packages) based on a regexp search would already be a very good step, it's easy to manually jump on the relevant C/C++ symbol afterwards.

How do you actually jump afterwards?

@lionel-
Copy link
Member

lionel- commented Mar 10, 2018

How do you actually jump afterwards?

By moving the cursor on the function pointer and calling xref gain. In that C file the xref backend uses TAGS. But it's probably easy to write some regex that detects the function pointer and jump automatically. Those tables are well standardised.

This could be doable if we could identify package from where the symbol originates. PACKAGE arg is optional in .Call.

Hmm I didn't think of that. But that interface will be deprecated in the long term I think, R is shifting towards native symbol pointers. I think it's ok to only support the new interface.

Some might use multiple .Calls.

That doesn't seem like good practice. Is that a widespread idiom? If you're on the R side you can afford one indirection and call a wrapper. Anyway we can detect multiple .Call() and stop the jump there.

Combining backends or doing it ourselves, we have to solve same issue - how to combine and disambiguate. By "disambiguation menu" I meant how do we make distinction between R symbol and C symbol with the same name within our own backend. I might want to jump to do_printdefault from other locations, not just from within .Call (I do have xref-prompt-for-identifier set to t).

My feeling is that we'd actually have better experience by not combining all symbols from different languages. If you're in a R file you'd only get the R symbols, and in a C/C++ file you'd only get the native symbols. Then if we can pull off the UI I suggested above, and you want to go to the native side, you'd jump to the closure wrapper and ESS would automatically get you to the right place on the native side. If you want to jump to a function that is not exposed on the R side you'd just switch to a native file (e.g. with projectile) and jump from there.

@vspinu
Copy link
Member

vspinu commented Mar 10, 2018 via email

@lionel-
Copy link
Member

lionel- commented Mar 10, 2018

The automatic jumping to the .Call inside a function doesn't sound at all right. If I xref to a function I expect to be placed at the beginning of it.

Let's not discuss at length about the details. The important feature is to be able to jump to the right place from a jump within .Call(). Let's discuss the UI if/when we have concrete code for this.

In my mind such UI and the trickeries which you proposed are inferior to the simple two-shortcuts solutions what I use currently.

What you call trickeries I call DWIM. I don't think it's inferior, it's orthogonal. Indeed having multiple keybindings for different xref backends is a legit workflow and trivial to implement. It might make sense to provide keybindings for this in ESS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants