Mark inaccessible codefix parameter types as any

[MariaSolOs]

Fixes #53159 (comment)

[DanielRosenwasser]

You'll need to add a fourslash test case that fails on the main branch, but is fixed by this PR.

[DanielRosenwasser]

This is going to be too limited - it special-cases the parameter at the top level, but not other instances:

• deeper in the parameter type as a type argument

  foo(x: SomeType<TheInaccessibleType>): any

• deeper in the parameter type as the type of a property

  foo(x: { x: TheInaccessibleType }): any

• in the return type

  foo(): TheInaccessibleType

Here's my thought on the current change: you can fix the specific instance of the repro this way, but it's really a band-aid. I don't know how feasible it is, but I think a better fix would be one where we can somehow detect truncation when generating strings from type nodes.

I think others on the team will be able to give better pointers here.

[weswigham]

The symbol tracker actually has a reportTruncationError that is called so long as NoTruncation isn't passed. :P

You could stop passing NoTruncation, and if reportTruncationError on the SymbolTracker you pass in gets called, replace the whole generated type node with any.

[MariaSolOs]

@DanielRosenwasser you're right, this does feel like a hack to handle this particular scenario. I'll continue working on it.

About the fourslash test, I actually wrote one but then I wasn't sure of how to test the completion result, since the given completion doesn't have an attached code action (and I only found ways to verify completion code actions) and the snippet property in the initial completionInfo was undefined (?). I might have missed something though.

[MariaSolOs]

@weswigham well that was exactly the clue I needed :)

[MariaSolOs]

Oh but wait, I do need to pass NoTruncation:

if (context.truncating && context.flags & NodeBuilderFlags.NoTruncation) {
    context.tracker.reportTruncationError();
}

[andrewbranch]

If the type is too big for an error message, it’s also too big to insert as a parameter type in a completion snippet. We shouldn’t be inserting tens of kilobytes of type printout that’s going to hang the user’s editor while they struggle to delete it.

[DanielRosenwasser]

I partially agree, but what about the affected test case given at #52787 ?

Here, truncation occurred because of a slightly long union, even though the produced type is nonsensical. It would definitely feel off to produce a plain unknown or any.

[MariaSolOs]

Okay so the thing with #53159 (comment) is that it includes many bugs and there are several ways in which I can go around fixing it.

The crash is caused when formatting the type because some node ends up having a range that's outside its parent. Note that the input to the formatter involves a recursive type that uses a non-accessible alias, and is a synthesized node generated by the checker to produce the signature and so some nodes don't have their range/parent/text set yet. So far I haven't been able to reproduce the crash without using the replay script, since this isn't just vanilla formatting (the node must be in this synthesized state).

I can try fixing the crash, but then the generated type would be using non-accessible symbols. I could have a symbol tracker that keeps track of non-accessible nodes (and do something like the declaration transformer does), but then mapping the non-accessible symbol to the parameter/return type gets messy. Or I could use this helper method with each type in the signature declaration that the import adder failed to import, but it feels like a lot of work (the checker would generate the signature and then regenerate the type node just for this check) for what feels like a relatively rare scenario.

The last problem is the length of the type. Note @DanielRosenwasser that the type doesn't actually reach the truncation limit. The checker does generate a complete snippet, and so @weswigham's idea of having a symbol tracker that listens for truncation errors is not applicable here if I pass the NoTruncation flag. That being said, I agree with @andrewbranch: even if we format the type correctly, the generated type is so huge that it actually makes VS Code crash, and taking into account their long line limit, I feel like bailing when the snippet gets too large feels like the right approach.

And so, how should I proceed?

1. Fix the formatting crash (which is extremely hard to reproduce).
2. Keep track of non-accessible symbols in the signature.
3. Use a truncation limit when constructing the signature declaration.

Because of the reasons above, I would like to proceed with 3. However, I'm not exactly sure of what the generated signature should be then. Do I just omit the type then (it feels kinda wrong to replace the super long type by any since those are obviously different)? Do I just not offer that completion?

[DanielRosenwasser]

I won't have the chance in the next 2 days to figure out what the issue with (1) is - it sounds like you're saying it's not that the string is getting truncated, but rather, that the generated type nodes are getting reused in multiple parts of the tree, which the formatter really doesn't like.

My gut feel is that you want a combo of (2) and (3) - (2) is the golden path, and in some cases you want to really apply a truncation limit when the signature would be really bad. But I don't know how to thread that through.

[weswigham]

I won't have the chance in the next 2 days to figure out what the issue with (1) is - it sounds like you're saying it's not that the string is getting truncated, but rather, that the generated type nodes are getting reused in multiple parts of the tree, which the formatter really doesn't like.

We looked at the situation of the crash under the debugger a bit as a group yesterday - what we saw in the debugger is that there's an off-by-one error somewhere in the formatters' synthetic node position assignment logic. A } that was supposed to be the child of a node was positioned one character past the end of the node, so either the } had a slightly off position, or the node length for the parent was measured as one too short. I'm not too familiar with the position assignment logic the formatter uses for synthetic nodes, so I couldn't say which is more likely.

[MariaSolOs]

I don't want this to go stale and I'm not too sure of what the desired behavior is. @DanielRosenwasser could you please explain what the change should be?

[MariaSolOs]

Adding this as extra context:

• The crash can be reproduced when the snippet is printed using the emitter. This is because the nodes are assigned positions, which becomes problematic with recursive nodes.

The type that's being formatted when the crash happens is:

type _QueryDeepPartialEntity<T> = {
  [P in keyof T]?:
    |(T[P] extends Array<infer U>
      ? Array<_QueryDeepPartialEntity<U>>
      : T[P] extends ReadonlyArray<infer U>
      ? ReadonlyArray<_QueryDeepPartialEntity<U>>
      : _QueryDeepPartialEntity<T[P]>)
    | (() => string)
}

and the sequence of formatting actions looks something like: "format mapped type" -> "format union type" -> "format conditional type" -> "format array type" -> "format mapped type" -> crash, which makes sense because the array type recurses.

[sandersn]

@weswigham this seems like something that should be fixed, but it won't be relevant to Corsa until the codefixes that depend on this code are ported. Do you think it's worth keeping open until then? (I'm trying to reduce the number of open PRs.)