[Typechecker] Fix a crasher involving RawValue not being Equatable

[theblixguy]

This code was crashing because the RawValue did not conform to Equatable.

The diagnostic for this was in diagnoseConformanceFailure() but we didn't hit that for this case (because we didn't fail to satisfy the RawValue requirement), so I have moved it to checkEnumRawValues() which seems like a better place for it anyway.

Resolves SR-6897.

[theblixguy]

@swift-ci please smoke test

[theblixguy]

@swift-ci please smoke test

[theblixguy]

Hah, I found another bug. Consider this example:

struct S : ExpressibleByStringLiteral {
  init(stringLiteral: String) {}
}

enum E: S {
  case a
  typealias RawValue = S
  init?(rawValue: Int) { self = .a }
  var rawValue: Int { 0 }
}

This will cause the same crash in the verifier, but can be fixed if you remove the RawValue typealias (which will allow it to be inferred to be Int). This is because we don't have a witness for init(rawValue: S) and var rawValue: S.

After doing some digging, it appears that this is happening because in resolveWitnessViaLookup(), we are trying to derive a requirement, even when we already have a witness for it explicitly. In this case, we return ResolveWitnessResult::Missing and then try to lookup using the remaining strategies (derivation, default) which fails because we can't derive RawRepresentable conformance due to S's lack of conformance to Equatable.

We shouldn't be attempting to do this here if we have an explicit witness. In fact, we are correctly detecting the witness mismatch (due to TypeConflict), it just doesn't get diagnosed because we just return Missing before we get a chance to diagnose it.

I have tweaked the code to only derive it if we don't explicitly have a declaration, but let me know if you think there's a better way to handle it. I don't know if this needs to be special cased for RawRepresentable or be done generally.

[jrose-apple]

If an enum has an explicit raw type, we should probably ban having the RawRepresentable conformance using a different type. But that's technically source-breaking if anyone does something this wacky.

[theblixguy]

Yeah, I wonder how many people are actually doing that. I don't think there's any benefit if an enum inherits from type T and conforms to RawRepresentable with type U? Maybe there's some edge case that I am missing.

Anyway, as long as it don't crash the compiler it's probably fine to leave it as is for now :-) (but yeah, maybe we should revisit that in the future).

[jrose-apple]

Strictly speaking if it's an @objc enum the raw type defines your representation. But, ick.

[theblixguy]

Running a quick test to see if anything's broken and then I'll kick off source compat.

@swift-ci please smoke test

[theblixguy]

1. Failure in enum_conformance_synthesis.swift looks like an improvement (according to the note on the test) as we were still synthesising hashValue and ignoring the one that was provided (because it was not correct). Now, we diagnose it.
2. Failure in TestJSONEncoder.swift seems interesting - let me investigate. A simple struct conforming to Decodable and containing a single property should not cause a crash.

EDIT: Ah, this is because the intValue property in the struct was accidentally preventing the synthesis of the intValue for the nested CodingKey enum. Fixed it now.

[theblixguy]

@swift-ci please smoke test

[theblixguy]

@swift-ci please smoke test

[theblixguy]

@swift-ci please test source compatibility

[theblixguy]

@swift-ci please test

[theblixguy]

@swift-ci please test source compatibility

[theblixguy]

Failure on macOS is because SwiftSyntax is generating an enum case called rawValue in SyntaxNodes.swift.gyb, which prevents RawRepresentable synthesis because we now diagnose it (candidate is not a variable).

I think this is also an improvement, because we already do the same for:

enum Foo: RawRepresentable {
  typealias RawValue = Int
  case rawValue // error: candidate is not a variable
}

@slavapestov do you mind giving this PR another look? Do you have any suggestions? The change in resolveWitnessViaLookup is because we are ignoring provided witnesses when they are not valid and generating new ones via derivation (which leads to a crash in some cases when the derivation also fails).

[jrose-apple]

Failure on macOS is because SwiftSyntax is generating an enum case called rawValue in SyntaxNodes.swift.gyb, which prevents RawRepresentable synthesis because we now diagnose it (candidate is not a variable).

Wasn't this supposed to be filtered out because cases are static members and not instance members? In your other case there's no way to synthesize a rawValue because the cases don't have any association with Ints, but for an enum-with-raw-type there still is.

[theblixguy]

I haven't added any filtering logic - at the moment its quite simple as it's only doing a name lookup to see if a declaration already exists before deciding whether we should derive a requirement or not. In this case, there's a lookup to check 'rawValue' but it doesn't check whether its an enum case or an instance property. Perhaps the check needs to be smarter -

1. We could go beyond a simple name lookup check and also compare other properties (like static-ness, optionality, etc) but we would then be duplicating logic from matchWitness().
2. We could not derive anything if any of the matches aren't viable/are missing, but there needs to be logic to determine which matches are acceptable (or whether a "no match" is acceptable) and which matches should be diagnosed. Ex: we might not have a match for hashValue so that shouldn't be diagnosed, but derived. Although, if we already have one and it doesn't match the requirement then it should be diagnosed.

What do you think? Is there a better way to do this?

[jrose-apple]

lookupDirect isn't necessarily correct anyway, since that won't look into extensions. If you go through the normal qualified lookup path maybe we'll get the synthesis we need anyway.

[theblixguy]

Hmm I thought it did when I looked at the implementation, but alright, I'll do it the other way!

[jrose-apple]

It's definitely not okay to regress RawRepresentable synthesis when there's a case called rawValue.

[theblixguy]

Sure, let me do this another way then because only name lookup isn't going to work here if we want to fix the above scenario.