Skip to content

Readonly support for jsdoc #35790

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

Merged
merged 4 commits into from
Dec 20, 2019
Merged

Readonly support for jsdoc #35790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7808c4
Add @readonly
sandersn Dec 18, 2019
8974fb5
In-progress
sandersn Dec 19, 2019
55b48d4
Done except for cleanup and fix 1 bug
sandersn Dec 20, 2019
a1d3a3e
Fix last test and clean up
sandersn Dec 20, 2019
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
42 changes: 25 additions & 17 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11964,7 +11964,7 @@ namespace ts {
if (prop) {
if (accessExpression) {
markPropertyAsReferenced(prop, accessExpression, /*isThisAccess*/ accessExpression.expression.kind === SyntaxKind.ThisKeyword);
if (isAssignmentTarget(accessExpression) && (isReferenceToReadonlyEntity(accessExpression, prop) || isReferenceThroughNamespaceImport(accessExpression))) {
if (isAssignmentToReadonlyEntity(accessExpression, prop, getAssignmentTargetKind(accessExpression))) {
error(accessExpression.argumentExpression, Diagnostics.Cannot_assign_to_0_because_it_is_a_read_only_property, symbolToString(prop));
return undefined;
}
Expand Down Expand Up @@ -23046,11 +23046,9 @@ namespace ts {
markPropertyAsReferenced(prop, node, left.kind === SyntaxKind.ThisKeyword);
getNodeLinks(node).resolvedSymbol = prop;
checkPropertyAccessibility(node, left.kind === SyntaxKind.SuperKeyword, apparentType, prop);
if (assignmentKind) {
if (isReferenceToReadonlyEntity(<Expression>node, prop) || isReferenceThroughNamespaceImport(<Expression>node)) {
error(right, Diagnostics.Cannot_assign_to_0_because_it_is_a_read_only_property, idText(right));
return errorType;
}
if (isAssignmentToReadonlyEntity(node as Expression, prop, assignmentKind)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I collapsed these three checks into one function because they are always called together.

error(right, Diagnostics.Cannot_assign_to_0_because_it_is_a_read_only_property, idText(right));
return errorType;
}
propType = getConstraintForLocation(getTypeOfSymbol(prop), node);
}
Expand Down Expand Up @@ -26332,29 +26330,39 @@ namespace ts {
);
}

function isReferenceToReadonlyEntity(expr: Expression, symbol: Symbol): boolean {
function isAssignmentToReadonlyEntity(expr: Expression, symbol: Symbol, assignmentKind: AssignmentKind) {
if (assignmentKind === AssignmentKind.None) {
// no assigment means it doesn't matter whether the entity is readonly
return false;
}
if (isReadonlySymbol(symbol)) {
// Allow assignments to readonly properties within constructors of the same class declaration.
if (symbol.flags & SymbolFlags.Property &&
(expr.kind === SyntaxKind.PropertyAccessExpression || expr.kind === SyntaxKind.ElementAccessExpression) &&
(expr as AccessExpression).expression.kind === SyntaxKind.ThisKeyword) {
// Look for if this is the constructor for the class that `symbol` is a property of.
const func = getContainingFunction(expr);
if (!(func && func.kind === SyntaxKind.Constructor)) {
const ctor = getContainingFunction(expr);
if (!(ctor && ctor.kind === SyntaxKind.Constructor)) {
return true;
}
// If func.parent is a class and symbol is a (readonly) property of that class, or
// if func is a constructor and symbol is a (readonly) parameter property declared in it,
// then symbol is writeable here.
return !symbol.valueDeclaration || !(func.parent === symbol.valueDeclaration.parent || func === symbol.valueDeclaration.parent);
if (symbol.valueDeclaration) {
const isAssignmentDeclaration = isBinaryExpression(symbol.valueDeclaration);
const isLocalPropertyDeclaration = ctor.parent === symbol.valueDeclaration.parent;
const isLocalParameterProperty = ctor === symbol.valueDeclaration.parent;
const isLocalThisPropertyAssignment = isAssignmentDeclaration && symbol.parent?.valueDeclaration === ctor.parent;
Copy link
Member

Choose a reason for hiding this comment

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

This would be true inside any method, but we’ve already ensured we’re in a constructor 👍

const isLocalThisPropertyAssignmentConstructorFunction = isAssignmentDeclaration && symbol.parent?.valueDeclaration === ctor;
Copy link
Member

Choose a reason for hiding this comment

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

This one is this.x = whatever inside a JS constructor function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. Any suggestions for making the variable name clearer?

Copy link
Member

Choose a reason for hiding this comment

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

- isLocalThisPropertyAssignmentConstructorFunction
+ isLocalThisPropertyAssignmentInConstructorFunction

?

const isWriteableSymbol =
isLocalPropertyDeclaration
|| isLocalParameterProperty
|| isLocalThisPropertyAssignment
|| isLocalThisPropertyAssignmentConstructorFunction;
return !isWriteableSymbol;
}
}
return true;
}
return false;
}

function isReferenceThroughNamespaceImport(expr: Expression): boolean {
if (expr.kind === SyntaxKind.PropertyAccessExpression || expr.kind === SyntaxKind.ElementAccessExpression) {
// references through namespace import should be readonly
const node = skipParentheses((expr as AccessExpression).expression);
if (node.kind === SyntaxKind.Identifier) {
const symbol = getNodeLinks(node).resolvedSymbol!;
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ namespace ts {
case SyntaxKind.JSDocPublicTag:
case SyntaxKind.JSDocPrivateTag:
case SyntaxKind.JSDocProtectedTag:
case SyntaxKind.JSDocReadonlyTag:
return visitNode(cbNode, (node as JSDocTag).tagName);
case SyntaxKind.PartiallyEmittedExpression:
return visitNode(cbNode, (<PartiallyEmittedExpression>node).expression);
Expand Down Expand Up @@ -6845,6 +6846,9 @@ namespace ts {
case "protected":
tag = parseSimpleTag(start, SyntaxKind.JSDocProtectedTag, tagName);
break;
case "readonly":
tag = parseSimpleTag(start, SyntaxKind.JSDocReadonlyTag, tagName);
break;
case "this":
tag = parseThisTag(start, tagName);
break;
Expand Down
5 changes: 5 additions & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ namespace ts {
JSDocPublicTag,
JSDocPrivateTag,
JSDocProtectedTag,
JSDocReadonlyTag,
JSDocCallbackTag,
JSDocEnumTag,
JSDocParameterTag,
Expand Down Expand Up @@ -2632,6 +2633,10 @@ namespace ts {
kind: SyntaxKind.JSDocProtectedTag;
}

export interface JSDocReadonlyTag extends JSDocTag {
kind: SyntaxKind.JSDocReadonlyTag;
}

export interface JSDocEnumTag extends JSDocTag, Declaration {
parent: JSDoc;
kind: SyntaxKind.JSDocEnumTag;
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4130,7 +4130,8 @@ namespace ts {
// or when !(node.flags & NodeFlags.Synthesized) && node.kind !== SyntaxKind.SourceFile)
const tags = (getJSDocPublicTag(node) ? ModifierFlags.Public : ModifierFlags.None)
| (getJSDocPrivateTag(node) ? ModifierFlags.Private : ModifierFlags.None)
| (getJSDocProtectedTag(node) ? ModifierFlags.Protected : ModifierFlags.None);
| (getJSDocProtectedTag(node) ? ModifierFlags.Protected : ModifierFlags.None)
| (getJSDocReadonlyTag(node) ? ModifierFlags.Readonly : ModifierFlags.None);
flags |= tags;
}

Expand Down
9 changes: 9 additions & 0 deletions src/compiler/utilitiesPublic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,11 @@ namespace ts {
return getFirstJSDocTag(node, isJSDocProtectedTag);
}

/** Gets the JSDoc protected tag for the node if present */
export function getJSDocReadonlyTag(node: Node): JSDocReadonlyTag | undefined {
return getFirstJSDocTag(node, isJSDocReadonlyTag);
}

/** Gets the JSDoc enum tag for the node if present */
export function getJSDocEnumTag(node: Node): JSDocEnumTag | undefined {
return getFirstJSDocTag(node, isJSDocEnumTag);
Expand Down Expand Up @@ -1574,6 +1579,10 @@ namespace ts {
return node.kind === SyntaxKind.JSDocProtectedTag;
}

export function isJSDocReadonlyTag(node: Node): node is JSDocReadonlyTag {
return node.kind === SyntaxKind.JSDocReadonlyTag;
}

export function isJSDocEnumTag(node: Node): node is JSDocEnumTag {
return node.kind === SyntaxKind.JSDocEnumTag;
}
Expand Down
45 changes: 26 additions & 19 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,23 +386,24 @@ declare namespace ts {
JSDocPublicTag = 308,
JSDocPrivateTag = 309,
JSDocProtectedTag = 310,
JSDocCallbackTag = 311,
JSDocEnumTag = 312,
JSDocParameterTag = 313,
JSDocReturnTag = 314,
JSDocThisTag = 315,
JSDocTypeTag = 316,
JSDocTemplateTag = 317,
JSDocTypedefTag = 318,
JSDocPropertyTag = 319,
SyntaxList = 320,
NotEmittedStatement = 321,
PartiallyEmittedExpression = 322,
CommaListExpression = 323,
MergeDeclarationMarker = 324,
EndOfDeclarationMarker = 325,
SyntheticReferenceExpression = 326,
Count = 327,
JSDocReadonlyTag = 311,
JSDocCallbackTag = 312,
JSDocEnumTag = 313,
JSDocParameterTag = 314,
JSDocReturnTag = 315,
JSDocThisTag = 316,
JSDocTypeTag = 317,
JSDocTemplateTag = 318,
JSDocTypedefTag = 319,
JSDocPropertyTag = 320,
SyntaxList = 321,
NotEmittedStatement = 322,
PartiallyEmittedExpression = 323,
CommaListExpression = 324,
MergeDeclarationMarker = 325,
EndOfDeclarationMarker = 326,
SyntheticReferenceExpression = 327,
Count = 328,
FirstAssignment = 62,
LastAssignment = 74,
FirstCompoundAssignment = 63,
Expand Down Expand Up @@ -431,9 +432,9 @@ declare namespace ts {
LastStatement = 240,
FirstNode = 152,
FirstJSDocNode = 292,
LastJSDocNode = 319,
LastJSDocNode = 320,
FirstJSDocTagNode = 304,
LastJSDocTagNode = 319,
LastJSDocTagNode = 320,
}
export enum NodeFlags {
None = 0,
Expand Down Expand Up @@ -1632,6 +1633,9 @@ declare namespace ts {
export interface JSDocProtectedTag extends JSDocTag {
kind: SyntaxKind.JSDocProtectedTag;
}
export interface JSDocReadonlyTag extends JSDocTag {
kind: SyntaxKind.JSDocReadonlyTag;
}
export interface JSDocEnumTag extends JSDocTag, Declaration {
parent: JSDoc;
kind: SyntaxKind.JSDocEnumTag;
Expand Down Expand Up @@ -3472,6 +3476,8 @@ declare namespace ts {
function getJSDocPrivateTag(node: Node): JSDocPrivateTag | undefined;
/** Gets the JSDoc protected tag for the node if present */
function getJSDocProtectedTag(node: Node): JSDocProtectedTag | undefined;
/** Gets the JSDoc protected tag for the node if present */
function getJSDocReadonlyTag(node: Node): JSDocReadonlyTag | undefined;
/** Gets the JSDoc enum tag for the node if present */
function getJSDocEnumTag(node: Node): JSDocEnumTag | undefined;
/** Gets the JSDoc this tag for the node if present */
Expand Down Expand Up @@ -3679,6 +3685,7 @@ declare namespace ts {
function isJSDocPublicTag(node: Node): node is JSDocPublicTag;
function isJSDocPrivateTag(node: Node): node is JSDocPrivateTag;
function isJSDocProtectedTag(node: Node): node is JSDocProtectedTag;
function isJSDocReadonlyTag(node: Node): node is JSDocReadonlyTag;
function isJSDocEnumTag(node: Node): node is JSDocEnumTag;
function isJSDocThisTag(node: Node): node is JSDocThisTag;
function isJSDocParameterTag(node: Node): node is JSDocParameterTag;
Expand Down
45 changes: 26 additions & 19 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,23 +386,24 @@ declare namespace ts {
JSDocPublicTag = 308,
JSDocPrivateTag = 309,
JSDocProtectedTag = 310,
JSDocCallbackTag = 311,
JSDocEnumTag = 312,
JSDocParameterTag = 313,
JSDocReturnTag = 314,
JSDocThisTag = 315,
JSDocTypeTag = 316,
JSDocTemplateTag = 317,
JSDocTypedefTag = 318,
JSDocPropertyTag = 319,
SyntaxList = 320,
NotEmittedStatement = 321,
PartiallyEmittedExpression = 322,
CommaListExpression = 323,
MergeDeclarationMarker = 324,
EndOfDeclarationMarker = 325,
SyntheticReferenceExpression = 326,
Count = 327,
JSDocReadonlyTag = 311,
JSDocCallbackTag = 312,
JSDocEnumTag = 313,
JSDocParameterTag = 314,
JSDocReturnTag = 315,
JSDocThisTag = 316,
JSDocTypeTag = 317,
JSDocTemplateTag = 318,
JSDocTypedefTag = 319,
JSDocPropertyTag = 320,
SyntaxList = 321,
NotEmittedStatement = 322,
PartiallyEmittedExpression = 323,
CommaListExpression = 324,
MergeDeclarationMarker = 325,
EndOfDeclarationMarker = 326,
SyntheticReferenceExpression = 327,
Count = 328,
FirstAssignment = 62,
LastAssignment = 74,
FirstCompoundAssignment = 63,
Expand Down Expand Up @@ -431,9 +432,9 @@ declare namespace ts {
LastStatement = 240,
FirstNode = 152,
FirstJSDocNode = 292,
LastJSDocNode = 319,
LastJSDocNode = 320,
FirstJSDocTagNode = 304,
LastJSDocTagNode = 319,
LastJSDocTagNode = 320,
}
export enum NodeFlags {
None = 0,
Expand Down Expand Up @@ -1632,6 +1633,9 @@ declare namespace ts {
export interface JSDocProtectedTag extends JSDocTag {
kind: SyntaxKind.JSDocProtectedTag;
}
export interface JSDocReadonlyTag extends JSDocTag {
kind: SyntaxKind.JSDocReadonlyTag;
}
export interface JSDocEnumTag extends JSDocTag, Declaration {
parent: JSDoc;
kind: SyntaxKind.JSDocEnumTag;
Expand Down Expand Up @@ -3472,6 +3476,8 @@ declare namespace ts {
function getJSDocPrivateTag(node: Node): JSDocPrivateTag | undefined;
/** Gets the JSDoc protected tag for the node if present */
function getJSDocProtectedTag(node: Node): JSDocProtectedTag | undefined;
/** Gets the JSDoc protected tag for the node if present */
function getJSDocReadonlyTag(node: Node): JSDocReadonlyTag | undefined;
/** Gets the JSDoc enum tag for the node if present */
function getJSDocEnumTag(node: Node): JSDocEnumTag | undefined;
/** Gets the JSDoc this tag for the node if present */
Expand Down Expand Up @@ -3679,6 +3685,7 @@ declare namespace ts {
function isJSDocPublicTag(node: Node): node is JSDocPublicTag;
function isJSDocPrivateTag(node: Node): node is JSDocPrivateTag;
function isJSDocProtectedTag(node: Node): node is JSDocProtectedTag;
function isJSDocReadonlyTag(node: Node): node is JSDocReadonlyTag;
function isJSDocEnumTag(node: Node): node is JSDocEnumTag;
function isJSDocThisTag(node: Node): node is JSDocThisTag;
function isJSDocParameterTag(node: Node): node is JSDocParameterTag;
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/jsDeclarationsClasses.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ export class E<T, U> {
* @type {string}
* @readonly
*/
static staticReadonlyField: string;
static readonly staticReadonlyField: string;
static staticInitializedField: number;
/**
* @param {string} _p
Expand Down Expand Up @@ -508,7 +508,7 @@ export class E<T, U> {
* @type {T & U}
* @readonly
*/
readonlyField: T & U;
readonly readonlyField: T & U;
initializedField: number;
/**
* @param {U} _p
Expand Down
30 changes: 30 additions & 0 deletions tests/baselines/reference/jsdocReadonly.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
tests/cases/conformance/jsdoc/jsdocReadonly.js(23,3): error TS2540: Cannot assign to 'y' because it is a read-only property.


==== tests/cases/conformance/jsdoc/jsdocReadonly.js (1 errors) ====
class LOL {
/**
* @readonly
* @private
* @type {number}
* Order rules do not apply to JSDoc
*/
x = 1
/** @readonly */
y = 2
/** @readonly Definitely not here */
static z = 3
/** @readonly This is OK too */
constructor() {
/** ok */
this.y = 2
/** @readonly ok */
this.ka = 2
}
}

var l = new LOL()
l.y = 12
~
!!! error TS2540: Cannot assign to 'y' because it is a read-only property.

Loading