Skip to content

Add error message for set accessor visibility issues#33683

Merged
weswigham merged 1 commit into
microsoft:masterfrom
weswigham:accessor-declaration-emit-crash
Sep 30, 2019
Merged

Add error message for set accessor visibility issues#33683
weswigham merged 1 commit into
microsoft:masterfrom
weswigham:accessor-declaration-emit-crash

Conversation

@weswigham
Copy link
Copy Markdown
Member

@weswigham weswigham commented Sep 30, 2019

This fixes the crash in RWC.

sandersn
sandersn approved these changes Sep 30, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Code looks good but it would be nice to have tests for all 3 new errors.

Comment thread tests/baselines/reference/accessorDeclarationEmitVisibilityErrors.errors.txt
==== tests/cases/compiler/accessorDeclarationEmitVisibilityErrors.ts (2 errors) ====
export class Q {
set bet(arg: DoesNotExist) {}
~~~~~~~~~~~~
Copy link
Copy Markdown
Member

@sandersn sandersn Sep 30, 2019

Choose a reason for hiding this comment

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

names that don't exist create a "is using private name" errors? ok..

Copy link
Copy Markdown
Member Author

@weswigham weswigham Sep 30, 2019

Choose a reason for hiding this comment

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

Eeeeyup.

@weswigham
Copy link
Copy Markdown
Member Author

weswigham commented Sep 30, 2019

Code looks good but it would be nice to have tests for all 3 new errors.

Yeah, unfortunately, the other two are there to cover the space of errors allowed by the type declarations for accessibility errors, but I don't think they're possible to ever trigger as, at least right now, a setter cannot have inferred types. We have a lot of declaration emit errors like that, though (since we started serializing non-exported declarations if they're referenced by exported ones), so that's not so bad.

@weswigham weswigham merged commit 98cf317 into microsoft:master Sep 30, 2019
@weswigham weswigham deleted the accessor-declaration-emit-crash branch September 30, 2019 20:12
@weswigham
Copy link
Copy Markdown
Member Author

weswigham commented Sep 30, 2019

And now to wait for the next rwc diff on master, which'll include a ton of set/get declarations ❤️

@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersn sandersn sandersn approved these changes

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

Assignees

@weswigham weswigham

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@weswigham @sandersn