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

fix #11480, disallow delete operator on readonly property or index signature #11990

Merged
merged 6 commits into from
Dec 26, 2016

Conversation

HerringtonDarkholme
Copy link
Contributor

@HerringtonDarkholme HerringtonDarkholme commented Nov 2, 2016

Fixes #11480

It changed more tests than I thought. Should we check delete operator only if it is a property access or indexer?

@RyanCavanaugh
Copy link
Member

Looks good to me. Pretty stupid that we allowed delete x; before

@HerringtonDarkholme
Copy link
Contributor Author

Sorry, I didn't catch your point.

Do you mean "check readonly property only when delete property access or indexer" is good or "disallow delete anything other than property access or indexer"?

For the latter, I think it would break namespace.

namespace A {
  export var a = 123;
  delete a;
}

And it would break unintentional code like

// in other file like html
window.globalProperty = 123
// in ts
declare var globalProperty
delete globalPropery

@RyanCavanaugh
Copy link
Member

I believe both of those should be errors for the sake of clarity (unless there are some compelling use cases from real code to bring up).

In the namespace case, it would be very confusing because the behavior would have to change (to become an error) depending on whether you wrote export (since non-exported vars don't become properties). Writing delete A.a is much clearer.

In the global case, we often require globals to be prefixed by window. anyway, which seems like the correct thing to do here as well. And since we'd have to resolve this differently in modules vs non-modules anyway, it's also confusing in that regard.

@HerringtonDarkholme
Copy link
Contributor Author

@ahejlsberg I found resolvedIndexInfo is removed in #11929 . Is it intended?
I need indexInfo in checkDeleteExpression, otherwise I need to handle delete in getPropertyTypeForIndexType

@mhegazy
Copy link
Contributor

mhegazy commented Dec 22, 2016

@HerringtonDarkholme can you refresh the PR.

@HerringtonDarkholme
Copy link
Contributor Author

HerringtonDarkholme commented Dec 23, 2016

@dotnet-bot test ci please

Hmmm, it seems node.6 CI breaks due to mismatched shasum check.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 26, 2016

@dotnet-bot test ci please

@mhegazy mhegazy closed this Dec 26, 2016
@mhegazy mhegazy reopened this Dec 26, 2016
@mhegazy mhegazy merged commit 0b125a3 into microsoft:master Dec 26, 2016
@HerringtonDarkholme HerringtonDarkholme deleted the delete-readonly branch January 8, 2017 10:11
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants