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

mzp · 2018-10-02T04:08:43Z

Improve fix-it for following code to resolves SR-8340.

protocol P {
  subscript() -> Bool 
  // before: insert "{ get set }"
  // after: insert "{ get <#set#> }"

  var v : Bool
  // before: No fixit (error message only)
  // after: insert "{ get <#set#> } "

  let n : Bool
  // before: No fixit (error message only)
  // after: replace with "var" and insert "{ get } "
}

https://bugs.swift.org/browse/SR-8340

xedin

This is definitely a nice improvement, thank you! One thing - it looks like you should use getSourceRange() while replacing let with var, otherwise it would just insert var and keep let or rather et I think...

One way to validate everything would be to do @swift-ci build toolchain and verify in Xcode.

rintaro · 2018-10-03T07:44:25Z

I believe fixItReplace(...->getLoc(), "var") is OK because fixItReplace() receives only SourceRange and SourceLoc is converted to SourceRange automatically.
Of course we need test cases, though.

rintaro · 2018-10-03T07:45:33Z

test/decl/protocol/protocols.swift

@@ -502,6 +502,7 @@ protocol LetThereBeCrash {
  let x: Int
  // expected-error@-1 {{immutable property requirement must be declared as 'var' with a '{ get }' specifier}}
  // expected-note@-2 {{declared here}}
+  // {{13-13= { get \}}}


This {{ ... }} belongs to the line 503 (2 lines above)?

fixed at f0f33eb

rintaro · 2018-10-03T07:52:59Z

test/decl/protocol/protocols.swift

@@ -454,7 +454,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}} {{24-24= { get \}}}


I believe this diagnostics emits {{3-6=var}} as well.

Small tip: Try adding {{none}} without adding {{3-6=var}}. That should fail with fixit for test-cases. e.g.

test.swift:2:57: error: expected no fix-its; actual fix-it seen: {{11-11=,}} "foo": 1 // expected-error {{expected ',' separator}} {{none}} ^~~~~~~ {{11-11=,}}

You can paste it. (after you confirmed it's expected, of course)

Wow, it's useful tip.

fixed at 7d35c95

rintaro · 2018-10-03T07:54:36Z

test/decl/var/properties.swift

@@ -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#> \}}}


Remove // before {{13-13....

removed at f1d830a

rintaro · 2018-10-03T07:55:26Z

test/decl/var/lazy_properties.swift

@@ -12,6 +12,7 @@ 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@-2 {{lazy properties must have an initializer}}
+  // {{18-18= { get set \}}}


This fix-it belongs to line 13.

rintaro · 2018-10-03T07:56:53Z

lib/Sema/CodeSynthesis.cpp

+        if (braces.isValid())
+          diag
+            .fixItReplace(var->getParentPatternBinding()->getLoc(), "var")
+            .fixItReplace(braces, " { get }");


It looks like we don't have test cases for this path. Is this possible?

added test case at da9a19b

I don't think the test case added in da9a19b is actually testing this code path.
If so, the fix-it should be {{14-32= { get \}}} instead of {{14-32={ get <#set#> \}}}.
Currently, even if it's spelled with let, the parser creates non-let PatternBindingDecl if the decl has accessor block.

I think it's OK to remove if (braces.isValid()) block here.

rintaro · 2018-10-03T08:03:44Z

lib/Parse/ParseDecl.cpp

@@ -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#> }");


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?

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

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.

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

rintaro · 2018-10-03T08:09:15Z

lib/Sema/CodeSynthesis.cpp

+        auto diag = TC.diagnose(var->getLoc(), diag::protocol_property_must_be_computed);
+
+        if (braces.isValid())
+          diag.fixItReplace(braces, " { get <#set#> }");


" { adds extra space.

striped at 7ae9eb7

xedin · 2018-10-03T17:06:33Z

@rintaro SourceLoc forms a single chacter SourceRange, that’s why I don’t think that it’s desirable here

jrose-apple · 2018-10-03T17:22:14Z

SourceLoc does not form a single-character SourceRange; it forms a single-token SourceRange. You're mixing up SourceRange and CharSourceRange.

xedin · 2018-10-03T17:45:51Z

Ah, okay so it's just going to wrap the whole let token, that's reasonable then!

I mixed up couple of things

mzp · 2018-10-04T04:26:56Z

I pushed changes to fix commented code.

xedin · 2018-10-04T04:41:14Z

@swift-ci please smoke test

rintaro · 2018-10-04T08:19:01Z

lib/Sema/CodeSynthesis.cpp

+        if (braces.isValid())
+          diag
+            .fixItReplace(var->getParentPatternBinding()->getLoc(), "var")
+            .fixItReplace(braces, " { get }");


I don't think the test case added in da9a19b is actually testing this code path.
If so, the fix-it should be {{14-32= { get \}}} instead of {{14-32={ get <#set#> \}}}.
Currently, even if it's spelled with let, the parser creates non-let PatternBindingDecl if the decl has accessor block.

I think it's OK to remove if (braces.isValid()) block here.

rintaro · 2018-10-04T08:26:31Z

lib/Sema/CodeSynthesis.cpp

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


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.

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.

rintaro · 2018-10-04T16:15:24Z

@swift-ci Please smoke test

rintaro

Thanks!

…19660) * [Parser] Improve fix-it for subscription in protocol * [Sema] Add fix-it for property in protocol https://bugs.swift.org/browse/SR-8340

mzp added 2 commits October 2, 2018 11:54

[Parser] Improve fix-it for subscription in protocol

67bf1a9

https://bugs.swift.org/browse/SR-8340

[Sema] Add fix-it for property in protocol

ee4f9b2

https://bugs.swift.org/browse/SR-8340

mzp changed the title ~~Improve fix-it for var and subscript in Protocol~~ [SR-8340]Improve fix-it for var and subscript in Protocol Oct 2, 2018

jrose-apple requested review from rintaro and xedin October 2, 2018 18:22

xedin previously requested changes Oct 3, 2018

View reviewed changes

rintaro reviewed Oct 3, 2018

View reviewed changes

mzp added 6 commits October 4, 2018 11:55

fixup! [Sema] Add fix-it for property in protocol

f0f33eb

fixup! [Sema] Add fix-it for property in protocol

7d35c95

fixup! [Sema] Add fix-it for property in protocol

f1d830a

fixup! [Sema] Add fix-it for property in protocol

9ca89e4

fixup! [Sema] Add fix-it for property in protocol

7ae9eb7

fixup! [Sema] Add fix-it for property in protocol

da9a19b

rintaro reviewed Oct 4, 2018

View reviewed changes

mzp added 2 commits October 4, 2018 23:37

fixup! [Sema] Add fix-it for property in protocol

1ddfa58

fixup! [Sema] Add fix-it for property in protocol

fd5f3ff

rintaro approved these changes Oct 4, 2018

View reviewed changes

rintaro merged commit f2bdce8 into swiftlang:master Oct 4, 2018

swift-ci mentioned this pull request Oct 2, 2018

[SR-8340] Protocol var and subscript "must have explicit { get } or { get set }" could be improved #50868

Closed

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

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

Uh oh!

Conversation

mzp commented Oct 2, 2018

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

rintaro commented Oct 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Oct 3, 2018

Uh oh!

jrose-apple commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Oct 3, 2018

Uh oh!

mzp commented Oct 4, 2018

Uh oh!

xedin commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro commented Oct 4, 2018

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro Oct 3, 2018 •

edited

Loading

jrose-apple commented Oct 3, 2018 •

edited

Loading