Report deprecation for object literal assignments (#39374) #46167

ypresto · 2021-10-01T22:03:34Z

It's already in version milestone (although rescheduled sometimes), but it seems no one assigned, I tried to implement it.
Context: I want to use this to help upgrading major version of material-ui in my app. :)

We need diagnoses only for contextually typed object literals, and it is very similar to conditions for excess properties check. So I placed the logic to around of it.

This intendedly hides suggestion if any type errors in the assignment. Showing deprecation even if an error is found is perhaps better in some cases. But it requires additional traversing cost and also it can be noisy for majority cases.

Fixes #39374

sandersn

Functionality looks right to me, it just needs to be called from the right place.

sandersn · 2021-10-08T21:47:23Z

src/compiler/checker.ts

@@ -16924,7 +16924,7 @@ namespace ts {
            containingMessageChain: (() => DiagnosticMessageChain | undefined) | undefined,
            errorOutputContainer: { errors?: Diagnostic[], skipLogging?: boolean } | undefined
        ): boolean {
-            if (isTypeRelatedTo(source, target, relation)) return true;
+            if (isTypeRelatedTo(source, target, relation, /*reportDeprecatedProperties*/ true)) return true;


Are any other deprecations reported from isTypeRelatedTo? Based on my experience, it doesn't seem like the logical place for them.

Later: I checked the other callers of addDeprecatedSuggestion and they're all check* functions -- the top-level functions whose primary function is to issue errors.

You probably want to call your code starting from checkJSXAttribute or checkPropertyAssignment. Those both call checkExpressionForMutableLocation.

Also, there's already deprecation checking in checkIdentifier, for uncalled functions I believe. That code might apply after a few tweaks.

Thank you for feedback!

The reason I used checkTypeRelatedToAndOptionallyElaborate (it calls isTypeRelatedTo) for the entrypoint is that: we should know actual assignability (isRelated in my current code) to only report deprecation for finally selected overload signature.
Reporting from e.g. checkObjectLiteral / checkPropertyAssignment and checkJsxAttributes / checkJsxAttribute emits false-positive reports for every overload tried but not chosen by chooseOverload (I tried to implement but got that result 😭).

The another reason of my choice was: addDeprecatedSuggestion is called from getPropertyTypeForIndexType, which is not check* function.

Perhaps we can utilize logic similar to errorOutputContainer to stick with checkObjectLiteral and checkJsxAttributes, but it causes deep bucket brigade from checkExpression to every check* methods.

I also considered that calling reporting logic directly from checkApplicableSignatureForJsxOpeningLikeElement itself is cleaner, but it seems to require doubling type traversing logic (isRelatedTo) only for the deprecation report.

For more context:

In checkApplicableSignatureForJsxOpeningLikeElement, checkJsxAttributes is (indirectly) called from checkExpressionWithContextualType to retrieve type of written attributes (it returns Type), and that result will be passed to checkTypeRelatedToAndOptionallyElaborate to check assignability of the attributes vs the signature.

function checkApplicableSignatureForJsxOpeningLikeElement( node: JsxOpeningLikeElement, signature: Signature, relation: ESMap<string, RelationComparisonResult>, checkMode: CheckMode, reportErrors: boolean, containingMessageChain: (() => DiagnosticMessageChain | undefined) | undefined, errorOutputContainer: { errors?: Diagnostic[], skipLogging?: boolean } ) { // Stateless function components can have maximum of three arguments: "props", "context", and "updater". // However "context" and "updater" are implicit and can't be specify by users. Only the first parameter, props, // can be specified by users through attributes property. const paramType = getEffectiveFirstArgumentForJsxSignature(signature, node); const attributesType = checkExpressionWithContextualType(node.attributes, paramType, /*inferenceContext*/ undefined, checkMode); return checkTagNameDoesNotExpectTooManyArguments() && checkTypeRelatedToAndOptionallyElaborate( attributesType, paramType, ...

By doing good old printf debug in createJsxAttributesTypeFromAttributesProperty (body of checkJsxAttributes), stack trace is like this:

Error: debug at createJsxAttributesTypeFromAttributesProperty (src/compiler/checker.ts:27259:31) at checkJsxAttributes (src/compiler/checker.ts:27381:20) at checkExpressionWorker (src/compiler/checker.ts:33946:28) at checkExpression (src/compiler/checker.ts:33794:40) at checkExpressionWithContextualType (src/compiler/checker.ts:33407:30) at checkApplicableSignatureForJsxOpeningLikeElement (src/compiler/checker.ts:29296:36) at getSignatureApplicabilityError (src/compiler/checker.ts:29397:22) at chooseOverload (src/compiler/checker.ts:29987:25) at resolveCall (src/compiler/checker.ts:29814:26) at resolveJsxOpeningLikeElement (src/compiler/checker.ts:30687:20) at resolveSignature (src/compiler/checker.ts:30714:28) at getResolvedSignature (src/compiler/checker.ts:30737:28) at checkJsxOpeningLikeElementOrOpeningFragment (src/compiler/checker.ts:27728:29)

I think you can re-use most of the work with getResolvedSignature. See my other comment.

ypresto · 2021-10-13T14:33:11Z

Added test for function with overload.
https://github.com/microsoft/TypeScript/compare/d89d05d806208733638557924b080770a1069f1f..5a06bd3c736b2b34ade376986b234b0372472f17

sandersn

Would the code I propose below work?

sandersn · 2021-10-14T22:33:46Z

src/compiler/checker.ts

@@ -18441,6 +18456,27 @@ namespace ts {
                return prop.valueDeclaration && container.valueDeclaration && prop.valueDeclaration.parent === container.valueDeclaration;
            }

+            function checkDeprecatedProperties(source: FreshObjectLiteralType, target: Type) {


you should be able to call this from createJsxAttributesTypeFromAttributesProperty, inside the for (const attributeDecl of attributes.properties) loop.

Before the loop, save const sig = resolveSignature(openingLikeElement)

const arg = getTypeOfSymbol(sig.parameters[0]) (you should first check sig.parameters.length)

Inside the loop, checkDeprecatedProperty(member, getPropertyOfType(arg, member.name))

checkDeprecatedProperty is mostly the body of the for loop of checkDeprecatedProperties. I'm not sure where isExcessPropertyCheckTarget will need to go, probably before the loop in createJsxAttributesTypeFromAttributesProperty. It should report errors directly instead of storing them in a list.

createJsxAttributesTypeFromAttributesProperty() is called from resolvedSignature() itself (see stack trace in this comment #46167 (comment) ), so 1. in your suggestion causes an infinite recursion.

Actual stack trace when placing 1. code

Placing this right before loop in createJsxAttributesTypeFromAttributesProperty():

const sig = resolveSignature(openingLikeElement, undefined, checkMode ?? CheckMode.Normal);

caused below stack trace:

1) fourslash tests tests/cases/fourslash/jsdocDeprecated_suggestion16_jsxArguments.ts fourslash test jsdocDeprecated_suggestion16_jsxArguments.ts runs correctly: RangeError: Maximum call stack size exceeded at identifierIsThisKeyword (src/compiler/utilities.ts:4457:19) at isThisIdentifier (src/compiler/utilities.ts:4441:65) at Object.isThisInTypeQuery (src/compiler/utilities.ts:4445:14) at isMatchingReference (src/compiler/checker.ts:22547:28) at getTypeAtFlowAssignment (src/compiler/checker.ts:23708:21) at getTypeAtFlowNode (src/compiler/checker.ts:23626:32) at getFlowTypeOfReference (src/compiler/checker.ts:23578:53) at checkIdentifier (src/compiler/checker.ts:25002:30) at checkExpressionWorker (src/compiler/checker.ts:33838:28) at checkExpression (src/compiler/checker.ts:33781:40) at resolveJsxOpeningLikeElement (src/compiler/checker.ts:30657:31) at resolveSignature (src/compiler/checker.ts:30701:28) at createJsxAttributesTypeFromAttributesProperty (src/compiler/checker.ts:27225:25) at checkJsxAttributes (src/compiler/checker.ts:27368:20) at checkExpressionWorker (src/compiler/checker.ts:33933:28) at checkExpression (src/compiler/checker.ts:33781:40) at checkExpressionWithContextualType (src/compiler/checker.ts:33394:30) at checkApplicableSignatureForJsxOpeningLikeElement (src/compiler/checker.ts:29283:36) at getSignatureApplicabilityError (src/compiler/checker.ts:29384:22) at chooseOverload (src/compiler/checker.ts:29933:25) at resolveCall (src/compiler/checker.ts:29804:26) at resolveJsxOpeningLikeElement (src/compiler/checker.ts:30674:20) at resolveSignature (src/compiler/checker.ts:30701:28) at createJsxAttributesTypeFromAttributesProperty (src/compiler/checker.ts:27225:25) ...

In my understanding, call tree for resolving jsx element signature is roughly in below graph (some nodes are omitted):

checkJsxOpeningLikeElementOrOpeningFragment └getResolvedSignature └chooseOverload └checkApplicableSignatureForJsxOpeningLikeElement ├checkExpressionWithContextualType | └createJsxAttributesTypeFromAttributesProperty (create Type instance) └checkTypeRelatedToAndOptionallyElaborate └isTypeRelatedTo ("type check")

So we do not know final signature in createJsxAttributesTypeFromAttributesProperty.

How do you feel if I add new variable under createTypeChecker to store diagnostics while overload resolution? It may reduce bucket brigade in checkExpression*** call chain.

sandersn · 2021-10-14T22:41:43Z

src/compiler/checker.ts

@@ -16924,7 +16924,7 @@ namespace ts {
            containingMessageChain: (() => DiagnosticMessageChain | undefined) | undefined,
            errorOutputContainer: { errors?: Diagnostic[], skipLogging?: boolean } | undefined
        ): boolean {
-            if (isTypeRelatedTo(source, target, relation)) return true;
+            if (isTypeRelatedTo(source, target, relation, /*reportDeprecatedProperties*/ true)) return true;


I think you can re-use most of the work with getResolvedSignature. See my other comment.

sandersn

I tried moving this myself and ended up with commit a9d808b, which puts the call in checkObjectLiteral and checkJsxOpeningLikeElementOrOpeningFragment.

The downside is that the checkObjectLiteral is called once for each overload, so if any overload has a deprecated property, there will be a deprecated suggestion. As a result, I turned off reporting when the contextual type comes from an overload.

tests/cases/fourslash/jsdocDeprecated_suggestion16_jsxArguments.ts

tests/cases/fourslash/jsdocDeprecated_suggestion18_functionArgumentsWithOverloads.ts

tests/cases/fourslash/jsdocDeprecated_suggestion16_jsxArguments.ts

sandersn · 2021-11-03T15:53:36Z

tests/cases/fourslash/jsdocDeprecated_suggestion16_jsxArguments.ts

+//// <Component [|bool|] [|callback|]={() => {}} [|foo|]="foo" bar="bar" baz={{ [|foo|]: true }} />;
+
+//// // Skip spread in jsx.
+//// <Component {...{ foo: "foo", bar: "bar" }} />;


why skip spread in JSX?

(It was following same flow as excess property check which does not check for spread.)

sandersn · 2022-03-25T23:56:38Z

@ypresto Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs.

ypresto · 2022-04-10T20:13:06Z

Okay finally I made some time in next weekend. I'll fix conflicts and review comments. 🙏

…OpeningFragment It is now obvious that this only works when there's a contextual type for the object literal.

ypresto · 2022-05-16T18:56:47Z

I cherry-picked your a9d808b and added global variable of this comment:

How do you feel if I add new variable under createTypeChecker to store diagnostics while overload resolution? It may reduce bucket brigade in checkExpression*** call chain.

sandersn · 2025-04-14T16:03:47Z

Looking back at this PR, I feel like it makes too large and risky a change for the benefit of fixing its bug.
Also, Corsa changes the way that nested errors are computed, so merging this now is likely to cost even more time when it comes time to port it.

I think it's best to close this since it is now quite old.

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Oct 1, 2021

ypresto force-pushed the report-deprecation-object-literal branch from 3f3711f to d89d05d Compare October 2, 2021 05:54

andrewbranch requested a review from sandersn October 4, 2021 18:58

sandersn requested changes Oct 8, 2021

View reviewed changes

ypresto force-pushed the report-deprecation-object-literal branch from d89d05d to 5a06bd3 Compare October 13, 2021 14:31

sandersn requested changes Oct 14, 2021

View reviewed changes

sandersn requested changes Nov 3, 2021

View reviewed changes

sandersn self-assigned this Feb 19, 2022

ypresto and others added 4 commits May 16, 2022 05:15

Report deprecation for object literal assignments

2314140

Move reporting to checkObjectLiteral and checkJsxOpeningLikeElementOr…

fb52df2

…OpeningFragment It is now obvious that this only works when there's a contextual type for the object literal.

Fix error, fix lint

9d3bc7d

Report deprecation also for overloaded calls

e507dd3

ypresto force-pushed the report-deprecation-object-literal branch from 5a06bd3 to e507dd3 Compare May 16, 2022 18:18

Rename test files

60c62e1

ypresto requested a review from sandersn May 16, 2022 18:50

sandersn closed this Apr 14, 2025

github-project-automation bot moved this from Waiting on reviewers to Done in PR Backlog Apr 14, 2025

sandersn removed this from PR Backlog Apr 22, 2025

Report deprecation for object literal assignments (#39374) #46167

Report deprecation for object literal assignments (#39374) #46167

Uh oh!

Conversation

ypresto commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

ypresto Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

ypresto commented Oct 13, 2021

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

ypresto Oct 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandersn Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

ypresto May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Mar 25, 2022

Uh oh!

ypresto commented Apr 10, 2022

Uh oh!

ypresto commented May 16, 2022

Uh oh!

sandersn commented Apr 14, 2025

Uh oh!

Uh oh!

ypresto commented Oct 1, 2021 •

edited

Loading

ypresto Oct 13, 2021 •

edited

Loading

ypresto Oct 24, 2021 •

edited

Loading

ypresto May 18, 2022 •

edited

Loading