Improved readonly errors#22609
Open
NickSdot wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This surfaced via #22588 see #22588 (comment).
There are two commits. The second one is redoing the first one. Tim asked me in the linked comment to add
public(set)to areadonlyproperty. There are definitely other tests doing that, and I went ahead with it. Though, it seems very weird to me that we would have to usepublic(set)to override the implicitprotected(set)fromreadonly, just to then surface the actual error.I am not going to pretend I understand all the complexity involved here... But the current state of the PR seems more logical to me because we no longer need
public(set), and get actual helpful errors:instead of only changing:
to:
as the first commit did. This would make tests pass, but it's not honest because in combination with
readonlythe actual problem is not visibility. Not fully sure if it would be semantically defensible, but I think we could argue thatpublic(set)in combination withreadonlyitself, on a promoted property, never should have worked and should have errored -- because it achieves nothing. Anyway, too late to change that because it would be breaking.Fwiw, I know that a
setinterface property does not mean that the implementor must have a hook -- however, sincereadonlyis implicit protected, for a promoted property the right conclusion seems to be to acknowledge that in this case the error must be related to hooks. As mentioned above, changing visibility topublic(set)does not achieve anything other than reaching the actual error, and this will never change. However, it is possible that we will find a solution for allowing hooked readonly properties in the future -- which is another argument for having a hook related error, instead of a more generic one, until thenThat said, it's not impossible that I am missing something, but changing the order as I did here seems sound.
Aside: interface_get_set_readonly.phpt was renamed interface_readonly_get_set_promoted.phpt, not deleted
@iluuu1994 you added the "error message should be improved". Can I please have your opinion on this?