Skip to content
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

Add useDefineForClassFields flag for Set -> Define property declaration #33509

Merged
merged 57 commits into from
Sep 26, 2019
Merged
Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
f76e01f
Disallow property/accessor overrides
sandersn Sep 12, 2019
ac96739
Disallow uninitialised property overrides
sandersn Sep 13, 2019
c26785e
Merge branch 'master' into disallow-property-accessor-override
sandersn Sep 13, 2019
c4f334a
Updates from design review + fix ancient bug
sandersn Sep 13, 2019
70a15bd
Need to add a couple of errors and squash one
sandersn Sep 16, 2019
feb0fb6
Everything works so far
sandersn Sep 16, 2019
8686242
Check for constructor initialisation
sandersn Sep 16, 2019
2916c29
change error wording
sandersn Sep 16, 2019
2e5f57c
Improve error wording
sandersn Sep 16, 2019
f11f2be
Add codefix to add missing 'declare'
sandersn Sep 17, 2019
526ceb7
Merge branch 'master' into disallow-property-accessor-override
sandersn Sep 17, 2019
a8f4610
Merge branch 'master' into disallow-uninitialised-property-override
sandersn Sep 17, 2019
2238b66
Always emit accessors in .d.ts files
sandersn Sep 17, 2019
fcf0ff1
Allow 'declare' on any uninitialised property decl
sandersn Sep 17, 2019
810f923
Undo code moves
sandersn Sep 17, 2019
6408d7a
Let sleeping dogs lie
sandersn Sep 17, 2019
5ee3271
Correctly set NodeFlags.Ambient
sandersn Sep 17, 2019
e0ddced
Remove more unneeded code
sandersn Sep 17, 2019
610a62d
Merge branch 'disallow-property-accessor-override' into add-property-…
sandersn Sep 17, 2019
e7ba1ae
Merge branch 'disallow-uninitialised-property-override' into add-prop…
sandersn Sep 17, 2019
6c94395
Merge branch 'always-emit-accessors-in-dts' into add-property-define-…
sandersn Sep 17, 2019
6a066b9
Update baselines
sandersn Sep 17, 2019
0548220
Merge branch 'disallow-uninitialised-property-override' into sandersn…
sandersn Sep 17, 2019
7f69be7
Update baselines
sandersn Sep 17, 2019
8f42126
Merge branch 'always-emit-accessors-in-dts' into sandersn/add-propert…
sandersn Sep 17, 2019
d46a0db
Update baselines
sandersn Sep 17, 2019
58e1746
Ignore this-property assignments
sandersn Sep 19, 2019
9b358b1
Merge branch 'disallow-property-accessor-override' into sandersn/add-…
sandersn Sep 19, 2019
832d51f
Fix base-in-interface check
sandersn Sep 19, 2019
9c6aa17
Merge branch 'disallow-property-accessor-override' into sandersn/add-…
sandersn Sep 19, 2019
a645ca9
Do not error when base parent is interface
sandersn Sep 19, 2019
030c768
Fix base interface check
sandersn Sep 19, 2019
003d041
Add missed baselines
sandersn Sep 19, 2019
6ec445f
Fix check
sandersn Sep 19, 2019
c283574
Fix new errors in services
sandersn Sep 19, 2019
1c5f699
Fix new errors in services
sandersn Sep 19, 2019
2403116
Merge branch 'disallow-property-accessor-override' into sandersn/add-…
sandersn Sep 19, 2019
b45b5ba
Merge branch 'disallow-uninitialised-property-override' into sandersn…
sandersn Sep 19, 2019
8d99856
Fix errors in testRunner
sandersn Sep 19, 2019
f400822
Add flag and turn off errors when on
sandersn Sep 19, 2019
1f68bea
Structure of new emit is correct, fake content
sandersn Sep 20, 2019
6c35ac2
Basically right emit
sandersn Sep 20, 2019
fbeaf01
Fix one last unitialised property declaration
sandersn Sep 20, 2019
a5b1d8f
Haha no I missed another one
sandersn Sep 20, 2019
32a73b3
Fix whitespace back to CRLF
sandersn Sep 20, 2019
5028bfa
Minor fix and code cleanup
sandersn Sep 20, 2019
8053e05
New test case
sandersn Sep 20, 2019
7fdb423
Fix bug in isInitializedProperty
sandersn Sep 23, 2019
18c622c
Updates from design meeting.
sandersn Sep 23, 2019
8268f9a
Update baselines
sandersn Sep 23, 2019
5eeb2b0
Object.defineProperty for methods too
sandersn Sep 23, 2019
5ea2bc1
Update slow baselines
sandersn Sep 23, 2019
088daa8
Improve error message
sandersn Sep 23, 2019
e600ebe
Update src/compiler/transformers/utilities.ts
sandersn Sep 23, 2019
6e3bf5c
Add test of computed properties
sandersn Sep 23, 2019
ba876cf
Remove done TODO
sandersn Sep 23, 2019
6a1e24c
Merge branch 'master' into sandersn/add-property-define-flag
sandersn Sep 26, 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
60 changes: 50 additions & 10 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29443,7 +29443,6 @@ namespace ts {
}

