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

Emit references and definitions for the same anchor #4021

Open
ayazhafiz opened this issue Aug 23, 2019 · 8 comments
Open

Emit references and definitions for the same anchor #4021

ayazhafiz opened this issue Aug 23, 2019 · 8 comments
Labels
typescript Issues affecting the Typescript language

Comments

@ayazhafiz
Copy link
Contributor

See a much longer discussion at #3934.

The Python indexer, and since #3990 the TypeScript indexer, deal with imports defining a local import declaration and referencing a remote definition by creating a unique anchor for the import definitions and a unique anchor for the import reference.

//- @import defines/binding LocalFooDefintion
//- @foo ref RemoteFooDefinition
import { foo } from './bar'

This is done because the most popular Kythe UI currently chooses an anchor as referencing other semantic nodes or defining a semantic node, but not both.

If the UI is extended to support both definitions and references on one anchor, this can be closed and we can emit definitions and references on the same anchor.

//- @foo defines/binding LocalFooDefintion
//- @foo ref RemoteFooDefinition
import { foo } from './bar'
@kamahen
Copy link
Contributor

kamahen commented Sep 26, 2019

Using import for the ref confuses users (it's been tried, plus a few variants).

@shahms
Copy link
Contributor

shahms commented Sep 26, 2019

IIRC from the outcome of that discussion, the test should also include:

//- @foo defines/binding LocalFooDefintion
//- @foo ref RemoteFooDefinition
//- LocalFooDefinition aliases RemoteFooDefinition
import { foo } from './bar'

@kamahen
Copy link
Contributor

kamahen commented Sep 27, 2019

A generalization of issue #3815 -- it could be desirable for the indexer to specify the ordering of edges that the UI displays. (Issue #3815 refers to a single kind of edge from a node; but it might be desirable to specify an ordering for all edges from a node.)

@robinp
Copy link
Contributor

robinp commented Oct 7, 2019

As discussed in context of Python's from foo import * syntax in TreeTide/underhood#9 (comment), is it right to say that blanket imports happening at the * should have the same reference behavior, but actually emitting the for all the imported names that are actually used in the module? See that discussion for the rationale.

Example:

// foo exports a, b, c, ..., z
//- @"*" defines/binding LocalB
//- @"*" ref RemoteB
//- LocalB aliases RemoteB
//- @"*" defines/binding LocalC
//- @"*" ref RemoteC
//- LocalC aliases RemoteC
//- !{ @"*" defines/binding LocalA }    // smoke check, among other exported names
from foo import *

//- @b ref LocalB
//- @c ref LocalC
bar = b + c

@ayazhafiz
Copy link
Contributor Author

@robinp. I think yes. And as others have mentioned, there is an issue of practicality here for the user. In something like

from foo import * # imports a, b

myval = a + b

It may be expected that clicking on a takes one to the definition of a in the foo module. Clustering references on * in the import line would make this much more cumbersome. So here, prioritization of edges and aliasing would be very useful.

@nbeloglazov
Copy link
Collaborator

What is the current status on this issue? Was it blocked on CodeSearch not properly supporting it?

@shahms
Copy link
Contributor

shahms commented Apr 6, 2020

Largely, yes. CodeSearch always prefers definitions when an anchor has both. While there are still some issues with this approach, (notably for Go's embedded fields) it resolves the specific issue referenced from this bug. It's fine for indexers to emit overlapping references and definitions; UIs should probably prefer definitions if they have to pick a single one, but should probably present users with some choice.

@shahms shahms closed this as completed Apr 6, 2020
@shahms shahms reopened this Apr 6, 2020
@shahms
Copy link
Contributor

shahms commented Apr 6, 2020

Sorry, I'm not sure if the Python indexer does this or not, but it should be able to at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Issues affecting the Typescript language
Projects
None yet
Development

No branches or pull requests

5 participants