[Sema][GSB] Warn about redundant ': Any' conformances & constraints #17425

hamishknight · 2018-06-22T16:49:23Z

This PR implements a warning for explicitly stated : Any conformances and constraints, which are always redundant. In addition, a fix-it for removal is provided, except for the case of constraints that are parsed as inheritance clauses, e.g <T : Any> & protocol P : Any. I couldn't see a simple way to implement fix-its for these cases, as inherited types are just represented as TypeReprs in GSB.

From what I can tell (there may well be a better approach I'm overlooking), we'd need a custom data structure by which to represent inheritance constraints (which can include a removal range), much like RequirementRepr, which we would need to store on TypeDecl and ExtensionDecl AST nodes.

Such a change would be pretty widespread, so I think it would be best to do it in a follow-up PR (I'll file a JIRA if/when this is merged). GSB already doesn't provide fix-its for its existing redundant constraint warning, so I think this PR is an acceptable waypoint.

Resolves SR-8008 & SR-8009.

hamishknight · 2018-06-22T16:50:17Z

include/swift/AST/DiagnosticsSema.def

+        "redundant conformance of %0 to %1", (Type, Type))
+NOTE(all_types_implicitly_conform_to,none,
+     "all types implicitly conform to %0", (Type))
+


An alternative phrasing for the note would be "all types are subtypes of %0", which is more technically correct for Any as it's not actually a protocol you can conform to (just an empty protocol composition) – but I feel the current wording is more user friendly. Thoughts?

I think it's fair to say that one conforms to a protocol composition (e.g. we say that people conform to typealias Codable = Encodable & Decodable), even an empty one. I vote on keeping what you've written.

hamishknight · 2018-06-22T16:55:51Z

test/Generics/redundant_constraints.swift

+protocol P3 {
+  // expected-warning@-1 {{redundant conformance constraint 'Self.X1': 'Any'}}
+  // expected-note@-2 {{all types implicitly conform to 'Any'}}
+  // expected-warning@-3 {{redundant conformance constraint 'Self.X2': 'Any'}} {{21-35=}}


Diagnosing on the protocol declaration location for constraints relating to associated types is what we already do for other diagnostics in GSB, so I haven't changed the behaviour here.

Is this intentional?

huonw

Looks like a good step forward! I'd love to see some tests for the fixits (in tests/FixCode probably).

huonw · 2018-06-23T00:12:47Z

include/swift/AST/DiagnosticsSema.def

+        "redundant conformance of %0 to %1", (Type, Type))
+NOTE(all_types_implicitly_conform_to,none,
+     "all types implicitly conform to %0", (Type))
+


I think it's fair to say that one conforms to a protocol composition (e.g. we say that people conform to typealias Codable = Encodable & Decodable), even an empty one. I vote on keeping what you've written.

huonw · 2018-06-23T00:12:52Z

include/swift/AST/Decl.h

@@ -963,6 +963,8 @@ enum class RequirementReprKind : unsigned {
 /// \c GenericParamList assumes these are POD-like.
 class RequirementRepr {
  SourceLoc SeparatorLoc;
+  SourceRange RemovalRange;


Could you add a brief comment along the lines of "this range will include everything that needs to be removed to keep the program syntactically valid (e.g. where and commas), unlike just from the start of FirstType to the end of SecondType"?

huonw · 2018-06-23T02:36:46Z

lib/Parse/ParseGeneric.cpp

+    }
+    if (HasNextReq) {
+      // Include the next comma in the removal range of a middle requirement.
+      removalRange.End = NextCommaLoc;


I think this means that one can have two requirements with overlapping removal ranges, e.g. where X: Something, T: Any, U: Any, both those last ones remove the trailing ,. I don't know if the fixit-engine works correctly with this; might be worth including this sort of case in a test.

You're right – this does appear to cause issues. Using "fix all issues" in Xcode can result in a stray commas being left behind (but applying the fix-its individually appears to be okay). So it looks like we're just gathering the emitted fix-its and performing them without re-typechecking at all. I'm not too sure what the right solution is here.

If need be, we could always omit the removal range for the last requirement in a where clause with > 1 requirements, which should avoid any overlapping ranges (and more generally, cases where we'd need to re-typecheck to get a correct removal range such as needing to include the where token when removing all requirements). We'd also need to do something similar for inheritance clauses. Not ideal, but I guess it's better than having fix-mes that sometimes can't be reliably bulk-applied or no fix-mes at all.

Thoughts?

That would mean that there's no fixits for the removal in where T: Something, U: Any, right? It does seem annoying we can't do multiple overlapping deletions, but it's probably better to miss the help in some cases instead of mangling the code.

huonw · 2018-06-23T02:46:14Z

lib/AST/GenericSignatureBuilder.cpp

  ArrayRef<TypeLoc> inheritedTypes = typeDecl ? typeDecl->getInherited()
                                              : extDecl->getInherited();
  for (unsigned index : indices(inheritedTypes)) {
    Type inheritedType = typeDecl ? typeDecl->getInheritedType(index)
                                  : extDecl->getInheritedType(index);
    if (!inheritedType) continue;

+    // If the inherited type is 'Any', allow it to pass through verbatim, as
+    // GSB diagnoses redundant 'Any' constraints.
+    bool skipDecomposition = inheritedType->isEqual(ctx.TheAnyType);


Could this check be moved into visitInherited? Doing it out here both requires adding the argument somewhat unnecessarily (since the function has all the info it needs to do the computation itself), and also I suspect (but am not sure of the exact parser behaviour) it might miss cases like Any & P where the Any gets found recursively.

I purposefully did the check outside of visitInherited in order to miss cases like Any & P, partly in order to have parity with the behaviour for protocol composition constraints that appear in where clauses (which aren't decomposed until after the diagnostic), and partly because I think we should diagnose redundant Any members in protocol compositions as a part of a separate broader diagnostic (SR-8082) that warns on the usage of Any & ... anywhere, not just in constraints.

Does this sound good? If so, I'll expand on the comment to include the above rationale for performing the check outside of visitInherited, and add a test to make sure we ignore Any & P constraints for now.

Thanks for the review! I'll get on with adding the fix-me tests + removal range comment.

I think we should diagnose redundant Any members in protocol compositions as a part of a separate broader diagnostic (SR-8082) that warns on the usage of Any & ... anywhere, not just in constraints.

Ah, yeah, that's a more general and better approach.

Does this sound good? If so, I'll expand on the comment to include the above rationale for performing the check outside of visitInherited, and add a test to make sure we ignore Any & P constraints for now.

👍

hamishknight · 2018-07-02T18:03:05Z

Sorry for the delay on this – I've just pushed a few changes (and rebased to resolve a merge conflict).

Notable changes include:

Avoid overlapping removal fix-its. For requirements in where clauses, this means not emitting removal fix-its for the last requirement in a where clause with > 1 requirements. For inheritance clauses, we're able to make this a bit more fine-grained (as they're all emitted together) – we avoid emitting a removal fix-it for the second entry if we emitted a removal fix-it for the first entry.
Emit an error for duplicate 'Any' conformances. Previously for:
```
struct S : Any, Any {}
```
we emit two warnings for the redundant Any conformances. Now we emit an error instead for the second one, as it's a duplicate. This is more consistent with other diagnostics, but IMO the diagnostics are better being warnings in this case (but we can easily revert the commit later down the line if we want this behaviour, so I'm sticking to consistency for now). I did ask over at https://forums.swift.org/t/why-is-duplicate-conformance-an-error-instead-of-a-warning/14043/7 what the rationale was for duplicate inheritance being error instead of a warning in the case of conformances, but I don't think we reached a conclusive answer.
Add a highlight to both the conformance and constraint redundant 'Any' diagnostics that covers the source range (rather than the removal range), which IMO makes the diagnostic slightly nicer. Is there no way to test highlight ranges?
Add FixCode tests
Add some more explanatory comments
Removed tests that involved Swift 3 compatibility diagnostics/features, as I see that's going away soon (Remove support for -swift-version 3 from the expression type checker. #17691).

My question about the location of the diagnostic for associated type constraints remains. For the following:

protocol P {
  associatedtype X where X : Any
}

We currently emit the warning on the protocol declaration location (i.e P) rather than on the constraint (i.e X : Any), which is consistent with the other diagnostics in GSB. Is this intentional? It would seem to me that emitting on the constraint would be clearer.

Any and all feedback welcome!

hamishknight · 2018-07-03T14:44:21Z

lib/AST/GenericSignatureBuilder.cpp

    const auto &inherited = inheritedTypes[index];
-    visitInherited(inheritedType, inherited.getTypeRepr());
+    visitInherited(inheritedType, inherited.getTypeRepr(), skipDecomposition);
  }


Thinking about it, why do we decompose protocol compositions that appear as inherited type requirements earlier than requirements in a where clause? Would it be possible to eliminate visitInherited altogether and instead do a loop over the inherited types, calling addTypeRequirement for each one? I tried, and it didn't break any tests. That would allow us to get rid of this somewhat ugly special case for Any and generally simplify the code a bit.

hamishknight · 2018-07-11T15:58:10Z

(Just rebased to resolve another merge conflict)

- Protocol decls don't have explicit generic parameters, so don't use their location. - Get the location of the extended type token rather than the end brace token. Neither of these paths are being exercised yet, so no tests to accompany.

Such a stated conformance is always redundant. This patch purposefully ignores conformance constraints that are parsed as inheritance clauses, such as with protocols and generic placeholders (e.g '<T : Any>'). These are best diagnosed in the generic signature builder. Resolves SR-8008

Not all redundant type constraints have an associated protocol decl, e.g 'T : Any'.

Such a stated constraint is always redundant. Resolves SR-8009.

Note that this doesn't handle constraints that are parsed as inheritance clauses, such as <T : Any> as we don't currently have a custom data structure by which to represent it in the GSB.

…: Any' constraints and conformances This is in order to avoid emitting removal fix-its that would require their source range to be re-calculated after the application of another fix-it. FIX-ME(SR-8102): Improve the fix-it engine to handle such 'dependant' removal fix-its.

…ances Previously, for the following: struct S : Any, Any {} we would just emit two warnings. But in the interest of consistency, this commit changes the behaviour such that we instead emit a warning and an error for the duplicate conformance.

hamishknight · 2018-09-06T17:10:50Z

Going to close this for now – I'm going to split the source highlighting/removal stuff into a separate PR (as that's applicable to other GSB diagnostics), and then open a fresh PR for the redundant : Any diagnostic. I believe the diagnostic logic can be changed such that we do the diagnostic for constraints that are parsed as inheritance clauses in checkInheritanceClause rather than GSB thanks to the fact that I believe checkInheritanceClause no longer gets called multiple times for such cases.

hamishknight mentioned this pull request Jun 22, 2018

[Sema] Warn about redundant ': Any' conformances #17242

Closed

hamishknight commented Jun 22, 2018

View reviewed changes

jrose-apple requested review from DougGregor and huonw June 22, 2018 17:38

huonw reviewed Jun 23, 2018

View reviewed changes

hamishknight force-pushed the warn-conform-constrain-any branch from 64b8456 to 775282f Compare July 2, 2018 18:01

hamishknight force-pushed the warn-conform-constrain-any branch 2 times, most recently from 2066a4e to bffa963 Compare July 3, 2018 13:45

hamishknight commented Jul 3, 2018

View reviewed changes

hamishknight force-pushed the warn-conform-constrain-any branch from bffa963 to 1235c25 Compare July 4, 2018 09:26

hamishknight force-pushed the warn-conform-constrain-any branch from 1235c25 to 5c9dbc7 Compare July 11, 2018 15:57

hamishknight added 7 commits July 29, 2018 17:46

[GSB] Use Type instead of ProtocolDecl in a couple of diagnostics [NFC]

4ae85ac

Not all redundant type constraints have an associated protocol decl, e.g 'T : Any'.

[GSB] Warn about redundant ': Any' type constraints

94e4e38

Such a stated constraint is always redundant. Resolves SR-8009.

[Parse][GSB] Add fix-it to remove redundant ': Any' constraints

099430d

Note that this doesn't handle constraints that are parsed as inheritance clauses, such as <T : Any> as we don't currently have a custom data structure by which to represent it in the GSB.

hamishknight force-pushed the warn-conform-constrain-any branch from 5c9dbc7 to d5d6586 Compare July 29, 2018 18:37

hamishknight closed this Sep 6, 2018

[Sema][GSB] Warn about redundant ': Any' conformances & constraints #17425

[Sema][GSB] Warn about redundant ': Any' conformances & constraints #17425

Uh oh!

Conversation

hamishknight commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

hamishknight Jun 23, 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!

hamishknight commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamishknight Jul 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!

hamishknight commented Jul 11, 2018

Uh oh!

hamishknight commented Sep 6, 2018

Uh oh!

Uh oh!

hamishknight commented Jun 22, 2018 •

edited

Loading

hamishknight Jun 23, 2018 •

edited

Loading

hamishknight commented Jul 2, 2018 •

edited

Loading

hamishknight Jul 3, 2018 •

edited

Loading