function checkKindsOfPropertyMemberOverrides(type: InterfaceType, baseType: BaseType): void {

// TypeScript 1.0 spec (April 2014): 8.2.3
// A derived class inherits all members from its base class it doesn't override.
// Inheritance means that a derived class implicitly contains all non - overridden members of the base class.
Expand Down Expand Up @@ -29477,7 +29476,6 @@ namespace ts {
// type declaration, derived and base resolve to the same symbol even in the case of generic classes.
if (derived === base) {
// derived class inherits base without override/redeclaration

const derivedClassDecl = getClassLikeDeclarationOfSymbol(type.symbol)!;

// It is an error to inherit an abstract member without implementing it or being declared abstract.
Expand Down Expand Up @@ -29514,14 +29512,53 @@ namespace ts {
continue;
}

if (isPrototypeProperty(base) || base.flags & SymbolFlags.PropertyOrAccessor && derived.flags & SymbolFlags.PropertyOrAccessor) {
// method is overridden with method or property/accessor is overridden with property/accessor - correct case
continue;
}

let errorMessage: DiagnosticMessage;
if (isPrototypeProperty(base)) {
if (derived.flags & SymbolFlags.Accessor) {
const basePropertyFlags = base.flags & SymbolFlags.PropertyOrAccessor;
const derivedPropertyFlags = derived.flags & SymbolFlags.PropertyOrAccessor;
if (basePropertyFlags && derivedPropertyFlags) {
// property/accessor is overridden with property/accessor
if (!compilerOptions.useDefineForClassFields
|| baseDeclarationFlags & ModifierFlags.Abstract && !(base.valueDeclaration && isPropertyDeclaration(base.valueDeclaration) && base.valueDeclaration.initializer)
|| base.valueDeclaration && base.valueDeclaration.parent.kind === SyntaxKind.InterfaceDeclaration
|| derived.valueDeclaration && isBinaryExpression(derived.valueDeclaration)) {
// when the base property is abstract or from an interface, base/derived flags don't need to match
// same when the derived property is from an assignment
continue;
}
if (basePropertyFlags !== SymbolFlags.Property && derivedPropertyFlags === SymbolFlags.Property) {
errorMessage = Diagnostics.Class_0_defines_instance_member_accessor_1_but_extended_class_2_defines_it_as_instance_member_property;
}
else if (basePropertyFlags === SymbolFlags.Property && derivedPropertyFlags !== SymbolFlags.Property) {
errorMessage = Diagnostics.Class_0_defines_instance_member_property_1_but_extended_class_2_defines_it_as_instance_member_accessor;
}
else {
const uninitialized = find(derived.declarations, d => d.kind === SyntaxKind.PropertyDeclaration && !(d as PropertyDeclaration).initializer);
if (uninitialized
&& !(derived.flags & SymbolFlags.Transient)
&& !(baseDeclarationFlags & ModifierFlags.Abstract)
&& !(derivedDeclarationFlags & ModifierFlags.Abstract)
&& !derived.declarations.some(d => d.flags & NodeFlags.Ambient)) {
const constructor = findConstructorDeclaration(getClassLikeDeclarationOfSymbol(type.symbol)!);
const propName = (uninitialized as PropertyDeclaration).name;
if ((uninitialized as PropertyDeclaration).exclamationToken
|| !constructor
|| !isIdentifier(propName)
|| !strictNullChecks
|| !isPropertyInitializedInConstructor(propName, type, constructor)) {
const errorMessage = Diagnostics.Property_0_will_overwrite_the_base_property_in_1_Add_a_declare_modifier_or_an_initializer_to_avoid_this;
Copy link
Member

Choose a reason for hiding this comment

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

Add_a_declare_modifier_or_an_initializer_to_avoid_this

Suggestion: if the base property’s type is identical to the derived property’s type, better advice might be to delete the property declaration altogether.

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 don't know; I saw enough of this that I think there must be some kind of intent behind it. I don't think people truly trust inheritance. I know I don't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although adding an initializer isn't a propos in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Right—adding an an initializer changes the semantics, so assuming you don’t want that, the guidance is to add declare, which is utterly meaningless, no?

Copy link
Member

Choose a reason for hiding this comment

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

I’m not saying that “delete this” must be the only option listed in the error message; maybe

Property {0} will overwrite the base property in {1}. To silence this message, add an initializer. To avoid overwriting the base property, add a 'declare' modifier or remove this redundant declaration.

Copy link
Member Author

@sandersn sandersn Sep 23, 2019

Choose a reason for hiding this comment

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

How about:

"Property '{0}' will overwrite the base property in '{1}'. If this is intentional, add an initializer. Otherwise, add a 'declare' modifier or remove the redundant declaration.": {

(the base declaration might be the redundant one, so I changed "this redundant" to "the redundant".)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. I like that it clarifies that adding an initializer and adding a declare modifier essentially have opposite semantics. The original error message kind of sounds like they’ll have the same effect.

error(getNameOfDeclaration(derived.valueDeclaration) || derived.valueDeclaration, errorMessage, symbolToString(base), typeToString(baseType));
}
}
// correct case
continue;
}
}
else if (isPrototypeProperty(base)) {
if (isPrototypeProperty(derived)) {
// method is overridden with method -- correct case
continue;
}
else if (derived.flags & SymbolFlags.Accessor) {
errorMessage = Diagnostics.Class_0_defines_instance_member_function_1_but_extended_class_2_defines_it_as_instance_member_accessor;
}
else {
Expand Down Expand Up @@ -29583,6 +29620,9 @@ namespace ts {
}
const constructor = findConstructorDeclaration(node);
for (const member of node.members) {
if (getModifierFlags(member) & ModifierFlags.Ambient) {
continue;
}
if (isInstancePropertyWithoutInitializer(member)) {
const propName = (<PropertyDeclaration>member).name;
if (isIdentifier(propName)) {
Expand Down Expand Up @@ -32507,7 +32547,7 @@ namespace ts {
else if (flags & ModifierFlags.Async) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_in_an_ambient_context, "async");
}
else if (isClassLike(node.parent)) {
else if (isClassLike(node.parent) && !isPropertyDeclaration(node)) {
return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_class_element, "declare");
}
else if (node.kind === SyntaxKind.Parameter) {
Expand Down
7 changes: 7 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,13 @@ namespace ts {
category: Diagnostics.Advanced_Options,
description: Diagnostics.Disable_strict_checking_of_generic_signatures_in_function_types,
},
{
name: "useDefineForClassFields",
type: "boolean",
affectsSemanticDiagnostics: true,
category: Diagnostics.Advanced_Options,
description: Diagnostics.Emit_class_fields_with_Define_instead_of_Set,
},
{
name: "keyofStringsOnly",
type: "boolean",
Expand Down
38 changes: 34 additions & 4 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -2204,6 +2204,19 @@
"category": "Error",
"code": 2609
},
"Class '{0}' defines instance member accessor '{1}', but extended class '{2}' defines it as instance member property.": {
"category": "Error",
"code": 2610
},
"Class '{0}' defines instance member property '{1}', but extended class '{2}' defines it as instance member accessor.": {
"category": "Error",
"code": 2611
},
"Property '{0}' will overwrite the base property in '{1}'. Add a 'declare' modifier or an initializer to avoid this.": {
"category": "Error",
"code": 2612
},

"Cannot augment module '{0}' with value exports because it resolves to a non-module entity.": {
"category": "Error",
"code": 2649
Expand Down Expand Up @@ -3080,6 +3093,10 @@
"category": "Error",
"code": 5047
},
"Option '{0}' cannot be specified when option 'target' is 'ES3'.": {
"category": "Error",
"code": 5048
},
"Option '{0} can only be used when either option '--inlineSourceMap' or option '--sourceMap' is provided.": {
"category": "Error",
"code": 5051
Expand Down Expand Up @@ -3934,6 +3951,10 @@
"Conflicts are in this file.": {
"category": "Message",
"code": 6201
},
"Project references may not form a circular graph. Cycle detected: {0}": {
"category": "Error",
"code": 6202
},
"'{0}' was also declared here.": {
"category": "Message",
Expand Down Expand Up @@ -4007,6 +4028,10 @@
"category": "Message",
"code": 6220
},
"Emit class fields with Define instead of Set.": {
"category": "Message",
"code": 6221
},

"Projects to reference": {
"category": "Message",
Expand All @@ -4016,10 +4041,7 @@
"category": "Message",
"code": 6302
},
"Project references may not form a circular graph. Cycle detected: {0}": {
"category": "Error",
"code": 6202
},

"Composite projects may not disable declaration emit.": {
"category": "Error",
"code": 6304
Expand Down Expand Up @@ -5144,6 +5166,14 @@
"category": "Message",
"code": 95093
},
"Prefix with 'declare'": {
"category": "Message",
"code": 95094
},
"Prefix all incorrect property declarations with 'declare'": {
"category": "Message",
"code": 95095
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
48 changes: 46 additions & 2 deletions src/compiler/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,47 @@ namespace ts {
return node;
}

function createMethodCall(object: Expression, methodName: string | Identifier, argumentsList: readonly Expression[]) {
return createCall(
createPropertyAccess(object, asName(methodName)),
/*typeArguments*/ undefined,
argumentsList
);
}

function createGlobalMethodCall(globalObjectName: string, methodName: string, argumentsList: readonly Expression[]) {
return createMethodCall(createIdentifier(globalObjectName), methodName, argumentsList);
}

/* @internal */
export function createObjectDefinePropertyCall(target: Expression, propertyName: string | Expression, attributes: Expression) {
return createGlobalMethodCall("Object", "defineProperty", [target, asExpression(propertyName), attributes]);
}

function tryAddPropertyAssignment(properties: Push<PropertyAssignment>, propertyName: string, expression: Expression | undefined) {
if (expression) {
properties.push(createPropertyAssignment(propertyName, expression));
return true;
}
return false;
}

/* @internal */
export function createPropertyDescriptor(attributes: PropertyDescriptorAttributes, singleLine?: boolean) {
const properties: PropertyAssignment[] = [];
tryAddPropertyAssignment(properties, "enumerable", asExpression(attributes.enumerable));
tryAddPropertyAssignment(properties, "configurable", asExpression(attributes.configurable));

let isData = tryAddPropertyAssignment(properties, "writable", asExpression(attributes.writable));
isData = tryAddPropertyAssignment(properties, "value", attributes.value) || isData;

let isAccessor = tryAddPropertyAssignment(properties, "get", attributes.get);
isAccessor = tryAddPropertyAssignment(properties, "set", attributes.set) || isAccessor;

Debug.assert(!(isData && isAccessor), "A PropertyDescriptor may not be both an accessor descriptor and a data descriptor.");
return createObjectLiteral(properties, !singleLine);
}

export function updateMethod(
node: MethodDeclaration,
decorators: readonly Decorator[] | undefined,
Expand Down Expand Up @@ -3136,8 +3177,11 @@ namespace ts {
return isString(name) ? createIdentifier(name) : name;
}

function asExpression(value: string | number | Expression) {
return isString(value) || typeof value === "number" ? createLiteral(value) : value;
function asExpression<T extends Expression | undefined>(value: string | number | boolean | T): T | StringLiteral | NumericLiteral | BooleanLiteral {
return typeof value === "string" ? createStringLiteral(value) :
typeof value === "number" ? createNumericLiteral(""+value) :
typeof value === "boolean" ? value ? createTrue() : createFalse() :
value;
}

function asNodeArray<T extends Node>(array: readonly T[]): NodeArray<T>;
Expand Down
12 changes: 10 additions & 2 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5982,8 +5982,16 @@ namespace ts {
token() === SyntaxKind.NumericLiteral ||
token() === SyntaxKind.AsteriskToken ||
token() === SyntaxKind.OpenBracketToken) {

return parsePropertyOrMethodDeclaration(<PropertyDeclaration | MethodDeclaration>node);
const isAmbient = node.modifiers && some(node.modifiers, isDeclareModifier);
if (isAmbient) {
for (const m of node.modifiers!) {
m.flags |= NodeFlags.Ambient;
}
return doInsideOfContext(NodeFlags.Ambient, () => parsePropertyOrMethodDeclaration(node as PropertyDeclaration | MethodDeclaration));
}
else {
return parsePropertyOrMethodDeclaration(node as PropertyDeclaration | MethodDeclaration);
}
}

if (node.decorators || node.modifiers) {
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3003,6 +3003,10 @@ namespace ts {
}
}

if (options.useDefineForClassFields && languageVersion === ScriptTarget.ES3) {
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_when_option_target_is_ES3, "useDefineForClassFields");
}

if (!options.noEmit && options.allowJs && getEmitDeclarations(options)) {
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_with_option_1, "allowJs", getEmitDeclarationOptionName(options));
}
Expand Down
Loading