RFC: Make `DiagnosticSeverity` conform to `Equatable`

public enum DiagnosticSeverity: Sendable, Equatable {
  case error
  case warning
  case note
  case remark
}
  • Commonality: Do you expect this API to be widely used? Is it applicable in a variety of contexts?
  • Not trivially composable/Readability: Users could check for cases using if case before but using == is more convenient
5 Likes

Maybe widely used when macros become more mainstream or when even more functionality/complexity is unlocked for them. Regardless, I see conforming DiagnosticSeverity to Equatable as a beneficial addition since I'd rather use == instead of if case to check equality.

1 Like

Would you ever see a future situation where you would wish this type was also Hashable? Would it make sense to go ahead with Hashable now?

I do think it being Hashable now would be better as I do see future situations where I would use it indirectly and directly depending on what the macro's functionality and complexity should be.

1 Like

Aren't payload-less enums automatically Hashable?

That looks to be true implicitly, and is present in the generated assembly.

I think enums are only automatically Hashable if they are internal (maybe package), not if they are public.

That’s a great suggestion, @RandomHashTags. I’ll update the PR.

1 Like

I know that was proposed at one point, but I don't remember that we ever actually made that change. As far as I know the implicit Equatable and Hashable conformances on payload-less enums are still public.

Oh, I understand now: Equatable and Hashable are only inferred when building without library evolution enabled or when the enum is marked as @frozen. `DiagnosticSeverity` should conform to `Equatable` · Issue #2964 · swiftlang/swift-syntax · GitHub was reported from developers of swift-testing, which use a version of swift-syntax that has library evolution enabled because they are in the toolchain, so they don’t receive the automatic Equatable and Hashable conformance. So, this change only really affects them.

Hmm… are there any kind of edge-casey situations where code that did depend on the implicit conformance would now behave unexpected with the explicit conformance? I can't think of anything bad happening… were there any unit tests from before this diff that took advantage of the implicit Hashable?

No, swift-testing just hasn’t been able to use DiagnosticSeverity as an Equatable or Hashable type.

1 Like

(Which we'd like to do!)

3 Likes

I was thinking more about Equatable or Hashable assumptions outside of swift-testing.

For example… how is this check for value equality performed?

That code lives within the same module that defines DiagnosticsSeverity. It’s just that the implicit Hashable conformance is not exported in the public interface when enabling library evolution.

Right. That makes sense. Expanding on that idea could then be:

  • If library evolution is not enabled and…
  • Implicit Equatable or Hashable is assumed outside of swift-syntax…
  • Does the outside code depending on the implicit conformance behave unexpectedly now that conformance is explicit?

For example… is the Xcode source code putting these DiagnosticsSeverity values in a Set or Dictionary before drawing UI to display to the user?

I can't imagine any reason why this should break… and I would assume this came up when the decision to implement implicit conformance was first discussed (the "forward compatibility" with explicit conformance).

But this might all be super edge-casey… I don't have a strong opinion that this should block landing.

Yes, the only libraries that benefit from this change are ones that rely on a version of swift-syntax that has been built with library evolution enabled. All usages within swift-syntax or from packages that rely on swift-syntax without library evolution enabled (such as a SwiftPM package dependency), already assume that DiagnosticSeverity is Hashable.

So, this change is a no-op for most clients and aligns the library-evolution version closer with the non-library-evolution version.

1 Like

SGTM. Thanks!

Just curious, but is this expected? I don't see anything in the library-evolution proposal: swift-evolution/proposals/0260-library-evolution.md at main · swiftlang/swift-evolution · GitHub

Yes, my understanding is that this is expected behavior.

1 Like