Skip to content

Paths: Add maxCircularDepth option #1079

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
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

LaurynasGr
Copy link
Contributor

Adds maxCircularDepth option to Paths

@LaurynasGr LaurynasGr marked this pull request as draft March 4, 2025 22:04
@LaurynasGr LaurynasGr marked this pull request as ready for review March 4, 2025 23:15
@som-sm
Copy link
Collaborator

som-sm commented Mar 5, 2025

@LaurynasGr Types like the following shouldn't be affected by maxCircularDepth, but they currently do.

type Test = {
	foo: string;
	bar: {
		foo: string;
		bar: {};
	};
};

type P = Paths<Test, {maxCircularDepth: 0}>;
//   ^? type P = 'foo' | 'bar'

@som-sm
Copy link
Collaborator

som-sm commented Mar 5, 2025

On second thought, I think this option is just going to be more confusing than helpful.

  • I doubt if there's a proper way to solve Paths: Add maxCircularDepth option #1079 (comment), because how would you differentiate b/w structurally similar and truly recursive cases? Turns out it was simple!

  • The fact that maxRecursionDepth and maxCircularDepth don't target the same depth could lead to confusions. For e.g.,

    type Test = {
        a: string;
        b: {
      	  c: Test;
        };
    };
    
    type P1 = Paths<Test, {maxRecursionDepth: 1}>;
    //   ^? type P1 = "a" | "b" | "b.c"
    
    type P2 = Paths<Test, {maxCircularDepth: 1}>;
    //   ^? type P2 = "a" | "b" | "b.c" | "b.c.a" | "b.c.b" | "b.c.b.c"

@LaurynasGr
Copy link
Contributor Author

@LaurynasGr Types like the following shouldn't be affected by maxCircularDepth, but they currently do.

type Test = {
	foo: string;
	bar: {
		foo: string;
		bar: {};
	};
};

type P = Paths<Test, {maxCircularDepth: 0}>;
//   ^? type P = 'foo' | 'bar'

@som-sm sorted this one - was a simple a extends b not the same as b extends a issue (see 54691f1). Added tests to cover this as well.

Will respond to the other comment tomorrow with a real-world example where I need this specific option to stop VS Code from crashing without limiting myself to a very shallow depth with the general recursion depth option.

@som-sm
Copy link
Collaborator

som-sm commented Mar 5, 2025

sorted this one - was a simple a extends b not the same as b extends a issue

@LaurynasGr Great, I'll try to give this a read sometime tomorrow then!

@som-sm
Copy link
Collaborator

som-sm commented Mar 5, 2025

@LaurynasGr Also, since 0 depth currently means root level keys, like:

type Test = {a: Test};
type P = Paths<Test, {maxRecursionDepth: 0}>;
//   ^? type P = "a"

Should depth 0 for maxCircularDepth mean recursing into the circular reference once?

// Current behaviour
type Test = {a: Test};
type P = Paths<Test, {maxCircularDepth: 0}>;
//   ^? type P = "a"
// Suggested behaviour
type Test = {a: Test};
type P = Paths<Test, {maxCircularDepth: 0}>;
//   ^? type P = "a" | "a.a"

@LaurynasGr
Copy link
Contributor Author

@LaurynasGr Also, since 0 depth currently means root level keys, like:

@som-sm I think that having maxRecursionDepth: 0 return keys at level 0 makes sense because there's nothing below it you might want (i.e. the step below it is no keys at all)

Whereas maxCircularDepth: 0 essentially turns off recursion into circular references. If we were to change it so that maxCircularDepth: 0 would mean recursing into the circular reference once, if you wanted to fully turn off circular recursion you would have to provide maxCircularDepth: -1 which in my eyes is not a logical thing to provide for it

@LaurynasGr
Copy link
Contributor Author

On second thought, I think this option is just going to be more confusing than helpful.
<.....>

  • The fact that maxRecursionDepth and maxCircularDepth don't target the same depth could lead to confusions. For e.g.,
    type Test = {
        a: string;
        b: {
      	  c: Test;
        };
    };
    
    type P1 = Paths<Test, {maxRecursionDepth: 1}>;
    //   ^? type P1 = "a" | "b" | "b.c"
    
    type P2 = Paths<Test, {maxCircularDepth: 1}>;
    //   ^? type P2 = "a" | "b" | "b.c" | "b.c.a" | "b.c.b" | "b.c.b.c"

If this is related to

@LaurynasGr Also, since 0 depth currently means root level keys, like:
<...>
Should depth 0 for maxCircularDepth mean recursing into the circular reference once?

I answered it here #1079 (comment)

In other words the whole point of this option is to detach it from maxRecursionDepth - they do different things. Below is a real world example, why I taken on to add this option:

I have this state type

type SomeState = {
  a: {
    b: {
      c: {
        d: boolean;
        e: string;
        f: {
          g: number;
          h: boolean;
        };
      };
    };
    e: number;
  };
  connection: Connection;
  something: string;
  oneMoreThing: {
    a: string;
    b: {
      c: string[];
      d: {
        e: string;
        f: {
          g: number;
          h: boolean;
        };
      };
    };
  };
  getSomething: (param?: string) => Promise<Something>;
  // <....>
};

Note that the connection: Connection is referring to this type https://github.com/microsoft/vscode-languageserver-node/blob/main/server/src/common/server.ts#L1359 which has a bunch of properties and many of them recursively refers back to Connection by having it's own connection: Connection reference (e.g. console, tracer, telemetry etc.).

Now if I were to try to get Paths for SomeState

type SomeStateKeys = Paths<SomeState>;

my VS Code (running on Intel i9-13900k with 128GB RAM) will immediately choke
image

and then after thinking for a good while, it will just give up and say "No, sorry mate, this is too much for me"
image

The only solution I have right now is to limit the whole recursion to some arbitrary small number
image
image

But the issue here is that I loose all of the state keys that I myself defined and still get large numbers of keys I do not care about rom the Connection recursion.

Now if I just use maxCircularDepth: 0 I get all of my own keys, all of the non-recursive Connection keys and none of the circular ones I don't care about
image

@som-sm
Copy link
Collaborator

som-sm commented Mar 6, 2025

#1079 (comment)

Makes sense—I was just concerned that it might cause confusion if depth: 0 doesn't mean the same thing in all cases. But anyways, since maxCircularDepth is quite different from maxRecursionDepth and depth, I guess it's fine.


But can you please add a note in the description, similar to depth, clarifying what depth: 0 means.

Note: Depth starts at `0` for root properties.

@som-sm
Copy link
Collaborator

som-sm commented Mar 6, 2025

@LaurynasGr Thanks for the detailed explanation!


Haven't had a chance to properly review this PR yet, but it looks like maxCircularDepth doesn't do anything in this case:

type Test = [...Test[], string];

type P = Paths<Test, {maxCircularDepth: 0}>;
image

@LaurynasGr
Copy link
Contributor Author

But can you please add a note in the description, similar to depth, clarifying what depth: 0 means.

@som-sm Sure - added the note here b025220. The first example also clearly shows this, so we should be OK.
Screenshot 2025-03-06 at 16 35 29

@LaurynasGr
Copy link
Contributor Author

Haven't had a chance to properly review this PR yet, but it looks like maxCircularDepth doesn't do anything in this case:

type Test = [...Test[], string];

type P = Paths<Test, {maxCircularDepth: 0}>;

@som-sm oh right.. forgot to cover variable length arrays - sorry about that. Fixed it here 9f2f47d

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 this pull request may close these issues.

2 participants