Add new special assignment kinds for recognizing Object.defineProperty calls #27208

weswigham · 2018-09-19T00:35:17Z

As special property assignments in JS.

Fixes #25854
Fixes #26082
Fixes #6651

…y calls

sandersn · 2018-09-19T23:38:57Z

Does it also fix #26082 and #6651? (I have yet to read the code.)

sandersn

Initial comments. Most of them are nitpicky, and I haven't looked at binder or checker yet.

sandersn · 2018-09-19T23:41:24Z

src/compiler/utilities.ts

        const special = getAssignmentDeclarationKindWorker(expr);
        return special === AssignmentDeclarationKind.Property || isInJSFile(expr) ? special : AssignmentDeclarationKind.None;
    }

-    function getAssignmentDeclarationKindWorker(expr: BinaryExpression): AssignmentDeclarationKind {
+    export function isBindableObjectDefinePropertyCall(expr: CallExpression): expr is BindableObjectDefinePropertyCall {
+            return !(length(expr.arguments) !== 3 ||


! ... !== .. || ? Why not write it as ... === ... && ?

Also, I thought you said the third one was optional? I guess we won't be able to get the type in that case, but we could still bind the property with type any.

It's not optional, it just has a few different shapes it can take.

sandersn · 2018-09-19T23:45:44Z

src/compiler/checker.ts

        function isReadonlySymbol(symbol: Symbol): boolean {
            // The following symbols are considered read-only:
            // Properties with a 'readonly' modifier
            // Variables declared with 'const'
            // Get accessors without matching set accessors
            // Enum members
+            // JS Assignments with writable false or no setter


pedantically (since I spent a day thinking about terms), it's better to say // define-property assignments with writable false or no setter. This parallels terminology of things that are parllel, like "this-property assignments" or "prototype assignments".

sandersn · 2018-09-19T23:48:45Z

src/compiler/types.ts

+    /** @internal */
+    export type BindableObjectDefinePropertyCall = CallExpression & { arguments: { 0: EntityNameExpression, 1: StringLiteralLike | NumericLiteral, 2: ObjectLiteralExpression } };
+
+


extra newline

sandersn · 2018-09-19T23:51:13Z

src/compiler/utilities.ts

+                case SyntaxKind.PropertyAssignment:
+                    return init.initializer;
+                case SyntaxKind.ShorthandPropertyAssignment:
+                case SyntaxKind.MethodDeclaration:


which ones are allowed to be methods? get? set? value?

Get and set often are. Value can be, it'd be the same as a property with a function expression value, though.

I think this function is unused in my current implementation, though.

sandersn · 2018-09-19T23:54:46Z

src/compiler/utilities.ts

+                return AssignmentDeclarationKind.None;
+            }
+            const entityName = expr.arguments[0];
+            if ((isIdentifier(entityName) && entityName.escapedText === "exports") || (isPropertyAccessExpression(entityName) && isIdentifier(entityName.expression) && entityName.expression.escapedText === "module" && entityName.name.escapedText === "exports")) {


newline needed in here somewhere

weswigham · 2018-09-20T00:00:29Z

#6651 definitely is included here. At least for JS.

weswigham · 2018-09-20T00:06:21Z

I think I need extra handling to cover the prototype assignment case as in Object.defineProperty(Person.prototype for #26082.

weswigham · 2018-09-24T21:12:59Z

@sandersn I've added support for prototype property assignments.

sandersn · 2018-10-09T23:11:12Z

src/compiler/binder.ts

+                        case AssignmentDeclarationKind.None:
+                            break; // Nothing to do
+                        default:
+                            return Debug.fail("Unknown call expression assignment declaration kind");


Debug.assertNever is better here I think.

assignmentKind has all the non-object-define values still "possible" (they're not, because it's a call expression and not a binary expression) and so isn't never; so not quite?

sandersn · 2018-10-09T23:13:47Z

src/compiler/utilities.ts

+                return AssignmentDeclarationKind.None;
+            }
+            const entityName = expr.arguments[0];
+            if ((isIdentifier(entityName) && entityName.escapedText === "exports") ||


maybe we should have a is[Module]Exports utility since this is repeated for the ExportsProperty case below.

Hilariously enough, 10 lines up we already have one, with exactly what i was going to name it.

sandersn · 2018-10-09T23:21:33Z

src/compiler/checker.ts

+        function getInitializerTypeFromAssignmentDeclaration(symbol: Symbol, resolvedSymbol: Symbol | undefined, expression: BinaryExpression | CallExpression, kind: AssignmentDeclarationKind) {
+            if (isCallExpression(expression)) {
+                if (resolvedSymbol) {
+                    return getTypeOfSymbol(resolvedSymbol); // This shouldn't happen except under some hopefully forbidden merges of export assignments and object define assignments


is there a test for this case?

AFAIK I can't construct something that can actually hit this codepath currently (since something like Object.defineProperty(module, "exports", { value: "no" }) won't be bound to the module in the first place); but I wanna avoid crashing and burning if we change something, and this seems reasonable (ignoring the reassignment). I'll add a test with similar content that could trigger it if we ever do start binding it a bit differently, though.

src/compiler/checker.ts

sandersn · 2018-10-09T23:25:30Z

src/compiler/checker.ts

+                isObjectLiteralExpression(container.parent.parent) &&
+                isCallExpression(container.parent.parent.parent) &&
+                container.parent.parent.parent.arguments[2] === container.parent.parent &&
+                getAssignmentDeclarationKind(container.parent.parent.parent) === AssignmentDeclarationKind.ObjectDefinePrototypeProperty) {


(this applies to all the cases in this function) Would it be possible, and more efficient, to just check that parent x 3 exists and then just check the result of getAssignmentDeclarationKind? I think the rest of the predicate is duplicated with it.

Not quite - these are actually some extra constraints which ask that the third argument be specifically an object literal (rather than anything we might be able to pull a type from), to inject a this type into the method defined in it, even if Object.defineProperty's signature lacks a strong this (and, in fact, it does lack one).

sandersn · 2018-10-09T23:27:01Z

src/compiler/checker.ts

+                case AssignmentDeclarationKind.ObjectDefinePropertyValue:
+                case AssignmentDeclarationKind.ObjectDefinePropertyExports:
+                case AssignmentDeclarationKind.ObjectDefinePrototypeProperty:
+                    return Debug.fail("Unimplemented");


"Not yet implemented" or "Does not apply"? Isn't it the latter?

Yea, definitely the later.

src/compiler/checker.ts

sandersn

Some questions and suggestions, plus I need to look at the tests.

sandersn · 2018-10-10T15:59:58Z

Can you see what happens with the user tests? (and maybe RWC, although I don't think there's much JS there.)

tests/baselines/reference/checkExportsObjectAssignProperty.errors.txt

sandersn · 2018-10-10T16:08:01Z

tests/baselines/reference/checkObjectDefineProperty.errors.txt

+
+==== tests/cases/conformance/jsdoc/index.js (0 errors) ====
+    const x = {};
+    Object.defineProperty(x, "name", { value: "Charles", writable: true });


you should have a value test that leaves off writable, if that's allowed. Actually, you should have a test with value left out too. And maybe a test with some of the properties we don't care about?

Also a test with non-literals, just in case that hurts our ... error reporting or something?

According to MDN's Object.defineProperty page, we should have an error if both get/set and value are present. I didn't see a test for that either.

Also, who spells 'writeable' as 'writable'?
Later:everybody else, it turns out.

weswigham · 2018-10-10T20:28:12Z

@sandersn I've replied to or addressed all of your comments; how'd you feel about this now?

sandersn · 2018-10-11T15:15:10Z

Looks good except for four things:

A test that has an initialiser that is just a variable, not some literal.
A test that has a property name that is a variable of type string. (It shouldn’t result in a property, right?)
A test that has a property name that is a variable with constant value, if the behaviour is different than 2.
An error for when you have both value and get+set, or neither.

weswigham · 2018-10-11T16:36:05Z

An error for when you have both value and get+set, or neither

The function is still typechecked as a function call, so that aughta still be happening provided the lib definition is written such that the argument is a union (it isn't, it uses a single weak type).

sandersn · 2018-10-11T17:01:15Z

OK, then I guess we should consider changing it afterward. It would probably break some code, though.

sandersn · 2018-10-19T16:07:28Z

@weswigham It would be nice to get this one in. Will you have time to add those tests some time?

weswigham · 2018-10-19T16:22:13Z

Yeah, I'll take some time to add them today.

weswigham · 2018-10-19T20:11:42Z

@sandersn done

quantizor · 2020-04-08T16:37:03Z

Sorry to comment on a long closed issue, but is this also implemented for Object.defineProperties? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperties

weswigham · 2020-04-08T19:38:43Z

Nope.

calvintwr · 2021-08-15T15:04:00Z

Does this still work? It's not for me in VScode:

Version: 1.59.0
Commit: 379476f0e13988d90fab105c5c19e7abc8b1dea8
Date: 2021-08-04T23:14:40.191Z
Electron: 13.1.7
Chrome: 91.0.4472.124
Node.js: 14.16.0
V8: 9.1.269.36-electron.0
OS: Darwin x64 19.6.0

Suppose the following code:

test.ts:

const something = {
    somethingValue: 'test'
}
Object.defineProperty(something, 'definedProperty', {
    value: 'hello'
})
export default something

test2.ts:

import Thing from './test'

Thing.somethingValue // this is okay
Thing.definedProperty // this will have red curly underline

So why does Thing.definedProperty have red curlies? @weswigham @sandersn

weswigham · 2021-08-15T17:27:36Z

We only recognize them in .js files, not .ts.

calvintwr · 2021-08-15T17:35:56Z

I see. But because it wasn't working in JS, that's why I converted it to TS to see if it works.

I reverted the files to JS, and the screenshot below shows that it wasn't able to see the property defined using #defineProperty:

enricoangelon · 2021-11-27T12:04:11Z

We only recognize them in .js files, not .ts.

I think it's time for a .ts implementation too!

AR0x7E7 · 2022-04-02T13:23:44Z

using Visual Studio Code 1.66.0 I can confirm that Object.defineProperty appears to be not recognized

Add new special assignment kinds for recognizing Object.definePropert…

66a5531

…y calls

weswigham requested review from sandersn, RyanCavanaugh and a user September 19, 2018 00:35

sandersn reviewed Sep 19, 2018

View reviewed changes

RyanCavanaugh added this to the TypeScript 3.2 milestone Sep 20, 2018

weswigham added 3 commits September 24, 2018 10:41

Merge branch 'master' into special-object-assignment

f91debf

Merge branch 'master' into special-object-assignment

a1780fe

Add support for prototype assignments, fix nits

66d2bb7

sandersn self-assigned this Oct 9, 2018

sandersn reviewed Oct 9, 2018

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

sandersn reviewed Oct 9, 2018

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

sandersn requested changes Oct 9, 2018

View reviewed changes

sandersn reviewed Oct 10, 2018

View reviewed changes

tests/baselines/reference/checkExportsObjectAssignProperty.errors.txt Show resolved Hide resolved

sandersn reviewed Oct 10, 2018

View reviewed changes

Fix code review comments

01589fb

weswigham added 2 commits October 19, 2018 10:54

Merge branch 'master' into special-object-assignment

71e8d8c

Add test documenting behavior in a few more odd scenarios

8da77c6

sandersn approved these changes Oct 19, 2018

View reviewed changes

weswigham merged commit 69b1cb5 into microsoft:master Oct 19, 2018

weswigham deleted the special-object-assignment branch October 19, 2018 21:32

weswigham mentioned this pull request Nov 6, 2018

Javascript should support defineProperty assignments to exports #22439

Closed

		/** @internal */
		export type BindableObjectDefinePropertyCall = CallExpression & { arguments: { 0: EntityNameExpression, 1: StringLiteralLike \| NumericLiteral, 2: ObjectLiteralExpression } };

Add new special assignment kinds for recognizing Object.defineProperty calls #27208

Add new special assignment kinds for recognizing Object.defineProperty calls #27208

Conversation

weswigham commented Sep 19, 2018 • edited Loading

sandersn commented Sep 19, 2018

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Sep 20, 2018

weswigham commented Sep 20, 2018

weswigham commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

sandersn commented Oct 10, 2018

Choose a reason for hiding this comment

weswigham commented Oct 10, 2018

sandersn commented Oct 11, 2018 • edited Loading

weswigham commented Oct 11, 2018

sandersn commented Oct 11, 2018

sandersn commented Oct 19, 2018

weswigham commented Oct 19, 2018

weswigham commented Oct 19, 2018

quantizor commented Apr 8, 2020

weswigham commented Apr 8, 2020

calvintwr commented Aug 15, 2021

Suppose the following code:

weswigham commented Aug 15, 2021

calvintwr commented Aug 15, 2021 • edited Loading

enricoangelon commented Nov 27, 2021

AR0x7E7 commented Apr 2, 2022 • edited Loading

weswigham commented Sep 19, 2018 •

edited

Loading

sandersn commented Oct 11, 2018 •

edited

Loading

calvintwr commented Aug 15, 2021 •

edited

Loading

AR0x7E7 commented Apr 2, 2022 •

edited

Loading