[SR-13119] Confusing diagnostic when checking conformance for IUO of generic parameter

[typesanitizer]

Previous ID	SR-13119
Radar	rdar://problem/64953106
Original Reporter	@typesanitizer
Type	Bug
Status	Closed
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, StarterBug
Assignee	srinikhil07 (JIRA)
Priority	Medium

md5: cdcccb4342b92c089c6823747fe74efe

Issue Description:

Consider the following code:

struct S<T>: Codable {
  var s: T!
} 

With Xcode 12 beta 1, this generates the following diagnostic:

tmp.swift:1:8: error: type 'S' does not conform to protocol 'Decodable'
struct S<T>: Codable {
       ^
Swift.Decodable:2:5: note: protocol requires initializer 'init(from:)' with type 'Decodable'
    init(from decoder: Decoder) throws
    ^
tmp.swift:2:9: note: cannot automatically synthesize 'Decodable' because 'T?' does not conform to 'Decodable'
    var x: T!
        ^
tmp.swift:1:8: error: type 'S' does not conform to protocol 'Encodable'
struct S<T>: Codable {
       ^
Swift.Encodable:2:10: note: protocol requires function 'encode(to:)' with type 'Encodable'
    func encode(to encoder: Encoder) throws
         ^
tmp.swift:2:9: note: cannot automatically synthesize 'Encodable' because 'T?' does not conform to 'Encodable'
    var x: T!
        ^ 

The diagnostic says T? does not conform to Encodable but it should be saying T.

At the moment, the code example crashes with master, which is tracked in SR-13117.

[typesanitizer]

@swift-ci create

[typesanitizer]

@swift-ci create

[CodaFi]

Should be a simple matter of printing a TypeLoc instead of just a Type. Tagging this as a Starter Bug. If anybody wants to fix this, reply here for more details.

[swift-ci]

Comment by Nikhil (JIRA)

I would like to.

[CodaFi]

Great! So, you'll need to go update the two uses of `diag::codable_non_conforming_property_here` in the compiler, and its diagnostic argument string in DiagnosticsSema.def. In order to fix this bug, we need to make those diagnostics print exactly what the user wrote, so instead of using a Type, we're going to use an syntactic equivalent called a TypeRepr. These are created by the parser from raw source text. But, there's still the possibility we could be dealing with types that are not from a parsed representation - for example, what if we loaded S<T> from a swiftmodule? In that case, we ought to fall back to printing the semantic Type again. If that sounds complex, luckily we have an abstraction in the compiler for dealing with exactly this scenario called a TypeLoc. You give it a (possibly null) TypeRepr and a (usually not null) Type and when it's handed as an argument to a diagnostic, it'll print the parsed representation if it can get it, otherwise it'll print the type. This'll also handle the general case of doing stuff like trying to print a variable whose type is not given because it's inferred from its initializer.

Anyways, look in DiagnosticsSema.def for codable_non_conforming_property_here, and change its argument types to `(Type, TypeLoc)`, then in DerivedConformanceCodable.cpp, you'll need to change both usages of it to take TypeLocs. So, for the second usage on line 356, we want to change this

      varDecl->diagnose(diag::codable_non_conforming_property_here,
                          derived.getProtocolType(), varDecl->getType());

into

        TypeLoc typeLoc = {
          varDecl->getTypeReprOrParentPatternTypeRepr(),
          varDecl->getType(),
        };
        varDecl->diagnose(diag::codable_non_conforming_property_here,
                          derived.getProtocolType(), typeLoc);

Once you've got both callsites updated, ping me here and we can talk about writing tests.

[swift-ci]

Comment by Nikhil (JIRA)

@CodaFi

I have fixed it like you suggested.

1. I have changed def and cpp files,

2. For tests, I ran grep on tests folder and it gave me some results.

   grep -rn "cannot automatically synthesize" test | grep "does not conform"

