Skip to content

Change the way we iterate for relative scopes #2128

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

Open
Tracked by #2134
pokey opened this issue Dec 12, 2023 · 10 comments
Open
Tracked by #2134

Change the way we iterate for relative scopes #2128

pokey opened this issue Dec 12, 2023 · 10 comments

Comments

@pokey
Copy link
Member

pokey commented Dec 12, 2023

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:

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

@pokey
Copy link
Member Author

pokey commented Dec 12, 2023

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 Reevaluate iteration order for scopes that start or end at the same position #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

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Dec 14, 2023

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)

@pokey
Copy link
Member Author

pokey commented Dec 14, 2023

Yeah file should be iteration scope for statement. We should def add that with a test

@pokey
Copy link
Member Author

pokey commented Dec 14, 2023

@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:

@AndreasArvidsson does this match our discussion? cc/ @josharian

@AndreasArvidsson
Copy link
Member

Looks good

@pokey
Copy link
Member Author

pokey commented Dec 14, 2023

Note also that #2137 becomes much more natural: we just apply "grand" to iteration scope and then proceed as normal

@pokey
Copy link
Member Author

pokey commented Dec 18, 2023

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))

@bra1nDump
Copy link
Collaborator

bra1nDump commented Dec 19, 2023

CleanShot 2023-12-19 at 03 02 04@2x

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

@pokey
Copy link
Member Author

pokey commented Dec 21, 2023

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

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Dec 21, 2023

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.

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

Successfully merging a pull request may close this issue.

3 participants