Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-5215] Generated CodingKeys type is not available in function signatures #47791

Closed
swift-ci opened this issue Jun 14, 2017 · 14 comments
Closed

[SR-5215] Generated CodingKeys type is not available in function signatures #47791

swift-ci opened this issue Jun 14, 2017 · 14 comments
Assignees

Comments

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Jun 14, 2017

Previous ID SR-5215
Radar rdar://problem/32774780
Original Reporter edwardaux (JIRA User)
Type Bug
Status Resolved
Resolution Done
Environment

Xcode Version 9.0 beta (9M136h)
Apple Swift version 4.0 (swiftlang-900.0.43 clang-900.0.22.8)

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug
Assignee @itaiferber
Priority Medium

md5: 57756000cdaf8b024f7be02417094e67

Issue Description:

Referencing the generated CodingKeys enum compiles just fine within a method body. For example:

struct Working : Codable {
    let prop: String
    
    func blah() {
        let key: Working.CodingKeys = .prop
        print(key)
    }
}

However, if you try to include the CodingKeys type in a function signature, you get a compiler error saying 'CodingKeys' is not a member type of 'Broken'. For example, the following code doesn't compile:

struct Broken : Codable {
    let prop: String
    
    func blah(key: Broken.CodingKeys) {
    }
}

You can work around the compile error by explicitly declaring the CodingKeys enum like so:

struct Broken : Codable {
    let prop: String
    
    enum CodingKeys: String, CodingKey {
        case prop
    }
    func blah(key: Broken.CodingKeys) {
    }
}
@belkadan
Copy link
Contributor

@belkadan belkadan commented Jun 14, 2017

cc @itaiferber

This is because the CodingKeys enum isn't synthesized until the Codable conformance is checked. We should be handling this in TypeChecker::lookupMemberType, though.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 14, 2017

@swift-ci Create

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 28, 2017

@belkadan Would adding `NL_ProtocolMembers` to the qualified lookup in `lookupMemberType` normally address this?

There's the following note in `NL_Options`:

/// The default set of options used for qualified name lookup.
///
/// FIXME: Eventually, add NL_ProtocolMembers to this, once all of the
/// callers can handle it.
NL_QualifiedDefault = NL_RemoveNonVisible | NL_RemoveOverridden,

but unfortunately, turning `NL_ProtocolMembers` on by default prevents the stdlib from compiling... (There are a ton of ambiguous name lookups in `String` views.)

@belkadan
Copy link
Contributor

@belkadan belkadan commented Jun 29, 2017

No, that's for actually looking into protocols themselves, not members synthesized as part of checking conformances. It's not clear whether the FIXME is still what we want anyway.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 29, 2017

@belkadan Got it, makes sense. Is it reasonable, then, to expect to be able to do eager conformance checking (to force protocol conformance synthesis) before member lookup, a la:

LookupTypeResult TypeChecker::lookupMemberType(DeclContext *dc,
                                               Type type, Identifier name,
                                               NameLookupOptions options) {
  if (auto *decl = type->getAnyNominal()) {
    checkConformancesInContext(decl, decl);
  }

  // ...
}

I think we'd both prefer to make this fix not be Codable-specific, but the above fails sema in several places (the stdlib fails to compile). I'd rather not have to inspect the name to see if it's one of the known identifiers related to the Codable API...

@belkadan
Copy link
Contributor

@belkadan belkadan commented Jun 29, 2017

That seems really quite expensive. There's a section below marked

    // We couldn't find any normal declarations. Let's try inferring
    // associated types.

which is supposed to handle this, but it'll only work when the caller passes NameLookupFlags::ProtocolMembers. But we seem to be doing that in most cases, specifically resolveNestedIdentTypeComponent (which is what would be used here).

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 29, 2017

@belkadan In this case we never even reach there, though. The qualified lookup fails since the type doesn't yet exist, so we bail almost immediately:

  if (!dc->lookupQualified(type, name, subOptions, this, decls))
    return result;

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 29, 2017

Since CodingKeys is not a protocol requirement, BTW, there's nothing in place to force any sort of synthesis during qualified lookup (since even when NL_ProtocolMembers is on, it's not a member of either Encodable or Decodable). If eagerly forcing synthesis always isn't a good idea, we can do this specifically in the case of CodingKeys; it just feels a bit gross.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 29, 2017

I guess it's somewhat reasonable, though. Since CodingKeys is essentially lazily synthesized (in the sense that it's only present once either Encodable or Decodable conformance is verified), in the case where we ask for it eagerly (like here), we need to resolve it forcibly.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 29, 2017

For instance, the following allows lookup to succeed:

  auto lookupSuccess = dc->lookupQualified(type, name, subOptions, this, decls);
  if (!lookupSuccess) {
    // If the requested type is a CodingKeys type on something which may require
    // Encodable/Decodable synthesis, it may not have been synthesized yet. If
    // this is the case, force synthesis and attempt lookup again.
    NominalTypeDecl *targetDecl = nullptr;
    if (name == dc->getASTContext().Id_CodingKeys &&
        (targetDecl = type->getAnyNominal())) {
      checkConformancesInContext(targetDecl, targetDecl);
      lookupSuccess = dc->lookupQualified(type, name, subOptions, this, decls);
    }
  }

  if (!lookupSuccess)
    return result;

@belkadan
Copy link
Contributor

@belkadan belkadan commented Jun 29, 2017

Ah, I forgot that CodingKeys wasn't a requirement of anything. That explains why this is special.

Rather than trying to check all conformances, what do you think about asking conformsToProtocol(Encodable) and conformsToProtocol(Decodable)? That should be less work.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 29, 2017

@belkadan Sounds good — that's what I've put together so far. PR should be up soon with some testing.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jun 30, 2017

Up against master at PR-10723

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Jan 16, 2018

Forgot to resolve this issue — this went in with PR#10930 a while back.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants