bug: no J2D or find-refs of switch-case blocks over an object's type

[shrouxm]

Code like the following:

switch v := obj.(type) {
  case A: f1(v, v)
  case B: f2(v, v)
}

does not get indexed properly because when processing definitions, the definition for v has a nil associated def object, since there's a different def object for the v in each branch. Then when we encounter each use of v, there is an associated def object but since it's not indexed in our definitions we only output a hover result.

Discussion:

• If there's a way to know that this is why the def object is nil during the definitions pass, that would likely be the right way to solve.
• What LSIF should we actually output here? IMO:
  • Each case branch should have a different hover result cause they have different types.
  • The def result is canonical.
  • I think the reference result should point to all uses in every branch. Kind of weird to have a reference return results with different types, but I think the experience would be weirder if you hover over one of these and only the symbols in that branch highlight. Also the definition would need to have everything as a reference result anyway.
  • Open question: what should go in the hover result when hovering the definition?

Therefore the resulting LSIF structure should be:

vdef -> resultSet1
resultSet1 -> hoverResult1
resultSet1 -> resultSetShared
resultSetShared -> defResult1
resultSetShared -> refResult1
defResult1 -> vdef
refResult1 -> vdef
vref1 -> resultSet2
resultSet2 -> hoverResult2
resultSet2 -> resultSetShared
refResult1 -> vref1
vref2 -> resuiltSet2
refResult1 -> vref2
vref3 -> resultSet3
resultSet3 -> hoverResult3
resultSet3 -> resultSetShared
refResult1 -> vref3
vref4 -> resuiltSet3
refResult1 -> vref4

Thoughts?

Also how does the worker process this graph?

[efritz]

Looking into this found some curious comments that are influencing this data to be missing in the graph:

type types.Info struct {
	// Defs maps identifiers to the objects they define (including
	// package names, dots "." of dot-imports, and blank "_" identifiers).
	// For identifiers that do not denote objects (e.g., the package name
	// in package clauses, or symbolic variables t in t := x.(type) of
	// type switch headers), the corresponding objects are nil.
	//
	// For an embedded field, Defs returns the field *Var it defines.
	//
	// Invariant: Defs[id] == nil || Defs[id].Pos() == id.Pos()
	Defs map[*ast.Ident]Object

	// Implicits maps nodes to their implicitly declared objects, if any.
	// The following node and object types may appear:
	//
	//     node               declared object
	//
	//     *ast.ImportSpec    *PkgName for imports without renames
	//     *ast.CaseClause    type-specific *Var for each type switch case clause (incl. default)
	//     *ast.Field         anonymous parameter *Var (incl. unnamed results)
	//
	Implicits map[ast.Node]Object
}

[efritz]

I'm pretty sure at this point I've followed your exact steps up to https://github.com/sourcegraph/lsif-go/compare/garo/index-specific-files#diff-1913594ad09438ab1869eafef027bcd6R311 🤦 I basically made all the same changes as in your branch.

Well at least I learned a lot!

[efritz]

To confirm, your understanding, @gbrik , the following code has the following (relevant portion) LSIF graph.

package main

func main() {
	var v interface{} = 3

	switch e := v.(type) {
	case int:
		e++
	case bool:
		if e {
			// TODO
		}
	default:
	}
}

Definitions and references are well-formed (all point to the top e :=, and the others are references: I think this is the correct/expected behavior.

I think the only issue here is that the hover text for e can't be the first one that gets indexed (it doesn't even seem deterministic). I think fixing this is as easy splitting the hover results from the result set and instead put them directly on the ranges.

---

To explain how the worker would handle this: definition result id, reference result id, and hover result ids attached to a range will be the first reachable one (attached directly, or attached to a depth-1 result set, or attached to a depth-2 result set, etc). Every moniker reachable for a range belongs to that range (attached to the range, an attached result set, or another attached moniker).

[shrouxm]

@efritz ohh, i didn't notice the Implicits part though! probably each case in the comment needs a little somethin somethin extra?

everything you wrote SGTM, think we're on the same page.

re the worker, so if e.g. multiple reference results are attached at different depths, only the shallowest will actually end up attached? specifically I'm wondering if the worker has logic for, say, range1 points to refResult1, range2 points to refResult2, range3 points to both, will it create a new merged refResult on the fly?

[efritz]

I didn't notice the Implicits part though.

New to me until 2 hours ago too!

specifically I'm wondering if the worker has logic for, say, range1 points to refResult1, range2 points to refResult2, range3 points to both, will it create a new merged refResult on the fly?

Ranges and result set properties get "flattened" as part of canonicalization. While a range doesn't have a non-zero valued property, it will try to find one by depth as it walks the graph. After all these are flattened into the ranges we can get rid fo the concept of result sets and next edges.

[efritz]

Attempted to fix this in #122. It works much better but there's a TODO note we need to figure out how to solve (missing hover text for the definition of the symbolic variable in the type switch header itself).