Why is duplicate conformance an error instead of a warning?

Development Compiler
1

Currently, we diagnose the following with an error:

protocol P {}
struct S : P, P {} // error: Duplicate inheritance from 'P'

What's the rationale for this being an error rather than a warning? As I understand it, errors are for programs that aren't well-formed, but the above seems well-formed to me – the additional P in the inheritance clause is redundant and should therefore just be ignored (this is different from multiple inheritance with classes, which I agree should be illegal – that's currently its own error).

This is also the case with protocol "inheritance", where the types in the inheritance clause behave more like constraints than inherited types:

protocol P : AnyObject, AnyObject {} // error: Duplicate inheritance from 'AnyObject'

(we actually also emit a "Redundant constraint 'Self' : 'AnyObject'" warning from GSB for this, which is what I would have expected)

I'm asking because I'm currently in the middle of implementing a patch that emits a warning for redundant : Any constraints and conformances over at [Sema][GSB] Warn about redundant ': Any' conformances & constraints by hamishknight · Pull Request #17425 · apple/swift · GitHub. Currently, I've put the warning logic for redundant : Any conformances above the check for duplicate inheritance, meaning that we'll no longer emit an error for code like this (just two warnings):

struct S : Any, Any {}

I thought that accepting this would be okay while implementing the patch (it makes the diagnostics cleaner at least, rather than 2 warnings and an error), but now I'm having second thoughts. Should we continue to reject the above with an error? (and same for code like protocol P : Any, Any {}?).

2

The diagnostics are rather inconsistent in relation to these redundancies. The first example is an error, the second and third are completely ignored.

protocol P {}
struct S : P {}
extension S : P {} // Redundant conformance of 'S' to protocol 'P'

protocol P {}
struct S {}
extension S : P & P {}

protocol P {}
protocol Q: P {}
struct S: P, Q {}

Your particular case might well be a generic error (you can see the message ignoring the conformance). I agree it deserves a warning with a fixit rather than an error. Being an error, it is probably diagnosed after CSGen. A warning would require diagnosing that right before generating the duplicate constraint or removing a constraint upon diagnosing afterwards. The former remains the neatest variant. It would also be great to say 'conformance' instead of 'inheritance' when the duplicated subject is existential.

2 Likes
3

The third isn't something to flag at all; it's legitimate and relatively common to explicitly declare a conformance that might otherwise be implied, e.g. I personally find extension T: Equatable { ... } extension T: Hashable { ... } clearer and more obvious than just extension T: Hashable { ... }.

It would be nice if there was at least warnings for redundant protocol compositions like P & P (and P & Q). @hamishknight's [SR-8082] Swift should warn on redundant 'Any' members in a protocol composition · Issue #50615 · apple/swift · GitHub (which is what the Any work he mentions is working towards) is related to those.

2 Likes
4

I completely agree with extension T: Equatable {...}; extension T: Hashable {...}, but extension T: Equatable, Hashable doesn't quite convince me. I can sort of agree incrementally listing conformances in the latter way can help in getting a better picture of the hierarchy and requires less audit. Come to think of it, ignoring Equatable & Hashable would then have similar motivation. On the other hand, in general one might assume those are completely unrelated protocols and get the wrong idea. Anyways, I also understand it would be slightly inconsistent to complain about one way of doing it and ignore the other.

P.S. SR-7405 is apparently the right one for redundant protocol compositions.

5

Yeah, that's fair. It might be a bit weird to warn on Equatable & Hashable but allow Equatable, Hashable. Maybe this needs to be context sensitive: when declaring a conformance, redundancy is okay, but otherwise it gets a warning.

Ah, sorry. It didn't come up in my search, but should now be easier to find for people like me who only search for "protocol composition". :slight_smile:

1 Like
6

Yes, in general you should be careful about issuing warnings or errors for things that are technically redundant because sometimes that redundancy is beneficial for humans reading code. In general I think perhaps only the most vacuous duplications should be signalled.

3 Likes
7

I'm quite sympathetic towards the following being an error:

protocol P {}
struct S : P {}
extension S : P {} // error: Redundant conformance of 'S' to protocol 'P'

as it's most likely a sign that the user's trying to conform the type S to the protocol P in two different ways, which isn't something that's well-formed. This is most apparent when you consider conditional conformances, where we don't allow multiple conformances to the same protocol, even if the constraints are clearly disjoint:

protocol P {
  func foo()
}
struct S<T> {}

// error: Redundant conformance of 'S<T>' to protocol 'P'
extension S : P where T == Int {
  func foo() {}
}

extension S : P where T == String {
  func foo() {}
}

However, if the duplicate conformance shows up in the same inheritance clause, I think it's reasonable to downgrade to a warning, as it's fairly clear what the user's intent is. The diagnostic for duplicate inherited types in an inheritance clause (note this is separate from the above "redundant conformance" diag) is emitted in TypeChecker::checkInheritanceClause, which is called when the decl is validated – so I think we should be okay to emit a warning instead, but I'm not entirely sure.

I do agree that we should emit a warning for things like P & P, though I'm not so sure about things like P & Q where protocol Q : P {} – I think there are valid arguments for and against.

Should we open a separate thread to discuss the extent to which we should diagnose such redundancies? If possible, I would really like to keep this thread focussed on why the existing diagnostics for the following:

struct S : P, P {}
protocol Q : AnyObject, AnyObject {}
struct S1 : Any, Any {}

are errors rather than warnings, such that I can make sure https://github.com/apple/swift/pull/17425 has both correct and clean diagnostic behaviour for duplicate Any conformances.

Like Huon, I also didn't find this while searching – thanks for the link! :)