   Thanks.

[swift-ci]

Comment by Nikhil (JIRA)

I have added this PR #32611 and I can now successfully debug. I also get the below error now.

cannot automatically synthesize 'Decodable' because 'T!' does not conform to 'Decodable' 
      var s: T! 
             ^
cannot automatically synthesize 'Encodable' because 'T!' does not conform to 'Encodable'
   var s: T!
          ^

[typesanitizer]

That looks correct. Please mention "Fixes [rdar://problem/64953106]. Fixes SR-13119." (for cross-referencing) in the PR description when you open your PR with the fix. 🙂

[swift-ci]

Comment by Nikhil (JIRA)

theindigamer (JIRA User)

I am trying to run tests locally. I get below error for any test I run using lit.

~/Code/swift-source/build/Xcode-RelWithDebInfoAssert/swift-macosx-x86_64/test-macosx-x86_64/Parse/Output/weakLinked.swift.script: line 1: swift-frontend: command not found

[typesanitizer]

srinikhil07 (JIRA User), I suspect the issue might be that you haven't updated your compiler recently. You might need to rebase on top of recent changes. The swift-frontend target was newly added IIUC. I don't think this issue is related to this particular bug report; so it would be generally better to start a Swift forums thread in the Development:Compiler category to discuss issues encountered when building/running tests, so that (a) other people can see the post and try to help out and (b) it can more easily be discovered by someone else in case they hit the same issue.

[swift-ci]

Comment by Nikhil (JIRA)

theindigamer (JIRA User)

Thanks for the direction, I did a search in forums and I came across this discussion https://forums.swift.org/t/running-swift-standard-library-tests/17230/9

Seems with Xcode build alone lit doesn't work and we need ninja. If so, I think we can state this explicitly in docs.

[swift-ci]

Comment by Nikhil (JIRA)

theindigamer (JIRA User)

I ran the tests for 'codable' with lit using below command:

swift-source/llvm-project/llvm/utils/lit/lit.py -s -vv swift-source/build/Ninja-ReleaseAssert/swift-macosx-x86_64/test-macosx-x86_64 --filter "codable"

I got 5 failed tests:

Two tests are due to 'AnyHashable?' in tests we we now print as 'AnyHashable!'. I corrected them,

Two tests are for nested class/struct(e.g.,test/decl/protocol/special/coding/struct_codable_failure_diagnostics.swift), where the original vs new error is as below. Please suggest if the new diagnostic is okay or should we change anything?

cannot automatically synthesize 'Encodable' because 'C1.Nested' does not conform to 'Encodable'

cannot automatically synthesize 'Encodable' because 'Nested' does not conform to 'Encodable'

// Codable class with non-Codable property.
class C1 : Codable {
// expected-error@-1 {{type 'C1' does not conform to protocol 'Decodable'}}
// expected-error@-2 {{type 'C1' does not conform to protocol 'Encodable'}}

classNested{}
var a:String=""
var b:Int=0
var c: Nested =Nested()
// expected-note@-1 {{cannot automatically synthesize 'Decodable' because 'C1.Nested' does not conform to 'Decodable'}}
// expected-note@-2 {{cannot automatically synthesize 'Encodable' because 'C1.Nested' does not conform to 'Encodable'}}
}

One in test/decl/protocol/special/coding/struct_codable_simple.swift, the difference is as below. Please suggest if this is also okay or any changes needed.
cannot automatically synthesize 'Encodable' because '<<error type>>' does not conform to 'Encodable'
cannot automatically synthesize 'Encodable' because 'Int' does not conform to 'Encodable'

  // rdar://problem/59655704  
  struct SR_12248_1: Codable { // expected-error `type 'SR_12248_1' does not conform to protocol 'Encodable'`  
      var x:Int// expected-note {{'x' previously declared here}}  
      var x:Int// expected-error {{invalid redeclaration of 'x'}}  
     // expected-note@-1 {{cannot automatically synthesize 'Encodable' because '\<\<error type\>\>' does not conform to 'Encodable'}}  
     // expected-note@-2 {{cannot automatically synthesize 'Encodable' because '\<\<error type\>\>' does not conform to 'Encodable'}}  
  }  

Also, since our case is covered already should I write a new test case?

[LucianoPAlmeida]

 \>  One in test/decl/protocol/special/coding/struct_codable_simple.swift, the difference is as below. Please suggest if this is also okay or any changes needed.

Hey srinikhil07 (JIRA User) for the last <<error type>> diagnostics we have a PR open for that #32371

So I think you don't need to worry too much about that one 🙂

cc @CodaFi

[typesanitizer]

> Two tests are for nested class/struct(e.g.,test/decl/protocol/special/coding/struct_codable_failure_diagnostics.swift), where the original vs new error is as below. Please suggest if the new diagnostic is okay or should we change anything?

One possible change here might be to check if there is an IUO. Based on that, call Type::getString with PrintOptionalAsImplicitlyUnwrapped field set to true or false (cc @hborla, she suggested using this field). Unlike TypeRepr, Type will print out the fully qualified thing, so it will avoid this regression.

[swift-ci]

Comment by Nikhil (JIRA)

theindigamer (JIRA User)

Changes done. Will raise the pull request.