@itaiferber, any idea what's going on here? I know we've had problems here but I didn't expect crashes!

@belkadan I'm not sure, to be honest. This is the first I've seen of this, so I'll investigate. It looks like inheriting the superclass's method is somehow invalid? That seems really strange to me.

@swift-ci Create

Comment by Ammo Goettsch (JIRA)

I saw it again in another place in the code yesterday, where I was inheriting a class. Once I ported the base class code to use Codable (with some synthesized code), it started calling the wrong method in the derived class in a test (i.e. wrong offset again, this time hitting an actual method rather than the end of the table.) So I don't think it as rare as I thought.

Just to double-check, you are rebuilding the test after changing the codable classes, right?

Comment by Ammo Goettsch (JIRA)

Oh yes: clean, exit Xcode, rm -rf ~/Library/Developer/Xcode/DerivedData

I even sometimes reboot to clear out /var/folders where Xcode makes a mess... but I can't swear I have done that for this test. The attached test code should produce the problem though for you. Let me know if it does not repro and I can work with you to find the settings I might be using differently or send you a project file.

@belkadan Thanks for consolidating! Who might best be able to help me investigate what's happening with the vtable here (or help point in the right direction)? Encodable synthesis isn't doing anything special and follows the same model as Hashable and Equatable.

Encodable is special in that it is currently the only protocol for which we synthesize methods in class types. I'm also interested in finding a good solution to this – I ran into the same issue when I tried adding limited support for classes for Hashable synthesis in PR #15122.

I shoved @slavapestov at one of the dups but I don't know if he's started looking yet. In our brief conversations about it we haven't thought of anything better than "if you use a class, you must validate its Hashable conformance".

@belkadan What do you mean by "validate its conformance"? Don't we already have passes that validate the decls? Or is this at a different layer/level?

I mean it's not good enough to know that the class declares conformance to Hashable; we need to actually go check that it does so correctly, because that's what forces the new overridable declarations to be synthesized.

FWIW, I went for a little Mr Magoo-style adventure in the type checker, and I was excited to stumble upon TypeChecker::handleExternalDecl. Unfortunately I was distracted by Other Stuff before I had time to break anything. 🙂

@belkadan That's what I'm saying — the type checker already has passes that validate conformances; we do that on the second pass:

// lib/Sema/TypeCheckDecl.cpp:4631
void visitClassDecl(ClassDecl *CD) {
  TC.checkDeclAttributesEarly(CD);
  TC.computeAccessLevel(CD);

  if (!IsSecondPass) {
    checkUnsupportedNestedType(CD);

    TC.validateDecl(CD);
    if (!CD->hasValidSignature())
      return;

    TC.requestSuperclassLayout(CD);
    TC.DeclsToFinalize.remove(CD);

    {
      // Check for circular inheritance.
      SmallVector<ClassDecl *, 8> path;
      path.push_back(CD);
      checkCircularity(TC, CD, diag::circular_class_inheritance,
                       diag::class_here, path);
    }
  }

  // If this class needs an implicit constructor, add it.
  if (!IsFirstPass)
    TC.addImplicitConstructors(CD);

  CD->addImplicitDestructor();

  if (!IsFirstPass && !CD->isInvalid())
    TC.checkConformancesInContext(CD, CD);

  for (Decl *Member : CD->getMembers())
    visit(Member);

  // <snip>

  TC.checkDeclAttributes(CD);
}

Or am I still misunderstanding?

(I'd try to step through here but unfortunately my machine can't seem to build Swift at the moment)

@itaiferber I believe external decls aren't fully type checked.

@lorentey Ah; that might do it. If the class is defined in a separate file it's considered "external" and thus might not be fully validated?

Right, anything that has to do with a "pass" only applies to declarations in the current file. validateDecl doesn't normally go through that.

(…except for some types where it does, because past Jordan never finished factoring it out.)

@lorentey: That's about declarations imported from Clang, not declarations in other files.

Comment by Ammo Goettsch (JIRA)

As an aside: There could be other cases though where this is not happening. Shouldn't the compiler create an error instead of emitting code that ends up accessing effectively vtable[-1] ? It would be nice if the compiler could catch future instances of these incomplete types before you execute the code in the field...

The compiler can't know that something that doesn't exist is supposed to exist. If it did know that, it could just do the creating itself.

(It's most likely not doing vtable[-1]; it'd be doing vtable[2] when it should be doing vtable[3].)

Comment by Ammo Goettsch (JIRA)

Read my original debugging in this bug report. It is fetching the vtable size, instead of one of the entries. So either some index is initialized to -1, or the vtable stores its size in slot 0 (didn't check the compiler code.) If this was a normal vtable error where it is getting the wrong function because it disagrees on layout with another module, then you would be correct. But this is an out of bounds access. I even messed with the classes I used for testing to try different vtable sizes to confirm it is always accessing one word to the "left" of the vtable entries and getting the table size.

Just to reiterate: then it jumps to the table size interpreted as an address. The correct function it SHOULD have called is in fact in the vtable, as the first entry stored one word to the right of that. So it isn't missing the function either.

#16057