Better error message for accidental calls to get-accessors #37800

jtbandes · 2020-04-05T22:51:04Z

Fixes #24554 by adding a new Message when resolveCallExpression fails to find any call signatures, if the node.expression references a GetAccessor. Example test case with new message:

==== tests/cases/compiler/accessorAccidentalCallDiagnostic.ts (1 errors) ====
    // https://github.com/microsoft/TypeScript/issues/24554
    class Test24554 {
        get property(): number { return 1; }
    }
    function test24554(x: Test24554) {
        return x.property();
                 ~~~~~~~~
!!! error TS2349: This expression is not callable.
!!! error TS2349:   Type 'Number' has no call signatures.
!!! related TS6232 tests/cases/compiler/accessorAccidentalCallDiagnostic.ts:6:12: 'x.property' is a 'get' accessor; did you mean to use it without '()'?
!!! related TS2728 tests/cases/compiler/accessorAccidentalCallDiagnostic.ts:3:9: 'property' is declared here.
    }

This is my first attempt to contribute to TS, so I'm eager for any feedback! Specifically here are some questions I'd like someone familiar with the code to answer:

Was I right to just choose an available code in diagnosticMessages.json? Are they arranged in some grouping other than by code?
Is resolveCallExpression a good place to add this? The discussion on Clarify error message when an accessor is invoked. #27897 mentioned changing checkPropertyAccessExpression, but that seemed hard to me because the PropertyAccessExpression isn't really aware of its parent CallExpression (well, I guess it might be able to access parent, but that doesn't seem great—and there are some other diagnostics in this function already).
Any caveats to using getNodeLinks(node.expression).resolvedSymbol? That was the easiest way I found to reach the function and determine whether it's a GetAccessor, but it also seems to implicitly depend on something else having resolved the symbol first.
Is relatedInformation the right place to attach such a message? I'm a bit confused about the diagnostic "chain" and how relatedInfo fits in, but this seems to work.
I'd be interested in adding a code fix to delete the () as well. I started down that path, but it seems like fixes can't be triggered on relatedInformation, just on errors (I was able to create a new fix that got triggered on This_expression_is_not_callable errors, but it didn't trigger if I register it only for my new diagnostic message.) And since the fix's getCodeActions(context) function doesn't receive the diagnostic itself, just the code, it doesn't seem feasible to register it for all This_expression_is_not_callable errors and look at their relatedInformation to see if my new did you mean to use it without '()'? is attached. One approach I could consider is making it a separate error, but then it wouldn't be attached to the existing "not callable" error. Any advice here?

msftclas · 2020-04-05T22:51:16Z

All CLA requirements met.

DanielRosenwasser · 2020-04-09T18:32:54Z

@typescript-bot pack this

typescript-bot · 2020-04-09T18:32:58Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at f92edb7. You can monitor the build here.

src/compiler/checker.ts

DanielRosenwasser · 2020-04-09T18:38:03Z

src/compiler/diagnosticMessages.json

@@ -4384,6 +4384,10 @@
        "category": "Error",
        "code": 6231
    },
