check: fix false positive when an interface implements a sum type #12
Conversation
Sometimes it is possible for a sum type interface to be implemented by another interface. For example, we have a sum type called T including 3 variants, A, B, C. We also have an U interface embedding T and thus implementing T. Assuming that B and C implement U, a type switch statement can be considered exhaustive if all of A, B, C are listed. It should also be considered exhaustive if only A and U are listed. However, go-sumtype does not distinguish between concrete and interface types. It fails in both cases, requiring all of A, B, C, U to be listed. It is unlikely to code to be written in this way. U already covers B and C, and it is unnecessary to list them in a type switch statement. This commit fixes the problem by handling interfaces specially. Closes: BurntSushi#1 Closes: BurntSushi#3
| } | ||
| switch T(nil).(type) { | ||
| case *A, U: | ||
| } |
There was a problem hiding this comment.
So this test case is somewhat interesting. Your argument is compelling, but I don't think I'm quite 100% convinced yet. The problem, as I see it, is that both of these type switch statements, when used in real code, could execute without a single case firing. For example, in the former case, if x.(type) is a U, then none of the *A, *B or *C cases will catch it. Similarly, in the latter case, if x.(type) is a *B or a *C, then neither the *A or U cases will catch it. So basically, what you wind up with is a type switch that is not actually exhaustive. To me, this seems like a fundamental violation of what this tool is trying to do. That is, if this tool passes, then your switch statements should either be exhaustive for all sum types, or explicitly non-exhaustive with a default case.
On the other hand, I do agree that writing code that handles all of *A, *B, *C and U is likely not common and probably not what you want. But this change overall weakens what "exhaustiveness" actually means. So I'm not sure what to do. :-/
There was a problem hiding this comment.
So this test case is somewhat interesting. Your argument is compelling, but I don't think I'm quite 100% convinced yet. The problem, as I see it, is that both of these type switch statements, when used in real code, could execute without a single case firing.
Since T is sealed, U is also sealed if it implements T. All concrete types implementing U must also be defined in the same package. Therefore, U is a sum type, and U is a subset of T.
For example, in the former case, if
x.(type)is aU, then none of the*A,*Bor*Ccases will catch it.
If U implements T, a U is either *B, *C, or nil. The only missing case here is nil and listing U again does not help.
If U does not implement T, the type switch is still exhaustive because types implementing U but do not implementing T cannot be stored in a T. Therefore, listing *B and *C is sufficient.
Similarly, in the latter case, if
x.(type)is a*Bor a*C, then neither the*AorUcases will catch it.
Since *B and *C implement U, *B or *C must match U regardless of whether U implements T.
So basically, what you wind up with is a type switch that is not actually exhaustive. To me, this seems like a fundamental violation of what this tool is trying to do. That is, if this tool passes, then your switch statements should either be exhaustive for all sum types, or explicitly non-exhaustive with a
defaultcase.
I think a type switch is still exhaustive in both cases, but I agree it may look confusing if U does not implement T. Do you think it will look better if we require U to implement T?
|
Ping again ... I hope you have seen my reply. Do you have any new comment? There has been no activity on the pull request for 3 weeks. |
|
Because I'm busy. Please be patient. I don't have a reply yet because I need to try this out for myself. |
Update golang.org/x/tools to the latest version to fix internal errors when using Go 1.14. Other dependencies are updated as well.
Fix internal errors when using Go 1.18.
gofmt in Go 1.19 formats '//go-sumtype:decl' as '// go-sumtype:decl' because '-' is not allowed in the tool name part of a directive comment, breaking all sum type declarations after files are formatted. To keep the tool working with Go 1.19, we have to support an alternative form of declaration. We choose '//gosumtype:decl' by dropping '-' from the tool name.
Sometimes it is possible for a sum type interface to be implemented by
another interface. For example, we have a sum type called T including
3 variants, A, B, C. We also have an U interface embedding T and thus
implementing T. Assuming that B and C implement U, a type switch
statement can be considered exhaustive if all of A, B, C are listed.
It should also be considered exhaustive if only A and U are listed.
However, go-sumtype does not distinguish between concrete and interface
types. It fails in both cases, requiring all of A, B, C, U to be listed.
It is unlikely to code to be written in this way. U already covers B and
C, and it is unnecessary to list them in a type switch statement. This
commit fixes the problem by handling interfaces specially.
Closes: #1
Closes: #3