Skip to content

Drastically Simplify VarDecl Validation #27648

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 4 commits into from
Oct 14, 2019
Merged

Drastically Simplify VarDecl Validation #27648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd82b63
Push VarDecl's TypeRepr into ParamDecl
CodaFi Oct 11, 2019
742f6b2
Drastically Simplify VarDecl Validation
CodaFi Oct 11, 2019
28717ca
Restore the circular pattern binding validation hack
CodaFi Oct 13, 2019
0082ba5
Remove VarDecl::markInvalid
CodaFi Oct 13, 2019
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
40 changes: 20 additions & 20 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ namespace swift {
class GenericTypeParamDecl;
class GenericTypeParamType;
class ModuleDecl;
class NamedPattern;
class EnumCaseDecl;
class EnumElementDecl;
class ParameterList;
Expand Down Expand Up @@ -4773,6 +4774,8 @@ enum class PropertyWrapperSynthesizedPropertyKind {

/// VarDecl - 'var' and 'let' declarations.
class VarDecl : public AbstractStorageDecl {
NamedPattern *NamingPattern = nullptr;

public:
enum class Introducer : uint8_t {
Let = 0,
Expand All @@ -4786,10 +4789,6 @@ class VarDecl : public AbstractStorageDecl {
bool issCaptureList, SourceLoc nameLoc, Identifier name,
DeclContext *dc, StorageIsMutable_t supportsMutation);

TypeRepr *ParentRepr = nullptr;

Type typeInContext;

public:
VarDecl(bool isStatic, Introducer introducer, bool isCaptureList,
SourceLoc nameLoc, Identifier name, DeclContext *dc)
Expand All @@ -4806,26 +4805,10 @@ class VarDecl : public AbstractStorageDecl {
return hasName() ? getBaseName().getIdentifier().str() : "_";
}

/// Retrieve the TypeRepr corresponding to the parsed type of the parent
/// pattern, if it exists.
TypeRepr *getTypeRepr() const { return ParentRepr; }
void setTypeRepr(TypeRepr *repr) { ParentRepr = repr; }

bool hasType() const {
// We have a type if either the type has been computed already or if
// this is a deserialized declaration with an interface type.
return !typeInContext.isNull();
}

/// Get the type of the variable within its context. If the context is generic,
/// this will use archetypes.
Type getType() const;

/// Set the type of the variable within its context.
void setType(Type t);

void markInvalid();

/// Retrieve the source range of the variable type, or an invalid range if the
/// variable's type is not explicitly written in the source.
///
Expand Down Expand Up @@ -4864,6 +4847,14 @@ class VarDecl : public AbstractStorageDecl {
///
Pattern *getParentPattern() const;

/// Returns the parsed type of this variable declaration. For parameters, this
/// is the parsed type the user explicitly wrote. For variables, this is the
/// type the user wrote in the typed pattern that binds this variable.
///
/// Note that there are many cases where the user may elide types. This will
/// return null in those cases.
TypeRepr *getTypeReprOrParentPatternTypeRepr() const;

/// Return the statement that owns the pattern associated with this VarDecl,
/// if one exists.
///
Expand Down Expand Up @@ -4904,6 +4895,9 @@ class VarDecl : public AbstractStorageDecl {
Parent = v;
}

NamedPattern *getNamingPattern() const { return NamingPattern; }
Copy link
Contributor Author

@CodaFi CodaFi Oct 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The naming pattern is a compromise and a cache. We could absolutely requestify this and have it computed by rooting around in the parent pattern (binding) for the NamedPattern that has us/our canonical var as its decl.

void setNamingPattern(NamedPattern *Pat) { NamingPattern = Pat; }

/// If this is a VarDecl that does not belong to a CaseLabelItem's pattern,
/// return this. Otherwise, this VarDecl must belong to a CaseStmt's
/// CaseLabelItem. In that case, return the first case label item of the first
Expand Down Expand Up @@ -5158,6 +5152,8 @@ class ParamDecl : public VarDecl {
SourceLoc ArgumentNameLoc;
SourceLoc SpecifierLoc;

TypeRepr *TyRepr = nullptr;

struct StoredDefaultArgument {
PointerUnion<Expr *, VarDecl *> DefaultArg;
Initializer *InitContext = nullptr;
Expand Down Expand Up @@ -5203,6 +5199,10 @@ class ParamDecl : public VarDecl {

SourceLoc getSpecifierLoc() const { return SpecifierLoc; }

/// Retrieve the TypeRepr corresponding to the parsed type of the parameter, if it exists.
TypeRepr *getTypeRepr() const { return TyRepr; }
void setTypeRepr(TypeRepr *repr) { TyRepr = repr; }

DefaultArgumentKind getDefaultArgumentKind() const {
return static_cast<DefaultArgumentKind>(Bits.ParamDecl.defaultArgumentKind);
}
Expand Down
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 @@ -1509,6 +1509,9 @@ ERROR(pattern_binds_no_variables,none,
"%select{property|global variable}0 declaration does not bind any "
"variables",
(unsigned))
ERROR(variable_bound_by_no_pattern,none,
"variable %0 is not bound by any pattern",
(DeclName))

WARNING(optional_ambiguous_case_ref,none,
"assuming you mean '%0.%2'; did you mean '%1.%2' instead?",
Expand Down
6 changes: 2 additions & 4 deletions lib/AST/ASTDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ namespace {

if (auto *var = dyn_cast<VarDecl>(VD)) {
PrintWithColorRAII(OS, TypeColor) << " type='";
if (var->hasType())
if (auto varTy = var->hasInterfaceType())
var->getType().print(PrintWithColorRAII(OS, TypeColor).getOS());
else
PrintWithColorRAII(OS, TypeColor) << "<null type>";
Expand Down Expand Up @@ -954,13 +954,11 @@ namespace {
PrintWithColorRAII(OS, IdentifierColor)
<< " apiName=" << P->getArgumentName();

if (P->hasType()) {
if (P->hasInterfaceType()) {
PrintWithColorRAII(OS, TypeColor) << " type='";
P->getType().print(PrintWithColorRAII(OS, TypeColor).getOS());
PrintWithColorRAII(OS, TypeColor) << "'";
}

if (P->hasInterfaceType()) {
PrintWithColorRAII(OS, InterfaceTypeColor) << " interface type='";
P->getInterfaceType().print(
PrintWithColorRAII(OS, InterfaceTypeColor).getOS());
Expand Down
2 changes: 1 addition & 1 deletion lib/AST/ASTPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2524,7 +2524,7 @@ void PrintAST::visitVarDecl(VarDecl *decl) {
if (decl->hasInterfaceType()) {
Printer << ": ";
TypeLoc tyLoc;
if (auto *repr = decl->getTypeRepr())
if (auto *repr = decl->getTypeReprOrParentPatternTypeRepr())
tyLoc = TypeLoc(repr, decl->getInterfaceType());
else
tyLoc = TypeLoc::withoutLoc(decl->getInterfaceType());
Expand Down
34 changes: 12 additions & 22 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2595,7 +2595,7 @@ OpaqueReturnTypeRepr *ValueDecl::getOpaqueResultTypeRepr() const {
}
}
} else {
returnRepr = VD->getTypeRepr();
returnRepr = VD->getTypeReprOrParentPatternTypeRepr();
}
} else if (auto *FD = dyn_cast<FuncDecl>(this)) {
returnRepr = FD->getBodyResultTypeLoc().getTypeRepr();
Expand Down Expand Up @@ -5007,24 +5007,7 @@ VarDecl::VarDecl(DeclKind kind, bool isStatic, VarDecl::Introducer introducer,
}

Type VarDecl::getType() const {
if (!typeInContext) {
const_cast<VarDecl *>(this)->typeInContext =
getDeclContext()->mapTypeIntoContext(
getInterfaceType());
}

return typeInContext;
}

void VarDecl::setType(Type t) {
assert(t.isNull() || !t->is<InOutType>());
typeInContext = t;
}

void VarDecl::markInvalid() {
auto &Ctx = getASTContext();
setType(ErrorType::get(Ctx));
setInterfaceType(ErrorType::get(Ctx));
return getDeclContext()->mapTypeIntoContext(getInterfaceType());
}

/// Returns whether the var is settable in the specified context: this
Expand Down Expand Up @@ -5269,9 +5252,6 @@ Pattern *VarDecl::getParentPattern() const {
if (pat->containsVarDecl(this))
return pat;
}

//stmt->dump();
assert(0 && "Unknown parent pattern statement?");
}

// Otherwise, check if we have to walk our case stmt's var decl list to find
Expand All @@ -5285,6 +5265,16 @@ Pattern *VarDecl::getParentPattern() const {
return nullptr;
}

TypeRepr *VarDecl::getTypeReprOrParentPatternTypeRepr() const {
if (auto *param = dyn_cast<ParamDecl>(this))
return param->getTypeRepr();

if (auto *parentPattern = dyn_cast_or_null<TypedPattern>(getParentPattern()))
return parentPattern->getTypeRepr();

return nullptr;
}

NullablePtr<VarDecl>
VarDecl::getCorrespondingFirstCaseLabelItemVarDecl() const {
if (!hasName())
Expand Down
2 changes: 0 additions & 2 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2110,7 +2110,6 @@ applyPropertyOwnership(VarDecl *prop,
if (attrs & clang::ObjCPropertyDecl::OBJC_PR_weak) {
prop->getAttrs().add(new (ctx)
ReferenceOwnershipAttr(ReferenceOwnership::Weak));
prop->setType(WeakStorageType::get(prop->getType(), ctx));
prop->setInterfaceType(WeakStorageType::get(
prop->getInterfaceType(), ctx));
return;
Expand All @@ -2119,7 +2118,6 @@ applyPropertyOwnership(VarDecl *prop,
(attrs & clang::ObjCPropertyDecl::OBJC_PR_unsafe_unretained)) {
prop->getAttrs().add(
new (ctx) ReferenceOwnershipAttr(ReferenceOwnership::Unmanaged));
prop->setType(UnmanagedStorageType::get(prop->getType(), ctx));
prop->setInterfaceType(UnmanagedStorageType::get(
prop->getInterfaceType(), ctx));
return;
Expand Down
3 changes: 0 additions & 3 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4455,7 +4455,6 @@ namespace {
SourceLoc(),
/*argument label*/ SourceLoc(), Identifier(),
/*parameter name*/ SourceLoc(), ctx.getIdentifier("$0"), closure);
param->setType(baseTy);
param->setInterfaceType(baseTy->mapTypeOutOfContext());
param->setSpecifier(ParamSpecifier::Default);

Expand All @@ -4471,7 +4470,6 @@ namespace {
/*argument label*/ SourceLoc(), Identifier(),
/*parameter name*/ SourceLoc(),
ctx.getIdentifier("$kp$"), outerClosure);
outerParam->setType(keyPathTy);
outerParam->setInterfaceType(keyPathTy->mapTypeOutOfContext());
outerParam->setSpecifier(ParamSpecifier::Default);

Expand Down Expand Up @@ -4650,7 +4648,6 @@ namespace {
Expr *visitTapExpr(TapExpr *E) {
auto type = simplifyType(cs.getType(E));

E->getVar()->setType(type);
E->getVar()->setInterfaceType(type->mapTypeOutOfContext());

cs.setType(E, type);
Expand Down
25 changes: 4 additions & 21 deletions lib/Sema/CSDiag.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,7 +998,6 @@ namespace {
llvm::DenseMap<Expr*, Type> ExprTypes;
llvm::DenseMap<TypeLoc*, Type> TypeLocTypes;
llvm::DenseMap<Pattern*, Type> PatternTypes;
llvm::DenseMap<ParamDecl*, Type> ParamDeclTypes;
llvm::DenseMap<ParamDecl*, Type> ParamDeclInterfaceTypes;
llvm::DenseSet<ValueDecl*> PossiblyInvalidDecls;
ExprTypeSaverAndEraser(const ExprTypeSaverAndEraser&) = delete;
Expand Down Expand Up @@ -1045,10 +1044,6 @@ namespace {
// associated TypeLocs or resynthesized as fresh typevars.
if (auto *CE = dyn_cast<ClosureExpr>(expr))
for (auto P : *CE->getParameters()) {
if (P->hasType()) {
TS->ParamDeclTypes[P] = P->getType();
P->setType(Type());
}
if (P->hasInterfaceType()) {
TS->ParamDeclInterfaceTypes[P] = P->getInterfaceType();
P->setInterfaceType(Type());
Expand Down Expand Up @@ -1100,13 +1095,7 @@ namespace {

for (auto patternElt : PatternTypes)
patternElt.first->setType(patternElt.second);

for (auto paramDeclElt : ParamDeclTypes) {
assert(!paramDeclElt.first->isImmutable() ||
!paramDeclElt.second->is<InOutType>());
paramDeclElt.first->setType(paramDeclElt.second->getInOutObjectType());
}


for (auto paramDeclIfaceElt : ParamDeclInterfaceTypes) {
assert(!paramDeclIfaceElt.first->isImmutable() ||
!paramDeclIfaceElt.second->is<InOutType>());
Expand Down Expand Up @@ -1144,11 +1133,6 @@ namespace {
for (auto patternElt : PatternTypes)
if (!patternElt.first->hasType())
patternElt.first->setType(patternElt.second);

for (auto paramDeclElt : ParamDeclTypes)
if (!paramDeclElt.first->hasType()) {
paramDeclElt.first->setType(getParamBaseType(paramDeclElt));
}

for (auto paramDeclIfaceElt : ParamDeclInterfaceTypes)
if (!paramDeclIfaceElt.first->hasInterfaceType()) {
Expand Down Expand Up @@ -3762,10 +3746,9 @@ bool FailureDiagnosis::diagnoseClosureExpr(
//
// Handle this by rewriting the arguments to UnresolvedType().
for (auto VD : *CE->getParameters()) {
if (VD->hasType() && (VD->getType()->hasTypeVariable() ||
VD->getType()->hasError())) {
VD->setType(CS.getASTContext().TheUnresolvedType);
VD->setInterfaceType(VD->getType());
if (VD->hasInterfaceType() && (VD->getType()->hasTypeVariable() ||
VD->getType()->hasError())) {
VD->setInterfaceType(CS.getASTContext().TheUnresolvedType);
}
}

Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/ConstraintSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ Type ConstraintSystem::getUnopenedTypeOfReference(VarDecl *value, Type baseType,
if (!var->isInvalid()) {
TC.diagnose(var->getLoc(), diag::recursive_decl_reference,
var->getDescriptiveKind(), var->getName());
var->markInvalid();
var->setInterfaceType(ErrorType::get(getASTContext()));
}
return ErrorType::get(TC.Context);
}
Expand Down
4 changes: 2 additions & 2 deletions lib/Sema/DerivedConformanceCodable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ deriveBodyEncodable_encode(AbstractFunctionDecl *encodeDecl, void *) {
auto codingKeysType = codingKeysEnum->getDeclaredType();
auto *containerDecl = createKeyedContainer(C, funcDC,
C.getKeyedEncodingContainerDecl(),
codingKeysType,
codingKeysEnum->getDeclaredInterfaceType(),
VarDecl::Introducer::Var);

auto *containerExpr = new (C) DeclRefExpr(ConcreteDeclRef(containerDecl),
Expand Down Expand Up @@ -806,7 +806,7 @@ deriveBodyDecodable_init(AbstractFunctionDecl *initDecl, void *) {
auto codingKeysType = codingKeysEnum->getDeclaredType();
auto *containerDecl = createKeyedContainer(C, funcDC,
C.getKeyedDecodingContainerDecl(),
codingKeysType,
codingKeysEnum->getDeclaredInterfaceType(),
VarDecl::Introducer::Let);

auto *containerExpr = new (C) DeclRefExpr(ConcreteDeclRef(containerDecl),
Expand Down
4 changes: 2 additions & 2 deletions lib/Sema/DerivedConformanceEquatableHashable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ static VarDecl *indexedVarDecl(char prefixChar, int index, Type type,
/*IsCaptureList*/true, SourceLoc(),
C.getIdentifier(indexStrRef),
varContext);
varDecl->setType(type);
varDecl->setInterfaceType(type);
varDecl->setHasNonPatternBindingInit(true);
return varDecl;
}
Expand Down Expand Up @@ -1225,7 +1225,7 @@ static ValueDecl *deriveHashable_hashValue(DerivedConformance &derived) {
new (C) VarDecl(/*IsStatic*/false, VarDecl::Introducer::Var,
/*IsCaptureList*/false, SourceLoc(),
C.Id_hashValue, parentDC);
hashValueDecl->setType(intType);
hashValueDecl->setInterfaceType(intType);

ParameterList *params = ParameterList::createEmpty(C);

Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2077,7 +2077,7 @@ class VarDeclUsageChecker : public ASTWalker {

// If the variable was invalid, ignore it and notice that the code is
// malformed.
if (VD->isInvalid() || !VD->hasType()) {
if (!VD->getInterfaceType() || VD->isInvalid()) {
sawError = true;
return false;
}
Expand Down
1 change: 0 additions & 1 deletion lib/Sema/PCMacro.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,6 @@ class Instrumenter : InstrumenterBase {
/*IsCaptureList*/false, SourceLoc(),
Context.getIdentifier(NameBuf),
TypeCheckDC);
VD->setType(MaybeLoadInitExpr->getType());
VD->setInterfaceType(MaybeLoadInitExpr->getType()->mapTypeOutOfContext());
VD->setImplicit();

Expand Down
1 change: 0 additions & 1 deletion lib/Sema/PlaygroundTransform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,6 @@ class Instrumenter : InstrumenterBase {
/*IsCaptureList*/false, SourceLoc(),
Context.getIdentifier(NameBuf),
TypeCheckDC);
VD->setType(MaybeLoadInitExpr->getType());
VD->setInterfaceType(MaybeLoadInitExpr->getType()->mapTypeOutOfContext());
VD->setImplicit();

Expand Down
Loading