github / stack-graphs

Rust implementation of stack graphs
https://docs.rs/stack-graphs/*/stack_graphs/
Apache License 2.0
717 stars 122 forks source link

Documentation: Clarify doc-comments on 'shadows()' functions #425

Open Cody-Duncan opened 3 months ago

Cody-Duncan commented 3 months ago

TL;DR: Recommend updating the documentation on shadows() functions to define what the verb 'shadows' means, and to explicate that this operation is not a readonly relational property of two paths but instead a mutating iteration step along them (?).

Use Case: Understanding the system. Mostly for developers working on it, but useful to users trying to understand the API more deeply.

How I got here: I was reading fn run_defined() in stack-graphs/stack-graphs/src/assert.rs to try and figure out how to use this library to implement 'go to definition' or 'find all references'. I could not find any direct example of how to use the library's API to implement these features, and the test harnesses (which are amazing for testing, btw) somewhat obfuscate the core logic. I want to say that find_all_complete_partial_paths() is the function I should use to implement 'go to definition', but I'm not 100% sure. That call is followed by a check against shadows() to aggregate a list of actual_paths, so I dug into what 'shadows' means and found the documentation unclear.

Problem:

For these functions

The documentation states "Returns whether one (thing = edge | edge list | path) shadows another. Note that shadowing is not commutative — if thing A shadows thing B, the reverse is not true."

This is ambiguous. The documentation doesn't define what the verb "shadows" means.

What does shadows() mean?

My two initial ideas for what 'shadows()' means were:

1) refers to "variable shadowing" - Variable x is defined twice, at a global scope (at #A) and a local scope (at #B).
We can say that he inner variable in the local scope (at #B) shadows the outer variable in the global scope (at #A).

x = 0          #A
def outer():
    x = 1      #B

2) means "Supersets" or "Encompasses" - The graph has a route from A->B->C->D->E.
PartialPath #1 represents the route A->E. PartialPath #2 represents the route from B->D. PartialPath #3 represents the route from A->C. We could say that PartialPath #1 "supersets" or "encompasses" PartialPaths #2 and #3.

Understanding the Implementation of shadows()

Reading the implementation and writing down what it does:

For self's and other's PartialPathEdgeLists, They must must match Nodes at the front of the list. So if the two lists were L1:[A,B] and L2:[A,D], L1.shadows(L2) -> true.
If the front nodes mismatch, E.g. L1:[A,B] vs L2:[B,C] -> return false.
If self's front-node matches other's front-node and had greater precedence, E.G. L1:[+A,B] and L2:[-A,B]-> return true immediately.
If self's front-node matches other's front-node but has lesser precedence, E.G. L1:[-A,B] and L2:[+A,B] -> this is the only case that iterates to the next node in each list.
NOTE: each iteration step is destructive. PartialPathEdgeList::shadows() takes mutable PartialPathEdgeLists, pops the front node from each PartialPathEdgeList, and continues to do so in the looping case. PartialPathEdge::shadows() is readonly an non-destructive.

Assuming that read of it is correct, then the documentation is a bit misleading. The behavior is not the entire PartialPath shadowing another entire PartialPath, it's only the front node of each list. The description sounds like a relationship property, e.g. 'larger than', which are usually readonly operations; this makes state changes that aren't mentioned by the documentation (with the exception of PartialPathEdge::shadows(), which is readonly and returns a bool like a relationship property).

Thanks for your consideration. ^_^ If I figure out what shadows() means on my own, I'll make a Pull Request to update the documentation.