Skip to content

Code fix for accidental calls to get-accessors #38749

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 10 commits into from
Jun 18, 2020

Conversation

jtbandes
Copy link
Contributor

Backlog bug: #24554
This is a follow-up to #37800 which adds a code fix associated with the new error message.
cc @sandersn for review :)

The cleanest way I found to implement this was updating the parser to keep references to the open/close paren tokens. Unfortunately I wasn't able to make forEachChild handle these properly, (see commit history in this branch) so I just skip these nodes. Let me know if there's a better way!

 class Test24554 {
     get property(): number { return 1; }
 }
 function test24554(x: Test24554) {
-    return x.property();
+    return x.property;
 }
 function test_2(x: { y: Test24554 }) {
-    return x.y.property ( /* bye */ );
+    return x.y.property;
 }

Copy link

@SmolinPavel SmolinPavel left a comment

Choose a reason for hiding this comment

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

🥇

@jtbandes jtbandes force-pushed the fix-accidental-accessor-call branch from 2570825 to 7b9d7f0 Compare May 29, 2020 04:57
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good once the whitespace in diagnosticMessages.json is back to normal.

"category": "Error",
"code": 2422
},
"Class '{0}' defines instan
Copy link
Member

Choose a reason for hiding this comment

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

looks like whitespace changed in this file. Can you undo that change please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems like a recurring problem, at least for me. I fixed this merge conflict through the GitHub UI and this is what I got. Do you know why this might be happening? (Windows line endings for example?) I wonder if there's a way it can be resolved on master such that it stops happening.

In the meantime, I'll fix it on this branch shortly...

Copy link
Member

Choose a reason for hiding this comment

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

What editor do you use? Do you have prettier or other formatting configured? The linux+emacs and macos+code people don't have this problem as far as I'm aware, although it does have Windows line endings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I guess it's possible that the problem only happens in the GitHub merge UI. I didn't keep careful track of when it happened and when it didn't. Anyway, fixed now :)

@jtbandes jtbandes requested a review from sandersn June 18, 2020 19:08
@sandersn sandersn merged commit 8136047 into microsoft:master Jun 18, 2020
@jtbandes jtbandes deleted the fix-accidental-accessor-call branch June 18, 2020 21:00
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.

4 participants