cursorless
cursorless copied to clipboard
Change the way we iterate for relative scopes
The problem
Today, the iteration semantics for relative scopes differs from the semantics for "every" and ordinal scopes. The latter first expand to iteration scope (if input target has no explicit range), and then get a list of all top-level scopes in that iteration scope. The former iterate directly from the input target, with no knowledge of iteration scopes. These semantics lead to the following problems:
- https://github.com/cursorless-dev/cursorless/issues/1597
- https://github.com/cursorless-dev/cursorless/pull/2110
- @josharian's riddle of "take scope take next scope take scope" not being equivalent to "take scope"
The solution
If the scope has no iteration scope, maybe we just leave today's behaviour?
Otherwise, we first want to find the proper iteration scope in which to iterate:
- For relative exclusive, we expand input target to smallest containing iteration scope
- For relative inclusive, we expand input target to smallest containing scope, then expand from there to smallest containing iteration scope. That handles the case where you're between scopes in the very smallest iteration scope, eg
if (true) { console.log(1) | console.log(2) } console.log(2)
Then we do the same thing we do with "every" / ordinal, where we construct a list of all scopes in the iteration scope. These are the scopes we use instead of directly working with the scope handler.
Note that if when iterating through the returned scopes, we run out, I think we move up to grand iteration scope and start over. Repeat that as many times as necessary
Thought about this a bit more, and a few issues:
- It solves @josharian's riddle kinda by accident. It effectively just switches things so we iterate forwards, and python doesn't have scopes that start at the same point, just ones that end at the same point. If we decided to iterate backwards when constructing the list, we'd still have the same problem. So feels like it's maybe not addressing the root problem here. Maybe https://github.com/cursorless-dev/cursorless/issues/1597 approach of changing iteration order to go biggest first does have something to it
- It is a bit tough to explain, tho the consistency is nice
- It doesn't really make sense for scopes that aren't hierarchical / don't have hierarchical iteration scope. Eg if you say "next token" at the end of a line, the suggested approach of trying to walk up iteration scope doesn't help you because there's only one level and you've exhausted it
I don't think these are necessarily showstoppers, but just makes me think we maybe haven't gotten to the bottom of this one yet
I started looking into this and of course ran into problem immediately :D With this example. There is no iteration scope for statement. I was thinking the entire program file as well as statement blocks sound reasonable.
if ( | true) {
console.log(1)
}
console.log(2)
Yeah file should be iteration scope for statement. We should def add that with a test
@AndreasArvidsson and I had a long discussion about this one. Here are our takeaways:
First of all, we like the idea proposed in this PR. It has nice intuitive characteristics wrt "next" whereby sitting in the "header" of a structure should skip its body, but sitting in the body should not.
We had a tangent where we thought maybe "interior" was the notion we actually wanted. The idea would be that for "next", you first expand input target to containing scope, and if that succeeds and you're not in its interior, you iterate starting from the end of the containing scope, otherwise iterate from input target as usual. In most cases, this would actually result in the same behaviour as the proposal in this issue. However, consider the scope type "arg", in the following code:
function aaa(bbb, ccc = ddd(eee, fff), ggg) {
...
}
We decided that "next arg" should be ggg from anywhere in ccc = ddd, but fff from anywhere in eee,. The iteration scope criterion works here, but the interior criterion doesn't, because "arg" doesn't have a natural interior.
In addition, the conistency with "every" is desirable, and the interior criterion would lose that.
In order for this to work well, it is important that if a scope is hierarchical, there is always an iteration scope between a scope and its parent. Ie Let $S$ be a scope type and $I(S)$ be its iteration scope type. If scopes $A$ and $B$ are of type $S$, and $A$ contains $B$, then there exists $C$ of type $I(S)$ such that $A$ contains $C$ and $C$ contains $B$.
The above criterion causes hierarchical scopes to devolve to hierarchies of linear scopes. Each scope exists on a flat plane with other scopes of that plane, and within that plane the scope type behaves like a flat scope, so that "next scope", "two scopes", "every scope" all work as intuitively as flat scopes
We would want to update the scope visualizer to be aware of these scope planes. For hierarchical scopes, your cursor position would determine which plane you're in by expanding to containing iteration scope and only showing scopes in that iteration scope, ignoring any scopes from descendant iteration scopes. We would have a way to indicate the available iteration scopes, but you'd only see actual scopes for one iteration scope at a time.
We've had complaints that deeply nested scopes are very hard to understand in our scope visualizer, so this might be both a visual / mental model win
It's also quite important then that we have good iteration scopes for every hierarchical scope
So the steps are:
- [ ] Add proper iteration scopes for our most popular languages, and good facets to track what they should be
- [ ] Update scope visualizer as described above
- [ ] https://github.com/cursorless-dev/cursorless/pull/2133
@AndreasArvidsson does this match our discussion? cc/ @josharian
Looks good
Note also that https://github.com/cursorless-dev/cursorless/issues/2137 becomes much more natural: we just apply "grand" to iteration scope and then proceed as normal
just had an example where I might not have wanted this behaviour:
aaa(bbb)
I had my cursor at the end of aaa and wanted to delete bbb, so said "chuck next arg". I think I would have been surprised if that behaved differently if the line were wrapped in a call, eg ccc(aaa(bbb))
My understanding of the desired functionality is:
- Be able to traverse siblings on the plane ignoring children
- Be able to traverse a flat list, this conflicts with the previous expectation (depth first search)
- Once all children are traversed pop and select containing parent
I feel like going to the parent once we have exhausted either all siblings or all children is non objectionable. I feel like being able to support both sibling only, as well as de first search both seem like reasonable expectations. The question is what should we default to?
- To avoid the issue brought in the last comment defaulting to flat will make sense.
For the unfortunate not default option we can add a meta modifier like take next sibling arg or take next flat arg
And here's another where I found today's "next" behaviour useful:
function usingSetting<T, U>(
section: string,
setting: string,
factory: (value: T) => U,
): U {
return vscodeApi.workspace.getConfiguration(section).get<boolean>(setting);
}
My cursor was in the function signature, and I just said "change next state" to change that return statement. I'm starting to get the feeling our intuitions maybe just differ here? I think in terms of today's implementation, where it is just dumb and moves forward until a statement starts
It's not only about intuition. That ordinal and relative behaves differently is 95% of the problem. Now you need two different intuitions which just doesn't work for me.
But for that example I don't agree. Next statement shouldn't be a child of the current in my mind.