+    "'{0}' is a 'get' accessor; did you mean to use it without '()'?": {


Suggested change

"'{0}' is a 'get' accessor; did you mean to use it without '()'?": {

"'{0}' is declared as a 'get' accessor and does not need to be called.": {

Actually, trying this out in an editor, I could see either one being good. It's up to you.

I could see the wording that mentions () being better if we could actually provide a code fix that deletes the (). I had some questions about that in my PR description, would you happen to have experience with code fixes and could help me figure out how to implement it cleanly?

With or without the codefix, I like the () message better since it shows what's wrong instead of naming it in terms that not everyone understands immediately.

re codefixes: There are some simple ones that you can basically copy, then you just have to learn how to extract the property access from the call, which should also be easy.

However, I'd rather just take the error alone and follow up with the codefix.

Actually, as a replacement for 'this expression is not callable', we should keep some elements of that:

Suggested change

"'{0}' is a 'get' accessor; did you mean to use it without '()'?": {

"This expression is not callable because it is a 'get' accessor; did you mean to use it without '()'?": {

(You already have an error span so I don't think you need to reproduce the expression in the error -- long expressions result in too-long errors.)

typescript-bot · 2020-04-09T18:49:54Z

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/70872/artifacts?artifactName=tgz&fileId=2EE3DBFF97B39DAEC967A7C041F96C371B613531C1CA0C3AE234ED7991E41E6102&fileName=/typescript-3.9.0-insiders.20200409.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

sandersn

This error should replace "This expression is not callable", or at least be added as an elaboration.

sandersn · 2020-04-15T15:15:19Z

src/compiler/diagnosticMessages.json

@@ -4384,6 +4384,10 @@
        "category": "Error",
        "code": 6231
    },
+    "'{0}' is a 'get' accessor; did you mean to use it without '()'?": {


With or without the codefix, I like the () message better since it shows what's wrong instead of naming it in terms that not everyone understands immediately.

sandersn · 2020-04-15T15:17:21Z

src/compiler/diagnosticMessages.json

@@ -4384,6 +4384,10 @@
        "category": "Error",
        "code": 6231
    },
+    "'{0}' is a 'get' accessor; did you mean to use it without '()'?": {


re codefixes: There are some simple ones that you can basically copy, then you just have to learn how to extract the property access from the call, which should also be easy.

However, I'd rather just take the error alone and follow up with the codefix.

sandersn · 2020-04-15T15:20:57Z

tests/baselines/reference/accessorAccidentalCallDiagnostic.errors.txt

+    function test24554(x: Test24554) {
+        return x.property();
+                 ~~~~~~~~
+!!! error TS2349: This expression is not callable.


I would much rather replace this message instead of tacking on related info.

If it's too hard to replace the message, it should be an elaboration, not related info. Elaborations provide additional messages; related info is designed to point to a different location.

sandersn · 2020-04-15T15:23:26Z

src/compiler/diagnosticMessages.json

@@ -4384,6 +4384,10 @@
        "category": "Error",
        "code": 6231
    },
+    "'{0}' is a 'get' accessor; did you mean to use it without '()'?": {


Actually, as a replacement for 'this expression is not callable', we should keep some elements of that:

Suggested change

"'{0}' is a 'get' accessor; did you mean to use it without '()'?": {

"This expression is not callable because it is a 'get' accessor; did you mean to use it without '()'?": {

(You already have an error span so I don't think you need to reproduce the expression in the error -- long expressions result in too-long errors.)

sandersn · 2020-04-15T15:24:55Z

src/services/codefixes/fixForgottenThisPropertyAccess.ts is a good, simple codefix to start from. But it requires a unique error code, which means replacing "This expression is not callable".

sandersn · 2020-04-15T16:57:09Z

(Sorry, I didn't read your description until just now -- you asked about every single one of the aspects I commented on.)

jtbandes · 2020-04-15T22:05:42Z

Thanks for the feedback!

But it requires a unique error code, which means replacing "This expression is not callable".

That partially answers my question about the best way to make the code fix. But is it really desirable to replace this error message? I guess I'm worried about the rare cases where the remove () suggestion is wrong, and then the new error message wouldn't really make sense. Should we try to also preserve the "not callable" error (the actual "root cause"), or do you really think it's best to replace it?

jtbandes · 2020-04-15T22:07:44Z

I see, I guess starting with "This expression is not callable because…" alleviates my concern. And I suppose we can still keep the "Type {0} has no call signatures" sub-error.

jtbandes · 2020-04-19T06:10:47Z

I added a code fix, but to make it easier I modified the parser to save the open/close paren tokens in CallExpression. Is that alright?

~~Edit: guess it's gonna take a lot more work to fix all the tests 😬~~

Edit 2: going to remove that from this PR and do it separtely.

jtbandes · 2020-04-19T23:17:13Z

Alright, got the code fix working, except that I had to omit the (/) tokens from forEachChild and change the test harness 😕 : https://github.com/jtbandes/TypeScript/pull/1/files#diff-e4047651bbe0a47423c7db850216a6afR122

jtbandes · 2020-04-22T19:41:39Z

Hi @sandersn just a friendly ping on this PR! No rush but I don't want it to languish for too long either :)

tests/baselines/reference/instancePropertyInClassType.errors.txt

sandersn · 2020-04-23T00:08:39Z

tests/baselines/reference/instancePropertyInClassType.errors.txt

-!!! error TS2349: This expression is not callable.
-!!! error TS2349:   Type 'Number' has no call signatures.
+!!! message TS6232: This expression is not callable because it is a 'get' accessor. Did you mean to use it without '()'?
+!!! related TS2728 tests/cases/conformance/classes/members/classTypes/instancePropertyInClassType.ts:4:13: 'y' is declared here.


I'm not convinced this is valuable. If it is, it should really be on the base "This expression is not callable." error as well. @DanielRosenwasser can you decide on this? I trust your opinion on related spans.

sandersn · 2020-04-23T00:13:28Z

src/compiler/checker.ts

+                // Diagnose get accessors incorrectly called as functions
+                const { resolvedSymbol } = getNodeLinks(errorTarget);
+                if (resolvedSymbol && resolvedSymbol.flags & SymbolFlags.GetAccessor) {
+                    diagnostic = createDiagnosticForNode(errorTarget, Diagnostics.This_expression_is_not_callable_because_it_is_a_get_accessor_Did_you_mean_to_use_it_without);


I think the change should be in invocationErrorDetails, very close to the base "Type_0_has_no_call_signatures", instead of out here in invocationError.

invocationErrorDetails currently doesn't have enough information (access to the node/symbol) to produce the new error. I could rethink its signature, but that seemed more invasive than just putting it here. I'll take another look later if you think it's better that way!

…al-accessor-call

jtbandes · 2020-04-30T06:08:37Z

Alright, I've moved the changes into invocationErrorDetails and removed the related info. (Considered keeping it but the code flow around chains vs related info is already convoluted and I didn't want to make it worse.) Let me know what you think!

jtbandes · 2020-05-06T00:47:47Z

Ping @sandersn 🙌 (Not really in a hurry, I'm sure you are quite busy — just want to avoid falling through the cracks!)

sandersn

Sorry, noticed one more thing. (This might be the fault of my merge.)

sandersn · 2020-05-20T23:48:32Z

src/compiler/diagnosticMessages.json

@@ -4416,6 +4416,10 @@
        "category": "Error",
        "code": 6233
    },
+    "This expression is not callable because it is a 'get' accessor. Did you mean to use it without '()'?": {
+        "category": "Message",


Should be an Error, not a Message.

sandersn · 2020-05-21T00:17:08Z

Travis is all green when I click through to it, so I'm going to go ahead and merge.

jtbandes · 2020-05-21T00:49:57Z

Great, thanks! I will send out another PR for the code fix in the near future.

jtbandes mentioned this pull request Apr 9, 2020

Can we give better error messages on invoked accessors? #24554

Closed

DanielRosenwasser requested changes Apr 9, 2020

View reviewed changes

jtbandes added 2 commits April 11, 2020 15:52

Better error message for accidental calls to get-accessors

61f6005

Add _0_is_declared_here pointing to accessor declaration

d00a5c9

jtbandes force-pushed the diagnose-accidental-accessor-call branch from f92edb7 to d00a5c9 Compare April 11, 2020 23:02

sandersn requested changes Apr 15, 2020

View reviewed changes

sandersn self-assigned this Apr 15, 2020

sandersn added the For Backlog Bug PRs that fix a backlog bug label Apr 15, 2020

replace the original not-callable error

d00f2b5

jtbandes force-pushed the diagnose-accidental-accessor-call branch from aa5838c to cd15560 Compare April 19, 2020 06:09

jtbandes force-pushed the diagnose-accidental-accessor-call branch from a4322d1 to d00f2b5 Compare April 19, 2020 22:15

jtbandes mentioned this pull request Apr 19, 2020

Code fix for accidental accessor calls jtbandes/TypeScript#1

Closed

jtbandes requested a review from sandersn April 19, 2020 23:17

sandersn requested changes Apr 23, 2020

View reviewed changes

jtbandes added 3 commits April 29, 2020 22:58

move to invocationErrorDetails

6051fc1

Merge remote-tracking branch 'upstream/master' into diagnose-accident…

fd4ecce

…al-accessor-call

fix order and tests

0b1cb74

jtbandes requested a review from sandersn April 30, 2020 06:07

jtbandes requested a review from DanielRosenwasser May 13, 2020 23:10

sandersn approved these changes May 20, 2020

View reviewed changes

Merge branch 'master' into diagnose-accidental-accessor-call

fc83d10

sandersn requested changes May 20, 2020

View reviewed changes

Make new error an error, not message

0b53172

sandersn approved these changes May 20, 2020

View reviewed changes

sandersn merged commit 45cf20c into microsoft:master May 21, 2020

jtbandes mentioned this pull request May 23, 2020

Code fix for accidental calls to get-accessors #38749

Merged

jtbandes deleted the diagnose-accidental-accessor-call branch May 23, 2020 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error message for accidental calls to get-accessors #37800

Better error message for accidental calls to get-accessors #37800

jtbandes commented Apr 5, 2020 •

edited

Loading

msftclas commented Apr 5, 2020 •

edited

Loading

DanielRosenwasser commented Apr 9, 2020

typescript-bot commented Apr 9, 2020 •

edited

Loading

DanielRosenwasser Apr 9, 2020

DanielRosenwasser Apr 9, 2020

jtbandes Apr 9, 2020

sandersn Apr 15, 2020

sandersn Apr 15, 2020

sandersn Apr 15, 2020 •

edited

Loading

typescript-bot commented Apr 9, 2020 •

edited

Loading

sandersn left a comment

sandersn Apr 15, 2020

sandersn Apr 15, 2020

sandersn Apr 15, 2020

sandersn Apr 15, 2020 •

edited

Loading

sandersn commented Apr 15, 2020

sandersn commented Apr 15, 2020

jtbandes commented Apr 15, 2020

jtbandes commented Apr 15, 2020

jtbandes commented Apr 19, 2020 •

edited

Loading

jtbandes commented Apr 19, 2020

jtbandes commented Apr 22, 2020

sandersn Apr 23, 2020

sandersn Apr 23, 2020

jtbandes Apr 23, 2020

jtbandes commented Apr 30, 2020

jtbandes commented May 6, 2020

sandersn left a comment

sandersn May 20, 2020

sandersn commented May 21, 2020

jtbandes commented May 21, 2020

	"'{0}' is a 'get' accessor; did you mean to use it without '()'?": {
	"'{0}' is declared as a 'get' accessor and does not need to be called.": {

	"'{0}' is a 'get' accessor; did you mean to use it without '()'?": {
	"This expression is not callable because it is a 'get' accessor; did you mean to use it without '()'?": {

Better error message for accidental calls to get-accessors #37800

Better error message for accidental calls to get-accessors #37800

Conversation

jtbandes commented Apr 5, 2020 • edited Loading

msftclas commented Apr 5, 2020 • edited Loading

DanielRosenwasser commented Apr 9, 2020

typescript-bot commented Apr 9, 2020 • edited Loading

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 Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

typescript-bot commented Apr 9, 2020 • edited Loading

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

sandersn Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

sandersn commented Apr 15, 2020

sandersn commented Apr 15, 2020

jtbandes commented Apr 15, 2020

jtbandes commented Apr 15, 2020

jtbandes commented Apr 19, 2020 • edited Loading

jtbandes commented Apr 19, 2020

jtbandes commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtbandes commented Apr 30, 2020

jtbandes commented May 6, 2020

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented May 21, 2020

jtbandes commented May 21, 2020

jtbandes commented Apr 5, 2020 •

edited

Loading

msftclas commented Apr 5, 2020 •

edited

Loading

typescript-bot commented Apr 9, 2020 •

edited

Loading

sandersn Apr 15, 2020 •

edited

Loading

typescript-bot commented Apr 9, 2020 •

edited

Loading

sandersn Apr 15, 2020 •

edited

Loading

jtbandes commented Apr 19, 2020 •

edited

Loading