-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Reuse checker's property accessibility check for completions #45388
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
Changes from 5 commits
89c4e8c
85e1f26
05c993b
6136c7c
8dd1cf6
2546be8
4ffa810
053c00b
7c3d29c
996bce5
7ca48dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -263,6 +263,30 @@ namespace ts { | |||||||
Uncapitalize: IntrinsicTypeKind.Uncapitalize | ||||||||
})); | ||||||||
|
||||||||
enum TypeChecks { | ||||||||
Ok, | ||||||||
HasDiagnostic | ||||||||
} | ||||||||
|
||||||||
interface CheckerResultOk { | ||||||||
result: TypeChecks.Ok | ||||||||
} | ||||||||
|
||||||||
interface CheckerResultHasDiagnostic { | ||||||||
result: TypeChecks.HasDiagnostic, | ||||||||
message: DiagnosticMessage; | ||||||||
args: | ||||||||
| never[] | ||||||||
gabritto marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| [string | number] | ||||||||
| [string | number, string | number] | ||||||||
| [string | number, string | number, string | number] | ||||||||
| [string | number, string | number, string | number, string | number]; | ||||||||
} | ||||||||
|
||||||||
type CheckerResult = | ||||||||
| CheckerResultOk | ||||||||
| CheckerResultHasDiagnostic; | ||||||||
|
||||||||
function SymbolLinks(this: SymbolLinks) { | ||||||||
} | ||||||||
|
||||||||
|
@@ -712,6 +736,11 @@ namespace ts { | |||||||
|
||||||||
getLocalTypeParametersOfClassOrInterfaceOrTypeAlias, | ||||||||
isDeclarationVisible, | ||||||||
isPropertyAccessible: (nodeIn, isSuper, writing, type, prop) => { | ||||||||
const node = getParseTreeNode(nodeIn); | ||||||||
return node ? | ||||||||
checkPropertyAccessibilityAtNode(node, isSuper, writing, type, prop).result === TypeChecks.Ok : false; | ||||||||
} | ||||||||
}; | ||||||||
|
||||||||
function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined { | ||||||||
|
@@ -27266,11 +27295,36 @@ namespace ts { | |||||||
node: PropertyAccessExpression | QualifiedName | PropertyAccessExpression | VariableDeclaration | ParameterDeclaration | ImportTypeNode | PropertyAssignment | ShorthandPropertyAssignment | BindingElement, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
isSuper: boolean, writing: boolean, type: Type, prop: Symbol, reportError = true): boolean { | ||||||||
|
||||||||
const flags = getDeclarationModifierFlagsFromSymbol(prop, writing); | ||||||||
const errorNode = node.kind === SyntaxKind.QualifiedName ? node.right : | ||||||||
node.kind === SyntaxKind.ImportType ? node : | ||||||||
node.kind === SyntaxKind.BindingElement && node.propertyName ? node.propertyName : node.name; | ||||||||
|
||||||||
const typeChecks = checkPropertyAccessibilityAtNode(node, isSuper, writing, type, prop); | ||||||||
switch (typeChecks.result) { | ||||||||
case (TypeChecks.Ok): | ||||||||
return true; | ||||||||
case (TypeChecks.HasDiagnostic): | ||||||||
if (reportError) { | ||||||||
error(errorNode, typeChecks.message, ...typeChecks.args); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: I've seen |
||||||||
} | ||||||||
return false; | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
// Possible concerns: | ||||||||
gabritto marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
// (1) I'm not sure if this function has the right behavior if `node` is allowed to be any node, | ||||||||
gabritto marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
// although we only use `node` for its location in the parse tree. | ||||||||
// (2) Does it even make sense to check for property accessibility at a certain arbitrary node? | ||||||||
// Maybe this function should be called "checkPropertyVisibilityAtNode" or something else. | ||||||||
function checkPropertyAccessibilityAtNode(node: Node, | ||||||||
isSuper: boolean, writing: boolean, | ||||||||
type: Type, prop: Symbol): CheckerResult { | ||||||||
const flags = getDeclarationModifierFlagsFromSymbol(prop, writing); | ||||||||
|
||||||||
const checkerOk: CheckerResultOk = { | ||||||||
result: TypeChecks.Ok | ||||||||
}; | ||||||||
|
||||||||
if (isSuper) { | ||||||||
// TS 1.0 spec (April 2014): 4.8.2 | ||||||||
// - In a constructor, instance member function, instance member accessor, or | ||||||||
|
@@ -27281,21 +27335,23 @@ namespace ts { | |||||||
// a super property access is permitted and must specify a public static member function of the base class. | ||||||||
if (languageVersion < ScriptTarget.ES2015) { | ||||||||
if (symbolHasNonMethodDeclaration(prop)) { | ||||||||
if (reportError) { | ||||||||
error(errorNode, Diagnostics.Only_public_and_protected_methods_of_the_base_class_are_accessible_via_the_super_keyword); | ||||||||
} | ||||||||
return false; | ||||||||
return { | ||||||||
result: TypeChecks.HasDiagnostic, | ||||||||
message: Diagnostics.Only_public_and_protected_methods_of_the_base_class_are_accessible_via_the_super_keyword, | ||||||||
args: emptyArray | ||||||||
}; | ||||||||
} | ||||||||
} | ||||||||
if (flags & ModifierFlags.Abstract) { | ||||||||
// A method cannot be accessed in a super property access if the method is abstract. | ||||||||
// This error could mask a private property access error. But, a member | ||||||||
// cannot simultaneously be private and abstract, so this will trigger an | ||||||||
// additional error elsewhere. | ||||||||
if (reportError) { | ||||||||
error(errorNode, Diagnostics.Abstract_method_0_in_class_1_cannot_be_accessed_via_super_expression, symbolToString(prop), typeToString(getDeclaringClass(prop)!)); | ||||||||
} | ||||||||
return false; | ||||||||
return { | ||||||||
result: TypeChecks.HasDiagnostic, | ||||||||
message: Diagnostics.Abstract_method_0_in_class_1_cannot_be_accessed_via_super_expression, | ||||||||
args: [symbolToString(prop), typeToString(getDeclaringClass(prop)!)] | ||||||||
}; | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
|
@@ -27304,16 +27360,17 @@ namespace ts { | |||||||
(isThisProperty(node) || isThisInitializedObjectBindingExpression(node) || isObjectBindingPattern(node.parent) && isThisInitializedDeclaration(node.parent.parent))) { | ||||||||
const declaringClassDeclaration = getClassLikeDeclarationOfSymbol(getParentOfSymbol(prop)!); | ||||||||
if (declaringClassDeclaration && isNodeUsedDuringClassInitialization(node)) { | ||||||||
if (reportError) { | ||||||||
error(errorNode, Diagnostics.Abstract_property_0_in_class_1_cannot_be_accessed_in_the_constructor, symbolToString(prop), getTextOfIdentifierOrLiteral(declaringClassDeclaration.name!)); // TODO: GH#18217 | ||||||||
} | ||||||||
return false; | ||||||||
return { | ||||||||
result: TypeChecks.HasDiagnostic, | ||||||||
message: Diagnostics.Abstract_property_0_in_class_1_cannot_be_accessed_in_the_constructor, | ||||||||
args: [symbolToString(prop), getTextOfIdentifierOrLiteral(declaringClassDeclaration.name!)] | ||||||||
}; | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
// Public properties are otherwise accessible. | ||||||||
if (!(flags & ModifierFlags.NonPublicAccessibilityModifier)) { | ||||||||
return true; | ||||||||
return checkerOk; | ||||||||
} | ||||||||
|
||||||||
// Property is known to be private or protected at this point | ||||||||
|
@@ -27322,19 +27379,20 @@ namespace ts { | |||||||
if (flags & ModifierFlags.Private) { | ||||||||
const declaringClassDeclaration = getClassLikeDeclarationOfSymbol(getParentOfSymbol(prop)!)!; | ||||||||
if (!isNodeWithinClass(node, declaringClassDeclaration)) { | ||||||||
if (reportError) { | ||||||||
error(errorNode, Diagnostics.Property_0_is_private_and_only_accessible_within_class_1, symbolToString(prop), typeToString(getDeclaringClass(prop)!)); | ||||||||
} | ||||||||
return false; | ||||||||
return { | ||||||||
result: TypeChecks.HasDiagnostic, | ||||||||
message: Diagnostics.Property_0_is_private_and_only_accessible_within_class_1, | ||||||||
args: [symbolToString(prop), typeToString(getDeclaringClass(prop)!)] | ||||||||
}; | ||||||||
} | ||||||||
return true; | ||||||||
return checkerOk; | ||||||||
} | ||||||||
|
||||||||
// Property is known to be protected at this point | ||||||||
|
||||||||
// All protected properties of a supertype are accessible in a super access | ||||||||
if (isSuper) { | ||||||||
return true; | ||||||||
return checkerOk; | ||||||||
} | ||||||||
|
||||||||
// Find the first enclosing class that has the declaring classes of the protected constituents | ||||||||
|
@@ -27349,30 +27407,32 @@ namespace ts { | |||||||
// static member access is disallow | ||||||||
let thisParameter: ParameterDeclaration | undefined; | ||||||||
if (flags & ModifierFlags.Static || !(thisParameter = getThisParameterFromNodeContext(node)) || !thisParameter.type) { | ||||||||
if (reportError) { | ||||||||
error(errorNode, Diagnostics.Property_0_is_protected_and_only_accessible_within_class_1_and_its_subclasses, symbolToString(prop), typeToString(getDeclaringClass(prop) || type)); | ||||||||
} | ||||||||
return false; | ||||||||
return { | ||||||||
result: TypeChecks.HasDiagnostic, | ||||||||
message: Diagnostics.Property_0_is_protected_and_only_accessible_within_class_1_and_its_subclasses, | ||||||||
args: [symbolToString(prop), typeToString(getDeclaringClass(prop) || type)] | ||||||||
}; | ||||||||
} | ||||||||
|
||||||||
const thisType = getTypeFromTypeNode(thisParameter.type); | ||||||||
enclosingClass = (((thisType.flags & TypeFlags.TypeParameter) ? getConstraintOfTypeParameter(thisType as TypeParameter) : thisType) as TypeReference).target; | ||||||||
} | ||||||||
// No further restrictions for static properties | ||||||||
if (flags & ModifierFlags.Static) { | ||||||||
return true; | ||||||||
return checkerOk; | ||||||||
} | ||||||||
if (type.flags & TypeFlags.TypeParameter) { | ||||||||
// get the original type -- represented as the type constraint of the 'this' type | ||||||||
type = (type as TypeParameter).isThisType ? getConstraintOfTypeParameter(type as TypeParameter)! : getBaseConstraintOfType(type as TypeParameter)!; // TODO: GH#18217 Use a different variable that's allowed to be undefined | ||||||||
} | ||||||||
if (!type || !hasBaseType(type, enclosingClass)) { | ||||||||
if (reportError) { | ||||||||
error(errorNode, Diagnostics.Property_0_is_protected_and_only_accessible_through_an_instance_of_class_1_This_is_an_instance_of_class_2, symbolToString(prop), typeToString(enclosingClass), typeToString(type)); | ||||||||
} | ||||||||
return false; | ||||||||
return { | ||||||||
result: TypeChecks.HasDiagnostic, | ||||||||
message: Diagnostics.Property_0_is_protected_and_only_accessible_through_an_instance_of_class_1_This_is_an_instance_of_class_2, | ||||||||
args: [symbolToString(prop), typeToString(enclosingClass), typeToString(type)] | ||||||||
}; | ||||||||
} | ||||||||
return true; | ||||||||
return checkerOk; | ||||||||
} | ||||||||
|
||||||||
function getThisParameterFromNodeContext(node: Node) { | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing this should be internal, unless we add some code here that determines what value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would definitely start this as internal. When are I had another idea in a comment in completions.ts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided this function shouldn't (and actually can't) calculate |
||
} | ||
|
||
/* @internal */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/// <reference path="fourslash.ts" /> | ||
|
||
////class Client { | ||
//// private close() { } | ||
//// public open() { } | ||
////} | ||
////type Wrap<T> = T & | ||
////{ | ||
//// [K in Extract<keyof T, string> as `${K}Wrapped`]: T[K]; | ||
////}; | ||
////class Service { | ||
//// method() { | ||
//// let service = undefined as unknown as Wrap<Client>; | ||
//// const { /*a*/ } = service; | ||
//// } | ||
////} | ||
|
||
verify.completions({ marker: "a", exact: ["open", "openWrapped"] }); |
Uh oh!
There was an error while loading. Please reload this page.