Skip to content

[AST] Replace type variables and placeholders in original ErrorTypes #82292

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
Jun 20, 2025
Merged

[AST] Replace type variables and placeholders in original ErrorTypes #82292

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
31 changes: 27 additions & 4 deletions lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3576,12 +3576,35 @@ void LocatableType::Profile(llvm::FoldingSetNodeID &id, SourceLoc loc,
// Simple accessors.
Type ErrorType::get(const ASTContext &C) { return C.TheErrorType; }

static Type replacingTypeVariablesAndPlaceholders(Type ty) {
if (!ty || !ty->hasTypeVariableOrPlaceholder())
return ty;

auto &ctx = ty->getASTContext();

return ty.transformRec([&](Type ty) -> std::optional<Type> {
if (!ty->hasTypeVariableOrPlaceholder())
return ty;

// Match the logic in `Solution::simplifyType` and use UnresolvedType.
// FIXME: Ideally we'd get rid of UnresolvedType and just use a fresh
// PlaceholderType, but we don't currently support placeholders with no
// originators.
Copy link
Contributor

@xedin xedin Jun 20, 2025

Choose a reason for hiding this comment

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

Just to note that this is indeed the long-term plan, we need to remove UnresolvedType in favor of PlaceholderType

Copy link
Contributor

@xedin xedin Jun 20, 2025

Choose a reason for hiding this comment

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

Maybe we should limit this transformation to solver allocated placeholder types instead of all of them. We could also expand ErrorExpr originator and replace type variables with placeholders that point to ASTNode that their locator simplifies down to...

Copy link
Contributor Author

@hamishknight hamishknight Jun 20, 2025

Choose a reason for hiding this comment

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

Maybe we should limit this transformation to solver allocated placeholder types instead of all of them

I would rather apply the transform consistently to match what we do in Solution::simplifyType, conditionalizing the logic on the allocation arena or originator seems a little odd to me. We should revisit this when we get rid of UnresolvedType though.

We could also expand ErrorExpr originator and replace type variables with placeholders that point to ASTNode that their locator simplifies down to...

Yeah maybe, I don't think there are any ErrorType consumers that currently really care about originators, but maybe that could be useful for debugging?

Copy link
Contributor

@xedin xedin Jun 20, 2025

Choose a reason for hiding this comment

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

Yeah, I was mostly talking long-term as part of UnresolvedType removal :)

auto *typePtr = ty.getPointer();
if (isa<TypeVariableType>(typePtr) || isa<PlaceholderType>(typePtr))
return ctx.TheUnresolvedType;

return std::nullopt;
});
}

Type ErrorType::get(Type originalType) {
// The original type is only used for printing/debugging, and we don't support
// solver-allocated ErrorTypes. As such, fold any type variables and
// placeholders into UnresolvedTypes, which print as placeholders.
originalType = replacingTypeVariablesAndPlaceholders(originalType);

ASSERT(originalType);
// We don't currently support solver-allocated error types, if we want
// this in the future we'll need to adjust `Solution::simplifyType` to fold
// them into regular ErrorTypes. Additionally, any clients of
// `typesSatisfyConstraint` will need to be taught not to pass such types.
ASSERT(!originalType->getRecursiveProperties().isSolverAllocated() &&
"Solver-allocated error types not supported");

Expand Down
7 changes: 1 addition & 6 deletions lib/AST/TypeSubstitution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,13 +446,8 @@ Type TypeSubstituter::transformDependentMemberType(DependentMemberType *dependen

auto result = conformance.getTypeWitness(assocType, IFS.getOptions());
if (result->is<ErrorType>()) {
// Substitute the base type for the original ErrorType for type printing.
// Avoid doing this if the substitutions introduce type variables or
// placeholders since ErrorTypes can't be solver-allocated currently (and
// type variables aren't helpful when printing anyway).
auto substBase = origBase.subst(IFS);
if (!substBase->hasTypeVariableOrPlaceholder())
return DependentMemberType::get(ErrorType::get(substBase), assocType);
return DependentMemberType::get(ErrorType::get(substBase), assocType);
}
return result;
}
Expand Down
2 changes: 1 addition & 1 deletion ...ompiler_crashers_2/cccd6394993ea4a2.swift → ...r_crashers_2_fixed/cccd6394993ea4a2.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// {"signature":"swift::ErrorType::get(swift::Type)"}
// RUN: not --crash %target-swift-frontend -typecheck %s
// RUN: not %target-swift-frontend -typecheck %s
@convention(c) _->Int