Skip to content

Mark inaccessible codefix parameter types as any #53327

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Mark inaccessible codefix parameter types as any #53327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
decbceb
Check symbol accessibility when generating snippet
Mar 13, 2023
56768f5
Update baseline
Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/services/codefixes/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
ArrowFunction,
Block,
CallExpression,
canHaveSymbol,
CharacterCodes,
ClassLikeDeclaration,
CodeFixContextBase,
Expand Down Expand Up @@ -38,8 +39,10 @@ import {
IntersectionType,
isArrowFunction,
isAutoAccessorPropertyDeclaration,
isConstructorTypeNode,
isFunctionDeclaration,
isFunctionExpression,
isFunctionTypeNode,
isGetAccessorDeclaration,
isIdentifier,
isImportTypeNode,
Expand All @@ -64,6 +67,7 @@ import {
NodeArray,
NodeBuilderFlags,
NodeFlags,
nodeIsSynthesized,
nullTransformationContext,
ObjectFlags,
ObjectLiteralExpression,
Expand All @@ -84,6 +88,7 @@ import {
some,
SourceFile,
Symbol,
SymbolAccessibility,
SymbolFlags,
SymbolTracker,
SyntaxKind,
Expand Down Expand Up @@ -363,6 +368,7 @@ export function createSignatureDeclarationFromSignature(
) {
const program = context.program;
const checker = program.getTypeChecker();
const resolver = checker.getEmitResolver();
const scriptTarget = getEmitScriptTarget(program.getCompilerOptions());
const isJs = isInJSFile(enclosingDeclaration);
const flags =
Expand Down Expand Up @@ -417,6 +423,9 @@ export function createSignatureDeclarationFromSignature(
type = importableReference.typeNode;
importSymbols(importAdder, importableReference.symbols);
}
else if (!isDeclTypeSymbolVisible(parameterDecl.type)) {
type = factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
}
Comment on lines +426 to +428
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MariaSolOs MariaSolOs Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but wait, I do need to pass NoTruncation:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MariaSolOs MariaSolOs Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@weswigham weswigham Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return factory.updateParameterDeclaration(
parameterDecl,
Expand Down Expand Up @@ -455,6 +464,23 @@ export function createSignatureDeclarationFromSignature(
return factory.updateFunctionDeclaration(signatureDeclaration, modifiers, signatureDeclaration.asteriskToken, tryCast(name, isIdentifier), typeParameters, parameters, type, body ?? signatureDeclaration.body);
}
return undefined;

function isDeclTypeSymbolVisible(declType: TypeNode | undefined) {
if (!declType || !canHaveSymbol(declType)) {
return true;
}

if (!declType.symbol) {
// These nodes won't have a symbol when generated via a code fix, so we treat them as an exception.
if (isFunctionTypeNode(declType) || isConstructorTypeNode(declType)) {
return nodeIsSynthesized(declType);
}
return false;
}

const access = resolver.isSymbolAccessible(declType.symbol, declType, declType.symbol.flags, /* shouldComputeAliasToMarkVisible */ true);
return access.accessibility === SymbolAccessibility.Accessible;
}
}

/** @internal */
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/completionsClassMembers4.baseline
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@
"kind": "method",
"kindModifiers": "",
"sortText": "17",
"insertText": "pipe(): Observable<any>;\npipe<A>(): Observable<A>;\npipe<A, B>(): Observable<B>;\npipe<A, B, C>(): Observable<C>;\npipe<A, B, C, D>(): Observable<D>;\npipe<A, B, C, D, E>(): Observable<E>;\npipe<A, B, C, D, E, F>(): Observable<F>;\npipe<A, B, C, D, E, F, G>(): Observable<G>;\npipe<A, B, C, D, E, F, G, H>(): Observable<H>;\npipe<A, B, C, D, E, F, G, H, I>(): Observable<I>;\npipe<A, B, C, D, E, F, G, H, I>(): Observable<unknown>;\npipe(): Observable<any> | Observable<A> | Observable<B> | Observable<C> | Observable<D> | Observable<E> | Observable<F> | Observable<G> | Observable<H> | Observable<I> | Observable<unknown> {\n $0\n}",
"insertText": "pipe(): Observable<any>;\npipe<A>(): Observable<A>;\npipe<A, B>(): Observable<B>;\npipe<A, B, C>(): Observable<C>;\npipe<A, B, C, D>(): Observable<D>;\npipe<A, B, C, D, E>(): Observable<E>;\npipe<A, B, C, D, E, F>(): Observable<F>;\npipe<A, B, C, D, E, F, G>(): Observable<G>;\npipe<A, B, C, D, E, F, G, H>(): Observable<H>;\npipe<A, B, C, D, E, F, G, H, I>(): Observable<I>;\npipe<A, B, C, D, E, F, G, H, I>(): Observable<unknown>;\npipe(): Observable<any> | Observable<unknown> | Observable<A> | Observable<B> | Observable<C> | Observable<D> | Observable<E> | Observable<F> | Observable<G> | Observable<H> | Observable<I> {\n $0\n}",
"isSnippet": true,
"displayParts": [
{
Expand Down