Skip to content

Report error on method name for chained method calls #31414

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
merged 5 commits into from
Jun 22, 2019
Merged

Report error on method name for chained method calls #31414

merged 5 commits into from
Jun 22, 2019

Conversation

dhruvrajvanshi
Copy link
Contributor

Fixes #31365

This is a first stab at fixing the issue. It reports the error at the name of the final method call.

Ideally, the error would be reported at the argument list span for the call but I couldn't find a straightforward way to get a span for the argument list. I'm willing to the PR if we really need reporting at the argument list.

@@ -28,19 +28,19 @@ tests/cases/conformance/expressions/functionCalls/callWithMissingVoid.ts(75,1):

declare const xAny: X<any>;
xAny.f() // error, any still expects an argument
~~~~~~~~
~
Copy link
Member

Choose a reason for hiding this comment

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

I think the idea was that the argument list was supposed to be included in the range.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, just saw your comment in the PR.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 15, 2019

Spoke with @RyanCavanaugh - we think just erroring on the name itself is fine, but we'd like to error on the invoked expression itself even when it's not a property access. In other words, if you don't have a property access as the target, use node.expression for the error span.

@dhruvrajvanshi
Copy link
Contributor Author

@DanielRosenwasser I'm not quite sure I understand.
Do you mean something like this?

function getCallErrorNode(node: CallLikeExpression): Node {
        if (isCallExpression(node)) {
            if (isPropertyAccessExpression(node.expression)) {
                return node.expression.name;
            } else {
                return node.expression
            }
        }
                
        return node;
}

Because this is breaking a lot of error messages (and they look worse IMO).

Ideally, the error should include the final method name and its argument list which could be done by including a span in NodeArray.

@DanielRosenwasser
Copy link
Member

Yeah, something like that code.

Because this is breaking a lot of error messages (and they look worse IMO).

Maybe we'd do this for only Identifier nodes. What looks grosser?

@dhruvrajvanshi
Copy link
Contributor Author

I've updated the PR so you can see for yourself. Most call argument arity errors exclude the argument list. It is more uniform with the new change though.

Is there a way to report diagnostics on spans instead of nodes?

@DanielRosenwasser
Copy link
Member

I think this is a great first step. If we want to iterate further, we'll take another PR.

@DanielRosenwasser DanielRosenwasser merged commit 2c458c0 into microsoft:master Jun 22, 2019
dragomirtitian added a commit to dragomirtitian/TypeScript4ExtJS that referenced this pull request Jun 28, 2019
1. When calling a non-callable expression the error span is on the call target not on the whole call
2. When calling a method, the error for overload resolution now includes the arguments (this was previously regressed by microsoft#31414)
dragomirtitian added a commit to dragomirtitian/TypeScript4ExtJS that referenced this pull request Jun 28, 2019
1. When calling a non-callable expression the error span is on the call target not on the whole call
2. When calling a method, the error for overload resolution now includes the arguments (this was previously regressed by microsoft#31414)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error messages for method chains
2 participants