Skip to content

[AST, Sema] Replace HoleType with PlaceholderType #35589

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

Merged
merged 14 commits into from
Feb 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3542,6 +3542,9 @@ ERROR(generic_type_requires_arguments,none,
NOTE(descriptive_generic_type_declared_here,none,
"%0 declared here", (StringRef))

ERROR(placeholder_type_not_allowed,none,
"you cannot use a placeholder type here", ())

WARNING(use_of_void_pointer,none,
"Unsafe%0Pointer<Void> has been replaced by Unsafe%0RawPointer", (StringRef))

Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/TypeNodes.def
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@

TYPE(Error, Type)
UNCHECKED_TYPE(Unresolved, Type)
UNCHECKED_TYPE(Hole, Type)
UNCHECKED_TYPE(Placeholder, Type)
ABSTRACT_TYPE(Builtin, Type)
ABSTRACT_TYPE(AnyBuiltinInteger, BuiltinType)
BUILTIN_TYPE(BuiltinInteger, AnyBuiltinIntegerType)
Expand Down
29 changes: 29 additions & 0 deletions include/swift/AST/TypeRepr.h
Original file line number Diff line number Diff line change
Expand Up @@ -1085,6 +1085,34 @@ class OpaqueReturnTypeRepr : public TypeRepr {
friend class TypeRepr;
};

/// TypeRepr for a user-specified placeholder (essentially, a user-facing
/// representation of an anonymous type variable.
///
/// Can occur anywhere a normal type would occur, though usually expected to be
/// used in structural positions like \c Generic<Int,_>.
class PlaceholderTypeRepr: public TypeRepr {
SourceLoc UnderscoreLoc;

public:
PlaceholderTypeRepr(SourceLoc loc)
: TypeRepr(TypeReprKind::Placeholder), UnderscoreLoc(loc)
{}

SourceLoc getUnderscoreLoc() const { return UnderscoreLoc; }

static bool classof(const TypeRepr *T) {
return T->getKind() == TypeReprKind::Placeholder;
}
static bool classof(const PlaceholderTypeRepr *T) { return true; }

private:
SourceLoc getStartLocImpl() const { return UnderscoreLoc; }
SourceLoc getEndLocImpl() const { return UnderscoreLoc; }
SourceLoc getLocImpl() const { return UnderscoreLoc; }
void printImpl(ASTPrinter &Printer, const PrintOptions &Opts) const;
friend class TypeRepr;
};

/// SIL-only TypeRepr for box types.
///
/// Boxes are either concrete: { var Int, let String }
Expand Down Expand Up @@ -1198,6 +1226,7 @@ inline bool TypeRepr::isSimple() const {
case TypeReprKind::SILBox:
case TypeReprKind::Shared:
case TypeReprKind::Owned:
case TypeReprKind::Placeholder:
return true;
}
llvm_unreachable("bad TypeRepr kind");
Expand Down
1 change: 1 addition & 0 deletions include/swift/AST/TypeReprNodes.def
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ TYPEREPR(Composition, TypeRepr)
TYPEREPR(Metatype, TypeRepr)
TYPEREPR(Protocol, TypeRepr)
TYPEREPR(OpaqueReturn, TypeRepr)
TYPEREPR(Placeholder, TypeRepr)
ABSTRACT_TYPEREPR(Specifier, TypeRepr)
TYPEREPR(InOut, SpecifierTypeRepr)
TYPEREPR(Shared, SpecifierTypeRepr)
Expand Down
44 changes: 23 additions & 21 deletions include/swift/AST/Types.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class Identifier;
class InOutType;
class OpaqueTypeDecl;
class OpenedArchetypeType;
class PlaceholderTypeRepr;
enum class ReferenceCounting : uint8_t;
enum class ResilienceExpansion : unsigned;
class SILModule;
Expand Down Expand Up @@ -126,7 +127,7 @@ class RecursiveTypeProperties {

/// This type expression contains an UnresolvedType.
HasUnresolvedType = 0x08,

/// Whether this type expression contains an unbound generic type.
HasUnboundGeneric = 0x10,

Expand All @@ -145,14 +146,14 @@ class RecursiveTypeProperties {

/// This type contains a DependentMemberType.
HasDependentMember = 0x200,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note - making unrelated whitespace changes like this sometimes complicates the review since it's easy to miss value changes...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that must have been clang-format, I didn't make any intentional whitespace changes here. I'll revert!


/// This type contains an OpaqueTypeArchetype.
HasOpaqueArchetype = 0x400,

/// This type contains a type hole.
HasTypeHole = 0x800,
/// This type contains a type placeholder.
HasPlaceholder = 0x800,

Last_Property = HasTypeHole
Last_Property = HasPlaceholder
};
enum { BitWidth = countBitsUsed(Property::Last_Property) };

Expand Down Expand Up @@ -207,9 +208,8 @@ class RecursiveTypeProperties {
/// generic type?
bool hasUnboundGeneric() const { return Bits & HasUnboundGeneric; }

/// Does a type with these properties structurally contain a
/// type hole?
bool hasTypeHole() const { return Bits & HasTypeHole; }
/// Does a type with these properties structurally contain a placeholder?
bool hasPlaceholder() const { return Bits & HasPlaceholder; }

/// Returns the set of properties present in either set.
friend RecursiveTypeProperties operator|(Property lhs, Property rhs) {
Expand Down Expand Up @@ -546,7 +546,7 @@ class alignas(1 << TypeAlignInBits) TypeBase {
/// Is this the 'Any' type?
bool isAny();

bool isHole();
bool isPlaceholder();

/// Does the type have outer parenthesis?
bool hasParenSugar() const { return getKind() == TypeKind::Paren; }
Expand Down Expand Up @@ -580,8 +580,10 @@ class alignas(1 << TypeAlignInBits) TypeBase {
return getRecursiveProperties().hasUnresolvedType();
}

/// Determine whether this type involves a hole.
bool hasHole() const { return getRecursiveProperties().hasTypeHole(); }
/// Determine whether this type involves a \c PlaceholderType.
bool hasPlaceholder() const {
return getRecursiveProperties().hasPlaceholder();
}

/// Determine whether the type involves a context-dependent archetype.
bool hasArchetype() const {
Expand Down Expand Up @@ -5809,31 +5811,31 @@ TypeVariableType : public TypeBase {
};
DEFINE_EMPTY_CAN_TYPE_WRAPPER(TypeVariableType, Type)

/// HoleType - This represents a placeholder type for a type variable
/// PlaceholderType - This represents a placeholder type for a type variable
/// or dependent member type that cannot be resolved to a concrete type
/// because the expression is ambiguous. This type is only used by the
/// constraint solver and transformed into UnresolvedType to be used in AST.
class HoleType : public TypeBase {
using Originator = llvm::PointerUnion<TypeVariableType *,
DependentMemberType *, VarDecl *,
ErrorExpr *>;
class PlaceholderType : public TypeBase {
using Originator =
llvm::PointerUnion<TypeVariableType *, DependentMemberType *, VarDecl *,
ErrorExpr *, PlaceholderTypeRepr *>;

Originator O;

HoleType(ASTContext &C, Originator originator,
RecursiveTypeProperties properties)
: TypeBase(TypeKind::Hole, &C, properties), O(originator) {}
PlaceholderType(ASTContext &C, Originator originator,
RecursiveTypeProperties properties)
: TypeBase(TypeKind::Placeholder, &C, properties), O(originator) {}

public:
static Type get(ASTContext &ctx, Originator originator);

Originator getOriginator() const { return O; }

static bool classof(const TypeBase *T) {
return T->getKind() == TypeKind::Hole;
return T->getKind() == TypeKind::Placeholder;
}
};
DEFINE_EMPTY_CAN_TYPE_WRAPPER(HoleType, Type)
DEFINE_EMPTY_CAN_TYPE_WRAPPER(PlaceholderType, Type)

inline bool TypeBase::isTypeVariableOrMember() {
if (is<TypeVariableType>())
Expand Down
16 changes: 8 additions & 8 deletions include/swift/Sema/CSBindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ struct PotentialBinding {

static PotentialBinding forHole(TypeVariableType *typeVar,
ConstraintLocator *locator) {
return {HoleType::get(typeVar->getASTContext(), typeVar),
return {PlaceholderType::get(typeVar->getASTContext(), typeVar),
AllowedBindingKind::Exact,
/*source=*/locator};
}
Expand Down Expand Up @@ -279,9 +279,9 @@ struct PotentialBindings {
bool isPotentiallyIncomplete() const;

/// If this type variable doesn't have any viable bindings, or
/// if there is only one binding and it's a hole type, consider
/// if there is only one binding and it's a placeholder type, consider
Copy link
Contributor

Choose a reason for hiding this comment

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

@Jumhyn @hborla What do you think about keeping "hole" terminology in the constraint system? My personal preference would be to keep calling "untyped" places in constraint system holes and call them placeholders in AST.

Copy link
Member

Choose a reason for hiding this comment

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

I agree - I like the term "hole" for the constraint system because "placeholder" implies that it can be replaced, but if we bind to a placeholder in the constraint system, that means it solver could not replace it with a real type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me, I don't have super strong feelings either way! So we'd keep the type name as PlaceholderType, but all the constraint system methods (such as isDirectHole()) stay as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that’s the idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Effectively we are just utilizing a placeholder in a solution and AST to represent places which couldn’t be inferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hborla @xedin Thoughts on TVO_CanBindToPlaceholder vs. TVO_CanBindToHole? This is the only one in the constraint system that seems to me like the use of "hole" is perhaps more confusing than "placeholder", since it most literally represents whether we will bind the type var to a PlaceholderType or not. But I'm good with whatever y'all think is best!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should leave it as hole since it would be consistent with the rest.

/// this type variable to be a hole in a constraint system
/// regardless of where hole type originated.
/// regardless of where the placeholder type originated.
bool isHole() const {
if (isDirectHole())
return true;
Expand All @@ -290,14 +290,14 @@ struct PotentialBindings {
return false;

const auto &binding = Bindings.front();
return binding.BindingType->is<HoleType>();
return binding.BindingType->is<PlaceholderType>();
}

/// Determines whether the only possible binding for this type variable
/// would be a hole type. This is different from `isHole` method because
/// type variable could also acquire a hole type transitively if one
/// of the type variables in its subtype/equivalence chain has been
/// bound to a hole type.
/// would be a placeholder type. This is different from `isHole` method
/// because type variable could also acquire a placeholder type transitively
/// if one of the type variables in its subtype/equivalence chain has been
/// bound to a placeholder type.
bool isDirectHole() const;

/// Determine if the bindings only constrain the type variable from above
Expand Down
14 changes: 14 additions & 0 deletions include/swift/Sema/ConstraintLocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,20 @@ class LocatorPathElt::ConformanceRequirement final
}
};

class LocatorPathElt::PlaceholderType final
: public StoredPointerElement<PlaceholderTypeRepr> {
public:
PlaceholderType(PlaceholderTypeRepr *placeholderRepr)
: StoredPointerElement(PathElementKind::PlaceholderType,
placeholderRepr) {}

PlaceholderTypeRepr *getPlaceholderRepr() const { return getStoredPointer(); }

static bool classof(const LocatorPathElt *elt) {
return elt->getKind() == ConstraintLocator::PlaceholderType;
}
};

namespace details {
template <typename CustomPathElement>
class PathElement {
Expand Down
3 changes: 3 additions & 0 deletions include/swift/Sema/ConstraintLocatorPathElts.def
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,9 @@ SIMPLE_LOCATOR_PATH_ELT(UnresolvedMemberChainResult)
/// The conformance requirement associated with a type.
CUSTOM_LOCATOR_PATH_ELT(ConformanceRequirement)

/// A source-specified placeholder.
CUSTOM_LOCATOR_PATH_ELT(PlaceholderType)

#undef LOCATOR_PATH_ELT
#undef CUSTOM_LOCATOR_PATH_ELT
#undef SIMPLE_LOCATOR_PATH_ELT
Expand Down
37 changes: 29 additions & 8 deletions include/swift/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ enum TypeVariableOptions {
/// Whether the type variable can be bound to a non-escaping type or not.
TVO_CanBindToNoEscape = 0x04,

/// Whether the type variable can be bound to a hole type or not.
/// Whether the type variable can be bound to a hole or not.
TVO_CanBindToHole = 0x08,

/// Whether a more specific deduction for this type variable implies a
Expand Down Expand Up @@ -327,7 +327,7 @@ class TypeVariableType::Implementation {
/// Whether this type variable can bind to an inout type.
bool canBindToNoEscape() const { return getRawOptions() & TVO_CanBindToNoEscape; }

/// Whether this type variable can bind to a hole type.
/// Whether this type variable can bind to a hole.
bool canBindToHole() const { return getRawOptions() & TVO_CanBindToHole; }

/// Whether this type variable prefers a subtype binding over a supertype
Expand Down Expand Up @@ -3759,14 +3759,15 @@ class ConstraintSystem {
Type openUnboundGenericType(GenericTypeDecl *decl, Type parentTy,
ConstraintLocatorBuilder locator);

/// "Open" the given type by replacing any occurrences of unbound
/// generic types with bound generic types with fresh type variables as
/// generic arguments.
/// Replace placeholder types with fresh type variables, and unbound generic
/// types with bound generic types whose generic args are fresh type
/// variables.
///
/// \param type The type to open.
/// \param type The type on which to perform the conversion.
///
/// \returns The opened type.
Type openUnboundGenericTypes(Type type, ConstraintLocatorBuilder locator);
/// \returns The converted type.
Type replaceInferableTypesWithTypeVars(Type type,
ConstraintLocatorBuilder locator);

/// "Open" the given type by replacing any occurrences of generic
/// parameter types and dependent member types with fresh type variables.
Expand Down Expand Up @@ -5024,6 +5025,26 @@ class OpenUnboundGenericType {
}
};

class HandlePlaceholderType {
ConstraintSystem &cs;
ConstraintLocator *locator;

public:
explicit HandlePlaceholderType(ConstraintSystem &cs,
const ConstraintLocatorBuilder &locator)
: cs(cs) {
this->locator = cs.getConstraintLocator(locator);
}

Type operator()(PlaceholderTypeRepr *placeholderRepr) const {
return cs.createTypeVariable(
cs.getConstraintLocator(
locator, LocatorPathElt::PlaceholderType(placeholderRepr)),
TVO_CanBindToNoEscape | TVO_PrefersSubtypeBinding |
TVO_CanBindToHole);
}
};

/// Compute the shuffle required to map from a given tuple type to
/// another tuple type.
///
Expand Down
16 changes: 7 additions & 9 deletions lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1517,9 +1517,8 @@ bool ASTContext::hadError() const {
/// Retrieve the arena from which we should allocate storage for a type.
static AllocationArena getArena(RecursiveTypeProperties properties) {
bool hasTypeVariable = properties.hasTypeVariable();
bool hasHole = properties.hasTypeHole();
return hasTypeVariable || hasHole ? AllocationArena::ConstraintSolver
: AllocationArena::Permanent;
return hasTypeVariable ? AllocationArena::ConstraintSolver
: AllocationArena::Permanent;
}

void ASTContext::addSearchPath(StringRef searchPath, bool isFramework,
Expand Down Expand Up @@ -2495,11 +2494,10 @@ Type ErrorType::get(Type originalType) {
return entry = new (mem) ErrorType(ctx, originalType, properties);
}

Type HoleType::get(ASTContext &ctx, Originator originator) {
Type PlaceholderType::get(ASTContext &ctx, Originator originator) {
assert(originator);
auto arena = getArena(RecursiveTypeProperties::HasTypeHole);
return new (ctx, arena)
HoleType(ctx, originator, RecursiveTypeProperties::HasTypeHole);
return new (ctx, AllocationArena::Permanent)
PlaceholderType(ctx, originator, RecursiveTypeProperties::HasPlaceholder);
}

BuiltinIntegerType *BuiltinIntegerType::get(BuiltinIntegerWidth BitWidth,
Expand Down Expand Up @@ -2943,7 +2941,7 @@ ReferenceStorageType *ReferenceStorageType::get(Type T,
ReferenceOwnership ownership,
const ASTContext &C) {
assert(!T->hasTypeVariable()); // not meaningful in type-checker
assert(!T->hasHole());
assert(!T->hasPlaceholder());
switch (optionalityOf(ownership)) {
case ReferenceOwnershipOptionality::Disallowed:
assert(!T->getOptionalObjectType() && "optional type is disallowed");
Expand Down Expand Up @@ -3330,7 +3328,7 @@ GenericFunctionType *GenericFunctionType::get(GenericSignature sig,
ExtInfo info) {
assert(sig && "no generic signature for generic function type?!");
assert(!result->hasTypeVariable());
assert(!result->hasHole());
assert(!result->hasPlaceholder());

llvm::FoldingSetNodeID id;
GenericFunctionType::Profile(id, sig, params, result, info);
Expand Down
Loading