Skip to content

[Diagnostics] Improve diagnostic for defaulted accessor in a protocol property. #35230

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 24, 2021
Merged

[Diagnostics] Improve diagnostic for defaulted accessor in a protocol property. #35230

merged 1 commit into from
Feb 24, 2021

Conversation

mininny
Copy link
Contributor

@mininny mininny commented Dec 27, 2020

Code example:

protocol P {
    var x : Int {
        get { 0 }
        //  ^ error: expected get or set in a protocol property
    }
}

we should provide a clearer diagnostic saying that you cannot provide a default implementation for a getter in a protocol, or something along those lines.

This PR adds a diagnostics that says protocol property cannot have a default implementation of {ACCESSOR_TYPE} when a body is provided to a computed property in protocol.

Resolves SR-13963.

@mininny mininny force-pushed the default-accessor-error-in-protocol branch from 6ba0975 to aa408f1 Compare December 28, 2020 15:59
LucianoPAlmeida
LucianoPAlmeida reviewed Dec 28, 2020
View reviewed changes
test/decl/protocol/invalid_accessors_implementation.swift Outdated
//===---

protocol Protocol1 {
var a: Int { get { 0 } } // expected-error {{protocol property cannot have a default implementation of getter}}
Copy link
Contributor

@LucianoPAlmeida LucianoPAlmeida Dec 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it wouldn't make sense(or be possible) to offer a fix-it to remove from lbrace to rbrace e.g. { 0 } in this case? This is more a question as I'm just thinking if it would be a good thing for the user also...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think it makes sense to offer a fix-it, but I'm not sure how I can get the SourceRange from the lbrace to rbrace without disrupting the ongoing parsing.. I'd really appreciate any feedbacks or pointers on how I could do that! :)

Copy link
Member

Choose a reason for hiding this comment

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

I prefer not to offer a fix-it for removing. We should be very cautious about deleting user's code.

@xedin xedin requested a review from rintaro January 4, 2021 17:07
@mininny
Copy link
Contributor Author

mininny commented Jan 22, 2021

There hasn't been any progress in this PR for a while. Is there anything I can do to move this forward? Not sure whom I should ping..

@rintaro
Copy link
Member

rintaro commented Jan 22, 2021

Ah sorry, this just slipped off my head. I will review this this weekend.

@rintaro rintaro self-assigned this Jan 22, 2021
rintaro
rintaro reviewed Jan 25, 2021
View reviewed changes
lib/Parse/ParseDecl.cpp Outdated
@@ -5759,7 +5759,11 @@ ParserStatus Parser::parseGetSet(ParseDeclOptions Flags,

// parsingLimitedSyntax mode cannot have a body.
if (parsingLimitedSyntax) {
diagnose(Tok, diag::expected_getset_in_protocol);
// No error should be generated on implicitly created getter.
auto IsImplicitGetter = Kind == AccessorKind::Get && Loc.isInvalid();
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is always true since this is in NotAccessor = parseAccessorIntroducer() branch.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test case for "implicit getter" in a protocol? e.g.:

protocol ProtocolXXX {
  var a: Int { return 1 }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I removed unnecessary IsImplicitGetter and added a test case for implicit getter :)

@rintaro rintaro removed their assignment Jan 25, 2021
@mininny mininny force-pushed the default-accessor-error-in-protocol branch from aa408f1 to dde7aa8 Compare January 30, 2021 03:49
xwu
xwu reviewed Jan 30, 2021
View reviewed changes
include/swift/AST/DiagnosticsParse.def Outdated
@@ -248,6 +248,8 @@ ERROR(computed_property_no_accessors, none,
"%select{computed property|subscript}0 must have accessors specified", (bool))
ERROR(expected_getset_in_protocol,none,
"expected get or set in a protocol property", ())
ERROR(unexpected_getset_implementation_in_protocol,none,
"protocol property cannot have a default implementation of %0", (StringRef))
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about a slight rewording so that what we're referring to (the property getter/setter) isn't split between the beginning and end of the sentence, and to clarify that the error is not because a default implementation isn't ever allowed, but not allowed here:

Suggested change
"protocol property cannot have a default implementation of %0", (StringRef))
"protocol property %0 cannot have a default implementation specified here; use an extension instead", (StringRef))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! changed it :)

@mininny mininny force-pushed the default-accessor-error-in-protocol branch from dde7aa8 to 000c6ed Compare February 7, 2021 04:25
@xedin xedin requested a review from rintaro February 9, 2021 17:13
rintaro
rintaro reviewed Feb 11, 2021
View reviewed changes
lib/Parse/ParseDecl.cpp Outdated
Comment on lines 5761 to 5763
if (parsingLimitedSyntax) {
diagnose(Tok, diag::expected_getset_in_protocol);
Invalid = true;
Copy link
Member

Choose a reason for hiding this comment

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

I feel uncomfortable with marking Invalid without emitting diagnostics. Could you just leave this diagnostics as is for now? I know

protocol Protocol9 {
   var a: Int { return 0 }
 }

results kind of duplicated errors (expected get or set in a protocol property and property in protocol must have explicit { get } or { get set } specifier) But still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I reverted the change :)

@mininny mininny force-pushed the default-accessor-error-in-protocol branch from 000c6ed to 518e1b0 Compare February 12, 2021 13:40
@rintaro
Copy link
Member

rintaro commented Feb 20, 2021

@swift-ci Please smoke test

2 similar comments
@rintaro
Copy link
Member

rintaro commented Feb 22, 2021

@swift-ci Please smoke test

@rintaro
Copy link
Member

rintaro commented Feb 23, 2021

@swift-ci Please smoke test

@rintaro
Copy link
Member

rintaro commented Feb 24, 2021

@swift-ci Please smoke test Windows

@rintaro rintaro merged commit 26aa91e into swiftlang:main Feb 24, 2021
@rintaro
Copy link
Member

rintaro commented Feb 24, 2021

Thank you Minhyuk!

@xwu xwu mentioned this pull request Feb 27, 2021
Merged
@typesanitizer typesanitizer mentioned this pull request Aug 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xwu xwu xwu left review comments

@rintaro rintaro rintaro left review comments

+1 more reviewer

@LucianoPAlmeida LucianoPAlmeida LucianoPAlmeida left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mininny @rintaro @xwu @LucianoPAlmeida