Skip to content

[6.0] SILGen: Ignore placeholders and missing methods during conformance emission #72915

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 9, 2024
Merged

[6.0] SILGen: Ignore placeholders and missing methods during conformance emission #72915

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions lib/SILGen/SILGenType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,11 +554,7 @@ class SILGenConformance : public SILGenWitnessTable<SILGenConformance> {
PrettyStackTraceConformance trace("generating SIL witness table",
Conformance);

// Check whether the conformance is valid first.
Conformance->resolveValueWitnesses();
if (Conformance->isInvalid())
return nullptr;

auto *proto = Conformance->getProtocol();
visitProtocolDecl(proto);

Expand Down Expand Up @@ -619,13 +615,12 @@ class SILGenConformance : public SILGenWitnessTable<SILGenConformance> {
return Conformance->getWitness(decl);
}

void addPlaceholder(MissingMemberDecl *placeholder) {
llvm_unreachable("generating a witness table with placeholders in it");
}

void addMissingMethod(SILDeclRef requirement) {
llvm_unreachable("generating a witness table with placeholders in it");
}
// Treat placeholders and missing methods as no-ops. These may be encountered
// during lazy typechecking when SILGen triggers witness resolution and
// discovers and invalid conformance. The diagnostics emitted during witness
// resolution should cause compilation to fail.
void addPlaceholder(MissingMemberDecl *placeholder) {}
void addMissingMethod(SILDeclRef requirement) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add asserts about that lazy type checking is enabled in these function bodies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think such an assert would be a liability in the future as we move towards more laziness by default in the compiler. In my opinion we should not assert unless we can guarantee that there is a fatal problem. We often have trouble building real-world projects with asserts compilers because of ambitious asserts, and this feels to me like it would be such an assert.

Copy link
Contributor

@nkcsgexi nkcsgexi Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels to me like it would be such an assert.

IIUC, are you saying that we may already step on the llvm_unreachable case nowadays?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm saying that I think leaving an assert in here predicated on a flag would be too fragile to be worth it. This is not the appropriate stage of compilation to decide whether or not this condition is unexpected and crash the compiler. The condition that ought to be checked is "did we fail to emit this witness and also not emit a diagnostic?". That would be better done at a later stage, where we can assume that if we've made it that far we didn't emit any fatal diagnostics during SILGen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'm just a little bit concerning about the potential loss of the qualification and documentation capability of an unreachable statement without providing alternatives back in place.


void addMethodImplementation(SILDeclRef requirementRef,
SILDeclRef witnessRef,
Expand Down