Skip to content
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

Disallow expression with type parameters as left side of property access #49464

Merged
merged 8 commits into from
Jun 24, 2022

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jun 9, 2022

Fixes #49293

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jun 9, 2022
src/compiler/parser.ts Outdated Show resolved Hide resolved
@jakebailey jakebailey changed the title Return early from parsing property access expression after type parameters Disallow expression with type parameters as left side of property access Jun 22, 2022
@jakebailey
Copy link
Member Author

This PR now uses the grammar check functionality in the checker rather than the parser as we discussed in the design meeting, however, I don't think that this is working how I'd desire.

grammarErrorOnNode only reports the error if the file has no other parser errors, so a number of our tests don't actually error, while they do error for other illegal constructs like a<b>[...]. See: 728c9b4 (#49464)

@jakebailey jakebailey marked this pull request as ready for review June 22, 2022 22:03
@@ -1493,6 +1493,10 @@
"category": "Message",
"code": 1476
},
"Instantiation expression cannot be followed by property access.": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this instead complain on the type arguments and say they should be removed? And quickfix?

@jakebailey
Copy link
Member Author

I made the change I prefer in #49645 to show the difference. See: 2925fcf (#49645)

@sandersn
Copy link
Member

Can you remind me why we decided to make this a grammar check?

@jakebailey
Copy link
Member Author

This was the suggestion by @ahejlsberg, as a potentially better recovery method. But, I think #49645's method here is better; the tree is the same in both cases, but it correctly fires when it's done in the parser (whereas grammar checks do not always fire).

@jakebailey
Copy link
Member Author

Updated to be in the parser, and to move the error message onto the type parameters.

@jakebailey jakebailey merged commit 569cdf1 into microsoft:main Jun 24, 2022
@jakebailey jakebailey deleted the fix-49293 branch June 24, 2022 22:54
@jakebailey
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.7

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 27, 2022

Heya @jakebailey, I've started to run the task to cherry-pick this into release-4.7 on this PR at db3e062. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.7 manually.

jakebailey added a commit to jakebailey/TypeScript that referenced this pull request Jun 27, 2022
@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Different behavior for a?.b<c>.d depending on the compilation target
4 participants