Reuse checker's property accessibility check for completions #45388

gabritto · 2021-08-09T23:59:32Z

This is a very rough draft PR attempting to use the checker's logic for checking property accessibility when filtering visible/accessible properties that should be offered for completion.

This PR was inspired by bug #45045, caused by trying to access a type's undefined symbol when checking if we should offer a property for completions in an object binding pattern (in tryGetObjectLikeCompletions from completions).
Looking at the code in completions, I thought the type checker must already do this sort of property accessibility check and set out to see if we could reuse the checker's code for this purpose.

The checker function that does this is checkPropertyAccessibility.
This function returns a boolean saying if a property access is valid, given a property access-like node, the property's symbol, the type where the property comes from, and some options (writing if it's a write access, isSuper if we're accessing a super property e.g. super.foo).
But, it also reports an invalid property error if the reportError flag is set to true.

Based on checkPropertyAccessibility, I created the function checkPropertyAccessibilityAtNode that has the same implementation, but with two differences:

checkPropertyAccessibility takes as argument nodes that are property access-like only. This is enough for type checking purposes, and also if we are in a completions scenario where we have an actual property access (e.g. foo.|), because then we have such a property access-like node. But, in the case of offering completions for an object binding pattern, we are trying to figure out if a property is accessible when we don't yet have a property access-like (i.e. a binding element) node. To that end, the new function checkPropertyAccessibilityAtNode accepts any Node, indicating the location in the AST where we'd want to access the property.
unlike checkPropertyAccessibility, checkPropertyAccessibilityAtNode does not report errors, but instead, if the property access is invalid, it returns diagnostics info (diagnostic message + arguments).

I changed checkPropertyAccessibility to call the new checkPropertyAccessibilityAtNode and report an error (i.e. call error(...)) depending on the returned value.
I also added a boolean-returning wrapper over the checkPropertyAccessibilityAtNode function (isPropertyAccessible), exposed by the checker and used by completions.

I only replaced the existing property accessibility completions-specific code to use the new isPropertyAccessible function in tryGetObjectLikeCompletions, but there might be other places that could use this new checker function.

The variable/function/type names and overall code structure is in no way ideal, but I wanted to know if the idea makes any sense before investing more time on making things pretty. I'm also very open to suggestions on alternative ways to achieve the same thing, or just explanations on why that sort of reuse shouldn't be done.

This reverts commit 8dd1cf6.

src/compiler/checker.ts

gabritto · 2021-08-10T00:10:18Z

src/compiler/types.ts

@@ -4354,6 +4354,7 @@ namespace ts {

        /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined;
        /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean;
+        /* @internal */ isPropertyAccessible(node: Node, isSuper: boolean, writing: boolean, type: Type, prop: Symbol): boolean;


I'm guessing this should be internal, unless we add some code here that determines what value isSuper and writing should be, and possibly get the type or the property symbol ourselves instead of as arguments to avoid incorrect usages of the function.

I would definitely start this as internal. When are isSuper and writing true? I think calculating them for the caller is a good idea.

I had another idea in a comment in completions.ts

Decided this function shouldn't (and actually can't) calculate isSuper and isWrite. This function does not assume we have an existing property access-y node, which we would need to calculate those arguments, so the caller should specify what are the appropriate arguments.

sandersn

I think making completions use the same algorithm as the checker is a good idea.

src/compiler/checker.ts

src/services/completions.ts

sandersn · 2021-08-10T23:52:46Z

src/compiler/types.ts

@@ -4354,6 +4354,7 @@ namespace ts {

        /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined;
        /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean;
+        /* @internal */ isPropertyAccessible(node: Node, isSuper: boolean, writing: boolean, type: Type, prop: Symbol): boolean;


I would definitely start this as internal. When are isSuper and writing true? I think calculating them for the caller is a good idea.

I had another idea in a comment in completions.ts

src/compiler/checker.ts

DanielRosenwasser · 2021-08-11T00:10:02Z

I agree with Nathan - this functionality shouldn't move too much around in the checker, and should instead keep consistent with the existing styles.

gabritto · 2021-08-16T22:20:47Z

src/compiler/checker.ts

            }
-            // In js files properties of unions are allowed in completion


Deleted this special casing of JS files because it's already dealt with in completions by addTypeProperties.

gabritto · 2021-08-16T22:24:17Z

src/compiler/types.ts

@@ -4216,7 +4216,7 @@ namespace ts {

        getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): string | number | undefined;
        isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName | ImportTypeNode, propertyName: string): boolean;
-        /** Exclude accesses to private properties or methods with a `this` parameter that `type` doesn't satisfy. */


