[Diagnostics] Improve {{none}} fix-it verifier #30791

omochi · 2020-04-03T10:42:16Z

This PR improve {{none}} fix-it verifier.

Currently, it means that no fix-it is expected.

I improve it to mean that no more fix-it is expected when use it with normal fix-it verifier at same time.

For example:

func labeledFunc(aa: Int, bb: Int) {}

func test() {
  // (1)
  labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}}
  
  // (2)
  labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}}
}

They means that diagnosed fix-its include {{15-18=aa}} at least.
They doesn't care about whether more other fix-its are emitted.
Actually (2) emits second fix-its for label bbx:, but it doesn't affect anything.

I introduce new feature for such situation to check strictly fix-its are just equal to expected.
For example:

func test() {
  // (3)
  labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}
  
  // (4)
  labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}
}

With above code, (3) is passed verification but (4) is not.

We can check fix-its more strictly.

Original code doesn't support using it with normal fix-its.

This is result from Xcode11.4

a.swift:3:135: error: expected no fix-its; actual fix-it seen: {{13-16=aa}} {{21-24=bb}}
labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{13-16=aa}} {{none}}
                                                                                                                                      ^~~~~~~
                                                                                                                                      {{13-16=aa}} {{21-24=bb}}

@CodaFi @gregomni @owenv Could you review this?

omochi · 2020-04-03T10:44:57Z

@swift-ci Please smoke test

owenv

I don't know how often this will be used, but I think you're right that it could be useful for some of the diagnostics like argument renaming which produce many fix-its. I have a few comments which I think could help improve the clarity of the code.

lib/Frontend/DiagnosticVerifier.cpp

LucianoPAlmeida · 2020-04-03T23:47:33Z

test/diagnostics/verifier.swift

@@ -16,7 +16,7 @@ let y: Int = "hello, world!" // expected-error@:49 {{cannot convert value of typ
 // Wrong fix-it
 let z: Int = "hello, world!" as Any
 // expected-error@-1 {{cannot convert value of type}} {{3-3=foobarbaz}}
-// CHECK: expected fix-it not seen; actual fix-its: {{[{][{]}}36-36= as! Int{{[}][}]}}
+// CHECK: expected fix-it not seen; actual fix-it seen: {{[{][{]}}36-36= as! Int{{[}][}]}}


Does this make sense to be actual fix-its seen: because it can have more than one? :)

Thanks. I will support plural form.

omochi · 2020-04-04T05:35:31Z

I have exactly been working on improving the diagnosis of argument labels recently.
I wanted this feature because I had to fix a lot of test cases in that work.
#30444

omochi · 2020-04-04T14:34:39Z

@owenv @LucianoPAlmeida

I updated implementation.

I followed the points except for the following.
#30791 (comment)

I think it's getting better.

owenv

I like the new direction, I just have a few more small comments. Also, thanks for adding all the new tests!

owenv · 2020-04-05T17:12:06Z

lib/Frontend/DiagnosticVerifier.cpp

+    auto makeActualFixitsPhrase =
+        [&](ArrayRef<DiagnosticInfo::FixIt> actualFixits,
+            std::string &actualFixitsStr) -> std::string {


Nit: I think this would be cleaner if it returned an std::pair of actualFixitsStr and the phrase instead of using actualFixitsStr as an out parameter.

OK. I did it.

owenv · 2020-04-05T17:12:50Z

lib/Frontend/DiagnosticVerifier.cpp

+        if (replStartLoc[-1] == ' ') {
+          replStartLoc--;
+        }


Nit: a comment explaining why this is needed might be nice for future readers.

I added comment.
https://github.com/apple/swift/blob/efaacddecf0b309215ff8e41b24d73558c64d6b8/lib/Frontend/DiagnosticVerifier.cpp#L604-L615

I also added here.
https://github.com/apple/swift/blob/efaacddecf0b309215ff8e41b24d73558c64d6b8/lib/Frontend/DiagnosticVerifier.cpp#L650-L661

owenv · 2020-04-05T17:14:21Z

lib/Frontend/DiagnosticVerifier.cpp

+      if (actualFixits.size() >= 2) {
+        str += "s";
+      }
+      str += " seen: ";
+      str += actualFixitsStr;


This is fine, but using llvm::Twine might make the concatenation a little more efficient

Thanks! I did it.

owenv · 2020-04-05T17:30:41Z

lib/Frontend/DiagnosticVerifier.cpp

+      const char *noneStartLoc =
+          expected.ExpectedEnd -
+          (fixitExpectationNoneString.size() + 4) /* length of '{{none}}' */;


I don't think the verifier currently requires {{none}} is at the end. I think it probably makes sense to report an error if it does not. I think we can diagnose this just before we start parsing a fix-it on line 469 if Expected.noExtraFixitsMayAppear is already true.

I thought that check is needed too.
I was going to make it another PR.

I implemented it and added test case.

omochi · 2020-04-05T18:10:05Z

Thank you for review. I will check tomorrow.

omochi · 2020-04-06T03:27:55Z

I updated 😄

@swift-ci Please smoke test

owenv

I left one optional comment for rewording a message, but this looks great. Thank you!

lib/Frontend/DiagnosticVerifier.cpp

Co-Authored-By: Owen Voorhees <[email protected]>

omochi · 2020-04-08T12:46:48Z

@owenv Thanks for your review and approval.
I updated message and fix some test cases for new feature.
After CI will passed, I will squash commits into one and merge PR.

@swift-ci Please smoke test

By the way, I granted a commit access recently, but a bot doesn't respond at all.
I hope it works this time.

owenv · 2020-04-08T14:24:17Z

@swift-ci please smoke test

CodaFi · 2020-04-09T00:54:35Z

lib/Frontend/DiagnosticVerifier.cpp

+    // it returns actual fix-its string and diagnostic phrase
+    auto makeActualFixitsPhrase =
+        [&](ArrayRef<DiagnosticInfo::FixIt> actualFixits)
+        -> std::tuple<std::string, std::string> {


std::tuple is not quite the vocabulary type you want to use here. Please use a real struct with named fields so we can avoid all of that tuple unpacking noise below and get some decently self-documenting code here.

I did it 🙂

CodaFi · 2020-04-09T00:54:58Z

lib/Frontend/DiagnosticVerifier.cpp

    }

-    bool matchedAllFixIts =
-        expected.Fixits.size() == FoundDiagnostic.FixIts.size();
+    bool isUnexpectedFixitsSeen =


Nit: const

omochi · 2020-04-09T13:22:50Z

@CodaFi Thank you for review!
I'm grad to get your comment before merge.
I will update.

omochi · 2020-04-09T15:30:15Z

@shahmishal Thanks for fixing my CI trigger.

@swift-ci Please smoke test

CodaFi · 2020-04-09T22:32:08Z

⛵

omochi · 2020-04-10T01:28:58Z

Thank you all!

omochi requested review from CodaFi, owenv and gregomni April 3, 2020 10:42

owenv reviewed Apr 3, 2020

View reviewed changes

LucianoPAlmeida reviewed Apr 3, 2020

View reviewed changes

omochi requested a review from owenv April 4, 2020 14:43

owenv reviewed Apr 5, 2020

View reviewed changes

omochi requested a review from owenv April 6, 2020 07:44

owenv approved these changes Apr 7, 2020

View reviewed changes

lib/Frontend/DiagnosticVerifier.cpp Outdated Show resolved Hide resolved

omochi and others added 10 commits April 8, 2020 21:11

[Diagnostics] Improve {{none}} fix-it verifier

94bb16a

split two conditions

1a434a4

define "none" constant

ca42e69

support plural

87edf8e

use Twine and add comment for replacement range

1971f4a

check if {{none}} is at the end

08d332f

use noneMarkerStartLoc

45c9034

update second {{none}} error message

11a20c9

Co-Authored-By: Owen Voorhees <[email protected]>

update test case for second {{none}}

34d52bb

fix test case for new {{none}} check

e8cc1f1

omochi force-pushed the verify-none branch from 465d76e to e8cc1f1 Compare April 8, 2020 12:41

CodaFi reviewed Apr 9, 2020

View reviewed changes

omochi added 2 commits April 10, 2020 00:12

Use named struct

eed176a

set const

9f59f11

CodaFi merged commit a07b35a into swiftlang:master Apr 9, 2020

omochi deleted the verify-none branch April 10, 2020 01:29

owenv mentioned this pull request Apr 10, 2020

[DiagnosticVerifier] Fix ASAN issue in the verifier #30947

Merged

[Diagnostics] Improve {{none}} fix-it verifier #30791

[Diagnostics] Improve {{none}} fix-it verifier #30791

Uh oh!

Conversation

omochi commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omochi commented Apr 3, 2020

Uh oh!

owenv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omochi commented Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omochi commented Apr 4, 2020

Uh oh!

owenv left a comment

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!

omochi commented Apr 5, 2020

Uh oh!

omochi commented Apr 6, 2020

Uh oh!

owenv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omochi commented Apr 8, 2020

Uh oh!

owenv commented Apr 8, 2020

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!

omochi commented Apr 9, 2020

Uh oh!

omochi commented Apr 9, 2020

Uh oh!

CodaFi commented Apr 9, 2020

Uh oh!

omochi commented Apr 10, 2020

Uh oh!

Uh oh!

omochi commented Apr 3, 2020 •

edited

Loading

omochi commented Apr 4, 2020 •

edited

Loading