Skip to content

Fix #7590: Allow 'readonly' to be used in constructor parameters #8555

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

Merged
6 commits merged into from
May 12, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 5 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12658,7 +12658,7 @@ namespace ts {

checkVariableLikeDeclaration(node);
let func = getContainingFunction(node);
if (node.flags & NodeFlags.AccessibilityModifier) {
if (node.flags & NodeFlags.ParameterPropertyModifier) {
func = getContainingFunction(node);
if (!(func.kind === SyntaxKind.Constructor && nodeIsPresent(func.body))) {
error(node, Diagnostics.A_parameter_property_is_only_allowed_in_a_constructor_implementation);
Expand Down Expand Up @@ -12994,7 +12994,7 @@ namespace ts {
// or the containing class declares instance member variables with initializers.
const superCallShouldBeFirst =
forEach((<ClassDeclaration>node.parent).members, isInstancePropertyWithInitializer) ||
forEach(node.parameters, p => p.flags & (NodeFlags.Public | NodeFlags.Private | NodeFlags.Protected));
forEach(node.parameters, p => p.flags & NodeFlags.ParameterPropertyModifier);

// Skip past any prologue directives to find the first statement
// to ensure that it was a super call.
Expand Down Expand Up @@ -17651,7 +17651,8 @@ namespace ts {
if (flags & NodeFlags.Readonly) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "readonly");
}
else if (node.kind !== SyntaxKind.PropertyDeclaration && node.kind !== SyntaxKind.PropertySignature && node.kind !== SyntaxKind.IndexSignature) {
else if (node.kind !== SyntaxKind.PropertyDeclaration && node.kind !== SyntaxKind.PropertySignature && node.kind !== SyntaxKind.IndexSignature && node.kind !== SyntaxKind.Parameter) {
// If node.kind === SyntaxKind.Parameter, checkParameter report an error if it's not a parameter property.
return grammarErrorOnNode(modifier, Diagnostics.readonly_modifier_can_only_appear_on_a_property_declaration_or_index_signature);
}
flags |= NodeFlags.Readonly;
Expand Down Expand Up @@ -17759,7 +17760,7 @@ namespace ts {
else if ((node.kind === SyntaxKind.ImportDeclaration || node.kind === SyntaxKind.ImportEqualsDeclaration) && flags & NodeFlags.Ambient) {
return grammarErrorOnNode(lastDeclare, Diagnostics.A_0_modifier_cannot_be_used_with_an_import_declaration, "declare");
}
else if (node.kind === SyntaxKind.Parameter && (flags & NodeFlags.AccessibilityModifier) && isBindingPattern((<ParameterDeclaration>node).name)) {
else if (node.kind === SyntaxKind.Parameter && (flags & NodeFlags.ParameterPropertyModifier) && isBindingPattern((<ParameterDeclaration>node).name)) {
return grammarErrorOnNode(node, Diagnostics.A_parameter_property_may_not_be_a_binding_pattern);
}
if (flags & NodeFlags.Async) {
Expand Down Expand Up @@ -18224,9 +18225,6 @@ namespace ts {
if (parameter.dotDotDotToken) {
return grammarErrorOnNode(parameter.dotDotDotToken, Diagnostics.A_set_accessor_cannot_have_rest_parameter);
}
else if (parameter.flags & NodeFlags.Modifier) {
return grammarErrorOnNode(accessor.name, Diagnostics.A_parameter_property_is_only_allowed_in_a_constructor_implementation);
}
else if (parameter.questionToken) {
return grammarErrorOnNode(parameter.questionToken, Diagnostics.A_set_accessor_cannot_have_an_optional_parameter);
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/declarationEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1051,7 +1051,7 @@ namespace ts {
function emitParameterProperties(constructorDeclaration: ConstructorDeclaration) {
if (constructorDeclaration) {
forEach(constructorDeclaration.parameters, param => {
if (param.flags & NodeFlags.AccessibilityModifier) {
if (param.flags & NodeFlags.ParameterPropertyModifier) {
emitPropertyDeclaration(param);
}
});
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4979,7 +4979,7 @@ const _super = (function (geti, seti) {

function emitParameterPropertyAssignments(node: ConstructorDeclaration) {
forEach(node.parameters, param => {
if (param.flags & NodeFlags.AccessibilityModifier) {
if (param.flags & NodeFlags.ParameterPropertyModifier) {
writeLine();
emitStart(param);
emitStart(param.name);
Expand Down
4 changes: 3 additions & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -407,8 +407,10 @@ namespace ts {
HasAggregatedChildData = 1 << 29, // If we've computed data from children and cached it in this node
HasJsxSpreadAttribute = 1 << 30,

Modifier = Export | Ambient | Public | Private | Protected | Static | Abstract | Default | Async,
Modifier = Export | Ambient | Public | Private | Protected | Static | Abstract | Default | Async | Readonly,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still want to keep Readonly in ? since you have made a new flag ParameterPropertyModifier

Copy link
Contributor

Choose a reason for hiding this comment

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

it is a modifier..

Copy link
Contributor

Choose a reason for hiding this comment

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

got cha, I got confused why it is here caz i thought it is not direct related with the bug.

AccessibilityModifier = Public | Private | Protected,
// Accessibility modifiers and 'readonly' can be attached to a parameter in a constructor to make it a property.
Copy link
Member

@DanielRosenwasser DanielRosenwasser May 11, 2016

Choose a reason for hiding this comment

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

The term for this is a "parameter property", so maybe a better name for the flag would be ParameterPropertyModifier

ParameterPropertyModifier = AccessibilityModifier | Readonly,
BlockScoped = Let | Const,

ReachabilityCheckFlags = HasImplicitReturn | HasExplicitReturn,
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3021,7 +3021,7 @@ namespace ts {
}

export function isParameterPropertyDeclaration(node: ParameterDeclaration): boolean {
return node.flags & NodeFlags.AccessibilityModifier && node.parent.kind === SyntaxKind.Constructor && isClassLike(node.parent.parent);
return node.flags & NodeFlags.ParameterPropertyModifier && node.parent.kind === SyntaxKind.Constructor && isClassLike(node.parent.parent);
}

export function startsWith(str: string, prefix: string): boolean {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,14 @@
tests/cases/compiler/accessorParameterAccessibilityModifier.ts(3,9): error TS2369: A parameter property is only allowed in a constructor implementation.
tests/cases/compiler/accessorParameterAccessibilityModifier.ts(3,11): error TS2369: A parameter property is only allowed in a constructor implementation.
tests/cases/compiler/accessorParameterAccessibilityModifier.ts(4,16): error TS2369: A parameter property is only allowed in a constructor implementation.
tests/cases/compiler/accessorParameterAccessibilityModifier.ts(4,18): error TS2369: A parameter property is only allowed in a constructor implementation.


==== tests/cases/compiler/accessorParameterAccessibilityModifier.ts (4 errors) ====
==== tests/cases/compiler/accessorParameterAccessibilityModifier.ts (2 errors) ====

class C {
set X(public v) { }
~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
static set X(public v2) { }
~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
}
20 changes: 20 additions & 0 deletions tests/baselines/reference/declarationEmit_readonly.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//// [declarationEmit_readonly.ts]

class C {
constructor(readonly x: number) {}
}

//// [declarationEmit_readonly.js]
var C = (function () {
function C(x) {
this.x = x;
}
return C;
}());


//// [declarationEmit_readonly.d.ts]
declare class C {
readonly x: number;
constructor(x: number);
}
8 changes: 8 additions & 0 deletions tests/baselines/reference/declarationEmit_readonly.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
=== tests/cases/conformance/classes/constructorDeclarations/constructorParameters/declarationEmit_readonly.ts ===

class C {
>C : Symbol(C, Decl(declarationEmit_readonly.ts, 0, 0))

constructor(readonly x: number) {}
>x : Symbol(C.x, Decl(declarationEmit_readonly.ts, 2, 16))
}
8 changes: 8 additions & 0 deletions tests/baselines/reference/declarationEmit_readonly.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
=== tests/cases/conformance/classes/constructorDeclarations/constructorParameters/declarationEmit_readonly.ts ===

class C {
>C : C

constructor(readonly x: number) {}
>x : number
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
tests/cases/conformance/parser/ecmascript5/MemberAccessorDeclarations/parserMemberAccessorDeclaration15.ts(2,8): error TS2369: A parameter property is only allowed in a constructor implementation.
tests/cases/conformance/parser/ecmascript5/MemberAccessorDeclarations/parserMemberAccessorDeclaration15.ts(2,12): error TS2369: A parameter property is only allowed in a constructor implementation.


==== tests/cases/conformance/parser/ecmascript5/MemberAccessorDeclarations/parserMemberAccessorDeclaration15.ts (2 errors) ====
==== tests/cases/conformance/parser/ecmascript5/MemberAccessorDeclarations/parserMemberAccessorDeclaration15.ts (1 errors) ====
class C {
set Foo(public a: number) { }
~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
~~~~~~~~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
}
13 changes: 13 additions & 0 deletions tests/baselines/reference/readonlyInAmbientClass.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyInAmbientClass.ts(2,14): error TS2369: A parameter property is only allowed in a constructor implementation.
tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyInAmbientClass.ts(3,9): error TS2369: A parameter property is only allowed in a constructor implementation.


==== tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyInAmbientClass.ts (2 errors) ====
declare class C{
constructor(readonly x: number);
~~~~~~~~~~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
method(readonly x: number);
~~~~~~~~~~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
}
7 changes: 7 additions & 0 deletions tests/baselines/reference/readonlyInAmbientClass.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//// [readonlyInAmbientClass.ts]
declare class C{
constructor(readonly x: number);
method(readonly x: number);
}

//// [readonlyInAmbientClass.js]
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyInConstructorParameters.ts(4,1): error TS2450: Left-hand side of assignment expression cannot be a constant or a read-only property.
tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyInConstructorParameters.ts(7,26): error TS1029: 'public' modifier must precede 'readonly' modifier.
tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyInConstructorParameters.ts(13,10): error TS2341: Property 'x' is private and only accessible within class 'F'.


==== tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyInConstructorParameters.ts (3 errors) ====
class C {
constructor(readonly x: number) {}
}
new C(1).x = 2;
~~~~~~~~~~
!!! error TS2450: Left-hand side of assignment expression cannot be a constant or a read-only property.

class E {
constructor(readonly public x: number) {}
~~~~~~
!!! error TS1029: 'public' modifier must precede 'readonly' modifier.
}

class F {
constructor(private readonly x: number) {}
}
new F(1).x;
~
!!! error TS2341: Property 'x' is private and only accessible within class 'F'.
36 changes: 36 additions & 0 deletions tests/baselines/reference/readonlyInConstructorParameters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//// [readonlyInConstructorParameters.ts]
class C {
constructor(readonly x: number) {}
}
new C(1).x = 2;

class E {
constructor(readonly public x: number) {}
}

class F {
constructor(private readonly x: number) {}
}
new F(1).x;

//// [readonlyInConstructorParameters.js]
var C = (function () {
function C(x) {
this.x = x;
}
return C;
}());
new C(1).x = 2;
var E = (function () {
function E(x) {
this.x = x;
}
return E;
}());
var F = (function () {
function F(x) {
this.x = x;
}
return F;
}());
new F(1).x;
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
tests/cases/compiler/readonlyInNonPropertyParameters.ts(4,9): error TS2369: A parameter property is only allowed in a constructor implementation.
tests/cases/compiler/readonlyInNonPropertyParameters.ts(5,8): error TS2369: A parameter property is only allowed in a constructor implementation.
tests/cases/compiler/readonlyInNonPropertyParameters.ts(7,2): error TS2369: A parameter property is only allowed in a constructor implementation.


==== tests/cases/compiler/readonlyInNonPropertyParameters.ts (3 errors) ====

// `readonly` won't work outside of property parameters
class X {
method(readonly x: number) {}
~~~~~~~~~~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
set x(readonly value: number) {}
~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
}
(readonly x) => 0;
~~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
// OK to use `readonly` as a name
(readonly) => 0;
27 changes: 27 additions & 0 deletions tests/baselines/reference/readonlyInNonPropertyParameters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//// [readonlyInNonPropertyParameters.ts]

// `readonly` won't work outside of property parameters
class X {
method(readonly x: number) {}
set x(readonly value: number) {}
}
(readonly x) => 0;
// OK to use `readonly` as a name
(readonly) => 0;

//// [readonlyInNonPropertyParameters.js]
// `readonly` won't work outside of property parameters
var X = (function () {
function X() {
}
X.prototype.method = function (x) { };
Object.defineProperty(X.prototype, "x", {
set: function (value) { },
enumerable: true,
configurable: true
});
return X;
}());
(function (x) { return 0; });
// OK to use `readonly` as a name
(function (readonly) { return 0; });
13 changes: 13 additions & 0 deletions tests/baselines/reference/readonlyReadonly.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyReadonly.ts(2,14): error TS1030: 'readonly' modifier already seen.
tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyReadonly.ts(3,26): error TS1030: 'readonly' modifier already seen.


==== tests/cases/conformance/classes/constructorDeclarations/constructorParameters/readonlyReadonly.ts (2 errors) ====
class C {
readonly readonly x: number;
~~~~~~~~
!!! error TS1030: 'readonly' modifier already seen.
constructor(readonly readonly y: number) {}
~~~~~~~~
!!! error TS1030: 'readonly' modifier already seen.
}
13 changes: 13 additions & 0 deletions tests/baselines/reference/readonlyReadonly.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//// [readonlyReadonly.ts]
class C {
readonly readonly x: number;
constructor(readonly readonly y: number) {}
}

//// [readonlyReadonly.js]
var C = (function () {
function C(y) {
this.y = y;
}
return C;
}());
10 changes: 10 additions & 0 deletions tests/cases/compiler/readonlyInNonPropertyParameters.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//@target: ES5

// `readonly` won't work outside of property parameters
class X {
method(readonly x: number) {}
set x(readonly value: number) {}
}
(readonly x) => 0;
// OK to use `readonly` as a name
(readonly) => 0;
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// @declaration: true

class C {
constructor(readonly x: number) {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
declare class C{
constructor(readonly x: number);
method(readonly x: number);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class C {
constructor(readonly x: number) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

also to clarify if it is just readonly without accessibility modifier, are we assuming it is public?

Copy link
Author

Choose a reason for hiding this comment

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

Yep. This is the same as for properties. (class C { readonly x: number; })

}
new C(1).x = 2;

class E {
constructor(readonly public x: number) {}
}

class F {
constructor(private readonly x: number) {}
}
new F(1).x;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class C {
readonly readonly x: number;
constructor(readonly readonly y: number) {}
}