Updating comment since the implementation of this function no longer verifies this type for performance reasons (see #31377).

gabritto · 2021-08-16T22:30:10Z

src/compiler/checker.ts

@@ -28128,19 +28175,45 @@ namespace ts {
            propertyName: __String,
            type: Type): boolean {

+            // Short-circuiting for improved performance.


Copied this from the existing isValidPropertyAccessWithType function.
It seemed to be there for performance reasons, doesn't look like it would change behavior, so I added it assuming someone had a good reason to write it in the first place.

gabritto · 2021-08-16T22:41:01Z

On the second pass here, I reviewed the checker functions used by completions related to checking property accessibility.
The existing functions, isValidPropertyAccess (public), isValidPropertyAccessForCompletions (marked as internal), and isValidPropertyAccessWithType (helper called by isValidPropertyAccess), are all called by completions when we have something like foo.|, to try to validate an existing property access-like node (e.g. in foo.|, we'd have a PropertyAccess node with empty string as property name). This differs from isPropertyAccessible (newly added), in that isPropertyAccessible does not assume we already have a property access-like node, and uses the node exclusively as a location.
The existing isValidPropertyAccess... functions also have slightly different parameter (e.g. one takes in a property symbol and the other a property name), so I decided to just change their implementations to call the new isPropertyAccessible function where possible. Further refactoring would probably require also refactoring completions code.

typescript-bot · 2021-08-16T22:42:58Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

sandersn · 2021-08-19T00:17:48Z

src/compiler/checker.ts

+         * @param location The location node where we want to check if the property is accessible.
+         * @param isSuper True if the access is from `super.`.
+         * @param writing True if this is a write property access, false if it is a read property access.
+         * @param type The type of the object whose property is being accessed. (Not the type of the property.)


minor: I've seen containingType as a pretty good name for this.

sandersn · 2021-08-19T00:19:42Z

src/compiler/checker.ts

@@ -27291,11 +27292,29 @@ namespace ts {
            node: PropertyAccessExpression | QualifiedName | PropertyAccessExpression | VariableDeclaration | ParameterDeclaration | ImportTypeNode | PropertyAssignment | ShorthandPropertyAssignment | BindingElement,
            isSuper: boolean, writing: boolean, type: Type, prop: Symbol, reportError = true): boolean {

-            const flags = getDeclarationModifierFlagsFromSymbol(prop, writing);
-            const errorNode = node.kind === SyntaxKind.QualifiedName ? node.right :
+            const errorNode = !reportError ? undefined : node.kind === SyntaxKind.QualifiedName ? node.right :


Suggested change

const errorNode = !reportError ? undefined : node.kind === SyntaxKind.QualifiedName ? node.right :

const errorNode = !reportError ? undefined :

node.kind === SyntaxKind.QualifiedName ? node.right :

…ft#45388) * add test for completions crash * WIP: experiment * remove comments * add call to getParseTreeNode * Revert "add call to getParseTreeNode" This reverts commit 8dd1cf6. * Fix comments * minor fixes * fix formatting * rename type to containingType

gabritto added 4 commits August 5, 2021 08:36

add test for completions crash

89c4e8c

WIP: experiment

85e1f26

Merge branch 'main' into gabritto/issue45045

05c993b

remove comments

6136c7c

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Aug 9, 2021

gabritto added 2 commits August 9, 2021 17:03

add call to getParseTreeNode

8dd1cf6

Revert "add call to getParseTreeNode"

2546be8

This reverts commit 8dd1cf6.

gabritto commented Aug 10, 2021

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

gabritto commented Aug 10, 2021

View reviewed changes

sandersn reviewed Aug 10, 2021

View reviewed changes

Fix comments

4ffa810

gabritto force-pushed the gabritto/issue45045 branch from 7631247 to 4ffa810 Compare August 16, 2021 22:15

Merge branch 'main' into gabritto/issue45045

053c00b

gabritto commented Aug 16, 2021

View reviewed changes

minor fixes

7c3d29c

gabritto commented Aug 16, 2021

View reviewed changes

gabritto marked this pull request as ready for review August 16, 2021 22:42

gabritto linked an issue Aug 16, 2021 that may be closed by this pull request

class method destructuring assignment autocomplete not work #45045

Closed

gabritto requested a review from sandersn August 17, 2021 16:52

sandersn approved these changes Aug 19, 2021

View reviewed changes

fix formatting

996bce5

gabritto changed the title ~~Draft: reuse checker's property accessibility check for completions~~ Reuse checker's property accessibility check for completions Aug 19, 2021

rename type to containingType

7ca48dc

gabritto merged commit 945179f into main Aug 19, 2021

gabritto deleted the gabritto/issue45045 branch August 19, 2021 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse checker's property accessibility check for completions #45388

Reuse checker's property accessibility check for completions #45388

gabritto commented Aug 9, 2021

gabritto Aug 10, 2021

sandersn Aug 10, 2021

gabritto Aug 16, 2021

sandersn left a comment

sandersn Aug 10, 2021

DanielRosenwasser commented Aug 11, 2021

gabritto Aug 16, 2021

gabritto Aug 16, 2021

gabritto Aug 16, 2021

gabritto commented Aug 16, 2021

typescript-bot commented Aug 16, 2021

sandersn Aug 19, 2021

sandersn Aug 19, 2021

		}
		// In js files properties of unions are allowed in completion

	const errorNode = !reportError ? undefined : node.kind === SyntaxKind.QualifiedName ? node.right :
	const errorNode = !reportError ? undefined :
	node.kind === SyntaxKind.QualifiedName ? node.right :

Reuse checker's property accessibility check for completions #45388

Reuse checker's property accessibility check for completions #45388

Conversation

gabritto commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabritto commented Aug 16, 2021

typescript-bot commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment