Make sure protocol witness errors don't leave the conformance context #16489

jrose-apple · 2018-05-10T05:07:01Z

That is, if there's a problem with a witness, and the witness comes from a different extension from the conformance (or the original type, when the conformance is on an extension), put the main diagnostic on the conformance, with a note on the witness. This involves some shuffling and rephrasing of existing diagnostics too.

There's a few reasons for this change:

More context. It may not be obvious why a declaration in file A.swift needs to be marked 'public' if you can't see the conformance in B.swift.
Better locations for imported declarations. If you're checking a conformance in a source file but the witness came from an imported module, it's better to put the diagnostic on the part you have control over. (This is especially true in Xcode, which can't display diagnostics on imported declarations in the source editor.)
Plays better with batch mode. Without this change, you can have diagnostics being reported in file A.swift that are tied to a conformance declared in file B.swift. Of course the contents of A.swift also affect the diagnostic, but compiling A.swift on its own wouldn't produce the diagnostic, and so putting it there is problematic. (See [Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 #16485.)

The change does in some cases make for a worse user experience, though; if you just want to apply the changes and move on, the main diagnostic isn't in the "right place". It's the note that has the info and possible fix-it. It's also a slightly more complicated implementation.

Let's be consistent!

jrose-apple · 2018-05-10T05:07:10Z

@swift-ci Please test

jrose-apple · 2018-05-10T05:07:17Z

@swift-ci Please test source compatibility

jrose-apple · 2018-05-10T05:07:57Z

Still needs new tests for the new behavior in addition to the updates for the existing tests, but I want to see if anything breaks, or if reviewers have objections.

davidungar

@jrose-apple What sort of review are you looking for here? Since I'm not familiar with the type-checker, I can't quickly offer thoughts on correctness. If you let me know what would be most helpful, I will do it. A very quick glance at the diffs did not result in anything jumping out at me.

jrose-apple · 2018-05-10T16:18:57Z

For you and Graydon it was more a CC than a review request, I guess. "Here's the part I've been working on, here's the general approach I took". It's Huon and/or Doug, who've been working with conformance checking more, that I really want to look over this.

jrose-apple · 2018-05-10T21:35:09Z

Now with tests for everything except the witness_non_objc_* diagnostics, which don't have any other tests except one lonely case that should probably be valid anyway (SR-7601). I can't figure out how to get the others to happen, so I'm leaving it for now.

@swift-ci Please smoke test

DougGregor

LGTM

huonw

Minor questions other than the ctor->isImplicit which is a just slightly-more-than-minor question.

huonw · 2018-05-10T22:34:04Z

lib/Sema/TypeCheckProtocol.cpp

+            // If the main diagnostic is emitted on the conformance, we want to
+            // attach the fix-it to the note that shows where the initializer is
+            // defined.
+            fixItDiag.reset();


The destructor-based emission of diagnostics makes this if kinda... magical.

Referencing the instances below, it seems this could almost be factored out, maybe?

I don't think there's enough commonality to factor it out, but I just realized that fixItDiag->flush() will work as well, since InFlightDiagnostic tracks whether the diagnostic has been flushed yet. I'll do that instead.

huonw · 2018-05-10T22:41:36Z

lib/Sema/TypeCheckProtocol.cpp

                         diag::witness_initializer_failability,
                         ctor->getFullName(),
                         witnessCtor->getFailability()
                           == OTK_ImplicitlyUnwrappedOptional)
            .highlight(witnessCtor->getFailabilityLoc());
+          if (diagLoc != witness->getLoc()) {


It seems every second diagnostic touched here gets this if block almost verbatim. We could consider factoring it out somehow?

I guess that makes sense for all the decl_declared_here ones, at least. I tried to think of how to fold it into getLocForDiagnosingWitness with a callback and couldn't, then tried to think of how to generalize it over any possible follow-up note and couldn't. But more of them ended up just being "declared here" than I originally thought.

huonw · 2018-05-10T22:43:03Z

lib/Sema/TypeCheckProtocol.cpp

+                witness->getFullName(),
+                proto->getFullName());
+            if (diagLoc == witness->getLoc()) {
+              addOptionalityFixIts(adjustments, ctx, witness, diag);


What's the thinking behind these only applying in this case, rather than the reset/emplace thing you've done for other similar versions to get fixits on the note?

I mostly only did that where there was a dedicated note, rather than just a "declared here". There are two reasons for that: "declared here" doesn't tell you what the fix-it is doing, and if the declaration is written somewhere else, it's less likely that its sole purpose is to satisfy this protocol. The one exception was the required initializer fix-it, because people complain that they never remember what they have to write for initializers but I couldn't think of a good custom note for that case.

Happy to accept feedback here.

Sounds like reasonable logic.

huonw · 2018-05-10T22:45:41Z

lib/Sema/TypeCheckProtocol.cpp

+                                             ctor->getFullName()));
+          }
+          if (!inExtension) {
+            fixItDiag->fixItInsert(ctor->getAttributeInsertionLoc(true),


I think this has changed behaviour: it will get attached to the witness_initializer_not_require diagnostic if ctor->isImplicit()?

It's subtle, but for an implicit constructor getAttributeInsertionLoc will (now) return an invalid location, which drops the fix-it. I found this more natural than trying to untangle which conditions are needed and which aren't.

jrose-apple · 2018-05-10T23:18:24Z

@swift-ci Please smoke test

jrose-apple · 2018-05-11T01:29:02Z

@swift-ci Please smoke test Linux

huonw · 2018-05-11T01:44:31Z

lib/Sema/TypeCheckProtocol.cpp

+                     type, genericParam, !genericParam.isNull(),
+                     assocType->getFullName(), Proto->getFullName());
+  if (typeDecl && diagLoc != typeDecl->getLoc())
+    ctx.Diags.diagnose(typeDecl, diag::type_declared_here);


emitDeclaredHereIfNeeded(ctx.Diags, diagLoc, typeDecl)?

Huh. This one was "type declared here" instead of "'Foo' declared here", but I guess we have other type witness diagnostics where I went with the "'Foo' declared here" one.

huonw · 2018-05-11T01:45:26Z

lib/Sema/TypeCheckProtocol.cpp

+                witness->getFullName(),
+                proto->getFullName());
+            if (diagLoc == witness->getLoc()) {
+              addOptionalityFixIts(adjustments, ctx, witness, diag);


Sounds like reasonable logic.

That is, if there's a problem with a witness, and the witness comes from a different extension from the conformance (or the original type, when the conformance is on an extension), put the main diagnostic on the conformance, with a note on the witness. This involves some shuffling and rephrasing of existing diagnostics too. There's a few reasons for this change: - More context. It may not be obvious why a declaration in file A.swift needs to be marked 'public' if you can't see the conformance in B.swift. - Better locations for imported declarations. If you're checking a conformance in a source file but the witness came from an imported module, it's better to put the diagnostic on the part you have control over. (This is especially true in Xcode, which can't display diagnostics on imported declarations in the source editor.) - Plays better with batch mode. Without this change, you can have diagnostics being reported in file A.swift that are tied to a conformance declared in file B.swift. Of course the contents of A.swift also affect the diagnostic, but compiling A.swift on its own wouldn't produce the diagnostic, and so putting it there is problematic. The change does in some cases make for a worse user experience, though; if you just want to apply the changes and move on, the main diagnostic isn't in the "right place". It's the note that has the info and possible fix-it. It's also a slightly more complicated implementation.

jrose-apple · 2018-05-11T02:32:00Z

@swift-ci Please smoke test

Make sure protocol witness errors don't leave the conformance context (cherry picked from commit 88169d6)

Convert backticks to single-quotes in Sema diagnostics

545b12f

Let's be consistent!

jrose-apple requested review from graydon, davidungar, DougGregor and huonw May 10, 2018 05:07

davidungar reviewed May 10, 2018

View reviewed changes

jrose-apple force-pushed the to-put-it-in-context branch from f6a1586 to ba5bca6 Compare May 10, 2018 21:33

DougGregor approved these changes May 10, 2018

View reviewed changes

huonw approved these changes May 10, 2018

View reviewed changes

jrose-apple force-pushed the to-put-it-in-context branch from ba5bca6 to 2be4616 Compare May 10, 2018 23:18

huonw reviewed May 11, 2018

View reviewed changes

davidungar approved these changes May 11, 2018

View reviewed changes

jrose-apple force-pushed the to-put-it-in-context branch from 2be4616 to 6bd7e5e Compare May 11, 2018 02:31

jrose-apple merged commit 88169d6 into swiftlang:master May 11, 2018

jrose-apple deleted the to-put-it-in-context branch May 11, 2018 04:44

jrose-apple added a commit to jrose-apple/swift that referenced this pull request May 11, 2018

Merge pull request swiftlang#16489 from jrose-apple/to-put-it-in-context

6c7aa73

Make sure protocol witness errors don't leave the conformance context (cherry picked from commit 88169d6)

jrose-apple added a commit to jrose-apple/swift that referenced this pull request May 11, 2018

Merge pull request swiftlang#16489 from jrose-apple/to-put-it-in-context

367f57e

Make sure protocol witness errors don't leave the conformance context (cherry picked from commit 88169d6)

This was referenced May 11, 2018

[4.2] Fixes for batch mode diagnostics in Xcode #16529

Merged

[4.2 EARLY] Fixes for batch mode diagnostics in Xcode #16530

Merged

Make sure protocol witness errors don't leave the conformance context #16489

Make sure protocol witness errors don't leave the conformance context #16489

Uh oh!

Conversation

jrose-apple commented May 10, 2018

Uh oh!

jrose-apple commented May 10, 2018

Uh oh!

jrose-apple commented May 10, 2018

Uh oh!

jrose-apple commented May 10, 2018

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented May 10, 2018

Uh oh!

jrose-apple commented May 10, 2018

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

huonw 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!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented May 10, 2018

Uh oh!

jrose-apple commented May 11, 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!

jrose-apple commented May 11, 2018

Uh oh!

Uh oh!