Skip to content

[Sema] Check that at least one buildBlock is callable given arg labels #33885

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

Closed
wants to merge 1 commit into from
Closed

[Sema] Check that at least one buildBlock is callable given arg labels #33885

Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5309,6 +5309,9 @@ NOTE(function_builder_non_static_buildblock, none,
NOTE(function_builder_buildblock_enum_case, none,
"enum case 'buildBlock' cannot be used to satisfy the function builder "
"requirement", ())
NOTE(function_builder_buildblock_arg_labels, none,
"potential match 'buildBlock' can never be called implicitly because it "
"has at least one labeled argument", ())
NOTE(function_builder_buildblock_not_static_method, none,
"potential match 'buildBlock' is not a static method", ())

Expand Down
78 changes: 55 additions & 23 deletions lib/Sema/BuilderTransform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1881,51 +1881,83 @@ std::vector<ReturnStmt *> TypeChecker::findReturnStatements(AnyFunctionRef fn) {

bool TypeChecker::typeSupportsBuilderOp(
Type builderType, DeclContext *dc, Identifier fnName,
ArrayRef<Identifier> argLabels, SmallVectorImpl<ValueDecl *> *allResults) {
ArrayRef<Identifier> argLabels, SmallVectorImpl<FuncDecl *> *matches,
SmallVectorImpl<BuilderOpMismatch> *nearMisses) {

auto recordNearMiss = [&](ValueDecl *vd, BuilderOpMismatchKind kind) {
if (nearMisses)
nearMisses->emplace_back(vd, kind);
};

bool foundMatch = false;
SmallVector<ValueDecl *, 4> foundDecls;
dc->lookupQualified(
builderType, DeclNameRef(fnName),
NL_QualifiedDefault | NL_ProtocolMembers, foundDecls);
for (auto decl : foundDecls) {
if (auto func = dyn_cast<FuncDecl>(decl)) {
// Function must be static.
if (!func->isStatic())
continue;
if (isa<EnumElementDecl>(decl)) {
recordNearMiss(decl, BuilderOpMismatchKind::EnumCase);
continue;
} else if (!isa<FuncDecl>(decl)) {
recordNearMiss(decl, BuilderOpMismatchKind::Other);
continue;
}

// Function must have the right argument labels, if provided.
if (!argLabels.empty()) {
auto funcLabels = func->getName().getArgumentNames();
if (argLabels.size() > funcLabels.size() ||
funcLabels.slice(0, argLabels.size()) != argLabels)
continue;
auto *func = cast<FuncDecl>(decl);
// Function must be static.
if (!func->isStatic()) {
recordNearMiss(func, BuilderOpMismatchKind::InstanceMethod);
continue;
}

// Function must have the right argument labels, if provided.
if (!argLabels.empty()) {
auto funcLabels = func->getName().getArgumentNames();
if (argLabels.size() > funcLabels.size() ||
funcLabels.slice(0, argLabels.size()) != argLabels) {
Copy link
Member

Choose a reason for hiding this comment

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

This is an approximation of the rule used to actually match call arguments. Do you want to go all the way and use matchCallArguments to get it exactly right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that makes sense for most of the build* functions (since the transform introduces a fixed number of arguments), but I don't think it would work for buildBlock since we won't know the correct number of arguments until we're actually evaluating a call, and every call to buildBlock may differ in the number of arguments.

It seems to me that, given that buildBlock calls may have an unbounded number of arguments (of any type), the best we can do when evaluating the buildBlock declaration is make sure that there are no required labeled arguments. If I'm missing something let me know!

recordNearMiss(func, BuilderOpMismatchKind::ArgumentLabelMismatch);
continue;
}
}

foundMatch = true;
break;
// After the labels provided by the builder transform, any additional
// labeled parameters must have a default value (or else the function will
// be uncallable by the builder transformation).
auto params = func->getParameters()->getArray();
auto isLabeledUndefaulted = [&](ParamDecl *param) -> bool {
return !param->isDefaultArgument() && !param->getArgumentName().empty();
};
if (llvm::any_of(params.drop_front(argLabels.size()),
isLabeledUndefaulted)) {
recordNearMiss(func,
BuilderOpMismatchKind::ExtraneousUndefaultedArgument);
continue;
}
}

if (allResults)
allResults->append(foundDecls.begin(), foundDecls.end());
foundMatch = true;

// If we're not recording any candidates or matches, we can bail out as soon
// as we find the first match.
if (!nearMisses && !matches)
break;

if (matches)
matches->push_back(func);
}

return foundMatch;
}

Type swift::inferFunctionBuilderComponentType(NominalTypeDecl *builder) {
Type componentType;

SmallVector<ValueDecl *, 4> potentialMatches;
SmallVector<FuncDecl *, 4> matches;
ASTContext &ctx = builder->getASTContext();
bool supportsBuildBlock = TypeChecker::typeSupportsBuilderOp(
builder->getDeclaredInterfaceType(), builder, ctx.Id_buildBlock,
/*argLabels=*/{}, &potentialMatches);
/*argLabels=*/{}, &matches);
if (supportsBuildBlock) {
for (auto decl : potentialMatches) {
auto func = dyn_cast<FuncDecl>(decl);
if (!func || !func->isStatic())
continue;

for (auto func : matches) {
// If we haven't seen a component type before, gather it.
if (!componentType) {
componentType = func->getResultInterfaceType();
Expand Down
46 changes: 31 additions & 15 deletions lib/Sema/TypeCheckAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3022,10 +3022,10 @@ void AttributeChecker::visitPropertyWrapperAttr(PropertyWrapperAttr *attr) {

void AttributeChecker::visitFunctionBuilderAttr(FunctionBuilderAttr *attr) {
auto *nominal = dyn_cast<NominalTypeDecl>(D);
SmallVector<ValueDecl *, 4> potentialMatches;
SmallVector<TypeChecker::BuilderOpMismatch, 4> nearMisses;
bool supportsBuildBlock = TypeChecker::typeSupportsBuilderOp(
nominal->getDeclaredType(), nominal, D->getASTContext().Id_buildBlock,
/*argLabels=*/{}, &potentialMatches);
/*argLabels=*/{}, nullptr, &nearMisses);

if (!supportsBuildBlock) {
{
Expand All @@ -3038,7 +3038,7 @@ void AttributeChecker::visitFunctionBuilderAttr(FunctionBuilderAttr *attr) {
Type componentType;
std::tie(buildInsertionLoc, stubIndent, componentType) =
determineFunctionBuilderBuildFixItInfo(nominal);
if (buildInsertionLoc.isValid() && potentialMatches.empty()) {
if (buildInsertionLoc.isValid() && nearMisses.empty()) {
std::string fixItString;
{
llvm::raw_string_ostream out(fixItString);
Expand All @@ -3054,19 +3054,35 @@ void AttributeChecker::visitFunctionBuilderAttr(FunctionBuilderAttr *attr) {

// For any close matches, attempt to explain to the user why they aren't
// valid.
for (auto *member : potentialMatches) {
if (member->isStatic() && isa<FuncDecl>(member))
continue;
for (auto nearMiss : nearMisses) {
auto *member = nearMiss.first;
auto note = diag::function_builder_buildblock_not_static_method;
std::function<void(InFlightDiagnostic &)> fixIt;

switch (nearMiss.second) {
case TypeChecker::BuilderOpMismatchKind::InstanceMethod:
if (member->getDeclContext()->getSelfNominalTypeDecl() == nominal) {
note = diag::function_builder_non_static_buildblock;
fixIt = [&](InFlightDiagnostic &diag) {
diag.fixItInsert(member->getAttributeInsertionLoc(true), "static ");
};
}
break;
case TypeChecker::BuilderOpMismatchKind::EnumCase:
note = diag::function_builder_buildblock_enum_case;
break;
case TypeChecker::BuilderOpMismatchKind::ExtraneousUndefaultedArgument:
note = diag::function_builder_buildblock_arg_labels;
break;
case TypeChecker::BuilderOpMismatchKind::Other:
break;
case TypeChecker::BuilderOpMismatchKind::ArgumentLabelMismatch:
llvm_unreachable("buildBlock should not check argument labels");
}

if (isa<FuncDecl>(member) &&
member->getDeclContext()->getSelfNominalTypeDecl() == nominal)
diagnose(member->getLoc(), diag::function_builder_non_static_buildblock)
.fixItInsert(member->getAttributeInsertionLoc(true), "static ");
else if (isa<EnumElementDecl>(member))
diagnose(member->getLoc(), diag::function_builder_buildblock_enum_case);
else
diagnose(member->getLoc(),
diag::function_builder_buildblock_not_static_method);
auto diag = diagnose(member->getLoc(), note);
if (fixIt)
fixIt(diag);
}
}
}
Expand Down
35 changes: 30 additions & 5 deletions lib/Sema/TypeChecker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,12 +1222,37 @@ bool requireArrayLiteralIntrinsics(ASTContext &ctx, SourceLoc loc);
/// an \c UnresolvedMemberExpr, \c nullptr is returned.
UnresolvedMemberExpr *getUnresolvedMemberChainBase(Expr *expr);

enum class BuilderOpMismatchKind : unsigned {
/// The potential match was an instance method rather than a static method.
InstanceMethod,

/// The potential match was an enum case.
EnumCase,

/// The potential match did not have the correct argument labels.
ArgumentLabelMismatch,

/// The potential match had an argument that would not be provided by the
/// function builder transformation.
ExtraneousUndefaultedArgument,

/// Catchall for mismatches which could not be classified more specifically.
Other
};

using BuilderOpMismatch = std::pair<ValueDecl *, BuilderOpMismatchKind>;

/// Checks whether a function builder type has a well-formed function builder
/// method with the given name. If provided and non-empty, the argument labels
/// are verified against any candidates.
bool typeSupportsBuilderOp(Type builderType, DeclContext *dc, Identifier fnName,
ArrayRef<Identifier> argLabels = {},
SmallVectorImpl<ValueDecl *> *allResults = nullptr);
/// method with the given name. If provided and non-empty, the \c argLabels
/// are verified against any candidates. If provided, \c nearMisses will be
/// populated with all members which have the correct base name, but were not
/// considered matches (as well as the reason they were not considered matches,
/// if known).
bool typeSupportsBuilderOp(
Type builderType, DeclContext *dc, Identifier fnName,
ArrayRef<Identifier> argLabels = {},
SmallVectorImpl<FuncDecl *> *matches = nullptr,
SmallVectorImpl<BuilderOpMismatch> *nearMisses = nullptr);
}; // namespace TypeChecker

/// Temporary on-stack storage and unescaping for encoded diagnostic
Expand Down
6 changes: 6 additions & 0 deletions test/decl/var/function_builders.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,11 @@ enum InvalidBuilder11 { // expected-error {{function builder must provide at lea
case buildBlock(Any) // expected-note {{enum case 'buildBlock' cannot be used to satisfy the function builder requirement}}
}

@_functionBuilder
struct InvalidBuilder12 { // expected-error {{function builder must provide at least one static 'buildBlock' method}}
static func buildBlock(exprs: Any...) -> Int { return exprs.count } // expected-note {{potential match 'buildBlock' can never be called implicitly because it has at least one labeled argument}}
}

struct S {
@ValidBuilder1 var v1: Int { 1 }
@ValidBuilder2 var v2: Int { 1 }
Expand All @@ -216,4 +221,5 @@ struct S {
@InvalidBuilder9 var i9: Int { 1 }
@InvalidBuilder10 var i10: Int { 1 }
@InvalidBuilder11 var i11: InvalidBuilder11 { 1 }
@InvalidBuilder12 var i12: Int { 1 } // expected-error {{missing argument label 'exprs:' in call}}
}