Skip to content

[SR-8340]Improve fix-it for var and subscript in Protocol #19660

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

Merged
merged 10 commits into from
Oct 4, 2018
Merged

[SR-8340]Improve fix-it for var and subscript in Protocol #19660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
67bf1a9
[Parser] Improve fix-it for subscription in protocol
mzp Sep 24, 2018
ee4f9b2
[Sema] Add fix-it for property in protocol
mzp Sep 24, 2018
f0f33eb
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
7d35c95
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
f1d830a
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
9ca89e4
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
7ae9eb7
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
da9a19b
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
1ddfa58
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
fd5f3ff
fixup! [Sema] Add fix-it for property in protocol
mzp Oct 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6226,7 +6226,7 @@ Parser::parseDeclSubscript(ParseDeclOptions Flags,
if (!Status.isError()) {
if (Flags.contains(PD_InProtocol)) {
diagnose(Tok, diag::expected_lbrace_subscript_protocol)
.fixItInsertAfter(ElementTy.get()->getEndLoc(), " { get set }");
.fixItInsertAfter(ElementTy.get()->getEndLoc(), " { get <#set#> }");
Copy link
Member

Choose a reason for hiding this comment

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

Generally, editor place holder (<#...#>) doesn't mean it's optional. Instead, we expect developers to replace it with actual code. But I admit it's handy in Xcode. (tab navigation, return key inserts set, single del key removes the placeholder).

@jrose-apple Any concern about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I found it a little funny too, but I think it's reasonable. @nkcsgexi might also be a good person to ask.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems reasonable to me as well. One benefit of placeholder is being easy to remove or being made solid, which i believe is @mzp 's motivation here to include set as a placeholder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the <#set#> was my idea, and that was my motivation.
I made SR-8340.

} else {
diagnose(Tok, diag::expected_lbrace_subscript);
}
Expand Down
18 changes: 14 additions & 4 deletions lib/Sema/CodeSynthesis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2023,11 +2023,21 @@ void swift::maybeAddAccessorsToStorage(TypeChecker &TC,
} else if (isa<ProtocolDecl>(dc)) {
if (storage->hasStorage()) {
auto var = cast<VarDecl>(storage);
if (var->isLet())

if (var->isLet()) {
TC.diagnose(var->getLoc(),
diag::protocol_property_must_be_computed_var);
else
TC.diagnose(var->getLoc(), diag::protocol_property_must_be_computed);
diag::protocol_property_must_be_computed_var)
.fixItReplace(var->getParentPatternBinding()->getLoc(), "var")
.fixItInsertAfter(var->getTypeLoc().getLoc(), " { get }");
} else {
auto diag = TC.diagnose(var->getLoc(), diag::protocol_property_must_be_computed);
auto braces = var->getBracesRange();

if (braces.isValid())
diag.fixItReplace(braces, "{ get <#set#> }");
else
diag.fixItInsertAfter(var->getTypeLoc().getLoc(), " { get <#set#> }");
Copy link
Member

Choose a reason for hiding this comment

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

Do we have test cases without type annotation? like:

protocol P {
   let foo
   var bar
}

If not, could you add them?
In these cases, ->getTypeLoc().getLoc() is invalid, so this fix-it should be ignored.
Just in case.

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 think it's OK to remove if (braces.isValid()) block here.

Your pointed-out is correct. I remove it at 1ddfa58.

Do we have test cases without type annotation?

I could not find such testcase. So, I added it in fd5f3ff.

}
}

setProtocolStorageImpl(TC, storage);
Expand Down
15 changes: 9 additions & 6 deletions test/decl/protocol/protocols.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,18 @@ protocol Test {
var creator: String { get }
var major : Int { get }
var minor : Int { get }
var subminor : Int // expected-error {{property in protocol must have explicit { get } or { get set } specifier}}
static var staticProperty: Int // expected-error{{property in protocol must have explicit { get } or { get set } specifier}}
var subminor : Int // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{21-21= { get <#set#> \}}}
static var staticProperty: Int // expected-error{{property in protocol must have explicit { get } or { get set } specifier}} {{33-33= { get <#set#> \}}}

let bugfix // expected-error {{type annotation missing in pattern}} expected-error {{immutable property requirement must be declared as 'var' with a '{ get }' specifier}}
var comment // expected-error {{type annotation missing in pattern}} expected-error {{property in protocol must have explicit { get } or { get set } specifier}}
}

protocol Test2 {
var property: Int { get }

var title: String = "The Art of War" { get } // expected-error{{initial value is not allowed here}} expected-error {{property in protocol must have explicit { get } or { get set } specifier}}
static var title2: String = "The Art of War" // expected-error{{initial value is not allowed here}} expected-error {{property in protocol must have explicit { get } or { get set } specifier}}
var title: String = "The Art of War" { get } // expected-error{{initial value is not allowed here}} expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{20-20= { get <#set#> \}}}
static var title2: String = "The Art of War" // expected-error{{initial value is not allowed here}} expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{28-28= { get <#set#> \}}}

associatedtype mytype
associatedtype mybadtype = Int
Expand Down Expand Up @@ -454,7 +457,7 @@ protocol ShouldntCrash {
let fullName: String { get } // expected-error {{'let' declarations cannot be computed properties}} {{3-6=var}}

// <rdar://problem/17200672> Let in protocol causes unclear errors and crashes
let fullName2: String // expected-error {{immutable property requirement must be declared as 'var' with a '{ get }' specifier}}
let fullName2: String // expected-error {{immutable property requirement must be declared as 'var' with a '{ get }' specifier}} {{3-6=var}} {{24-24= { get \}}}

// <rdar://problem/16789886> Assert on protocol property requirement without a type
var propertyWithoutType { get } // expected-error {{type annotation missing in pattern}}
Expand Down Expand Up @@ -500,7 +503,7 @@ class C4 : P4 { // expected-error {{type 'C4' does not conform to protocol 'P4'}
// <rdar://problem/25185722> Crash with invalid 'let' property in protocol
protocol LetThereBeCrash {
let x: Int
// expected-error@-1 {{immutable property requirement must be declared as 'var' with a '{ get }' specifier}}
// expected-error@-1 {{immutable property requirement must be declared as 'var' with a '{ get }' specifier}} {{13-13= { get \}}}
// expected-note@-2 {{declared here}}
}

Expand Down
6 changes: 3 additions & 3 deletions test/decl/subscript/subscripting.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,9 @@ class Foo {
protocol r23952125 {
associatedtype ItemType
var count: Int { get }
subscript(index: Int) -> ItemType // expected-error {{subscript in protocol must have explicit { get } or { get set } specifier}} {{36-36= { get set \}}}
var c : Int // expected-error {{property in protocol must have explicit { get } or { get set } specifier}}
subscript(index: Int) -> ItemType // expected-error {{subscript in protocol must have explicit { get } or { get set } specifier}} {{36-36= { get <#set#> \}}}

var c : Int // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{14-14= { get <#set#> \}}}
}

// <rdar://problem/16812341> QoI: Poor error message when providing a default value for a subscript parameter
Expand Down
2 changes: 1 addition & 1 deletion test/decl/var/lazy_properties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ struct S {

protocol SomeProtocol {
lazy var x : Int // expected-error {{'lazy' isn't allowed on a protocol requirement}} {{3-8=}}
// expected-error@-1 {{property in protocol must have explicit { get } or { get set } specifier}}
// expected-error@-1 {{property in protocol must have explicit { get } or { get set } specifier}} {{19-19= { get <#set#> \}}}
// expected-error@-2 {{lazy properties must have an initializer}}
lazy var y : Int { get } // expected-error {{'lazy' isn't allowed on a protocol requirement}} {{3-8=}}
// expected-error@-1 {{'lazy' must not be used on a computed property}}
Expand Down
16 changes: 10 additions & 6 deletions test/decl/var/properties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,10 +850,10 @@ struct WillSetDidSetDisambiguate3 {
}

protocol ProtocolGetSet1 {
var a: Int // expected-error {{property in protocol must have explicit { get } or { get set } specifier}}
var a: Int // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{13-13= { get <#set#> \}}}
}
protocol ProtocolGetSet2 {
var a: Int {} // expected-error {{property in protocol must have explicit { get } or { get set } specifier}}
var a: Int {} // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{14-16={ get <#set#> \}}}
}
protocol ProtocolGetSet3 {
var a: Int { get }
Expand All @@ -869,16 +869,20 @@ protocol ProtocolGetSet6 {
}

protocol ProtocolWillSetDidSet1 {
var a: Int { willSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} expected-error {{expected get or set in a protocol property}}
var a: Int { willSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{14-25={ get <#set#> \}}} expected-error {{expected get or set in a protocol property}}
}
protocol ProtocolWillSetDidSet2 {
var a: Int { didSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} expected-error {{expected get or set in a protocol property}}
var a: Int { didSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{14-24={ get <#set#> \}}} expected-error {{expected get or set in a protocol property}}
}
protocol ProtocolWillSetDidSet3 {
var a: Int { willSet didSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} expected-error 2 {{expected get or set in a protocol property}}
var a: Int { willSet didSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{14-32={ get <#set#> \}}} expected-error 2 {{expected get or set in a protocol property}}

}
protocol ProtocolWillSetDidSet4 {
var a: Int { didSet willSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} expected-error 2 {{expected get or set in a protocol property}}
var a: Int { didSet willSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{14-32={ get <#set#> \}}} expected-error 2 {{expected get or set in a protocol property}}
}
protocol ProtocolWillSetDidSet5 {
let a: Int { didSet willSet } // expected-error {{property in protocol must have explicit { get } or { get set } specifier}} {{14-32={ get <#set#> \}}} {{none}} expected-error 2 {{expected get or set in a protocol property}} expected-error {{'let' declarations cannot be computed properties}} {{3-6=var}}
}

var globalDidsetWillSet: Int { // expected-error {{non-member observing properties require an initializer}}
Expand Down