Skip to content

Commit 4108e1d

Browse files
modocachemodocache
committed
[Sema] Mark VarDecl in capture lists
Fixes SR-2757. Variables in capture lists are treated as 'let' constants, which can result in misleading, incorrect diagnostics. Mark them as such in order to produce better diagnostics, by adding an extra parameter to the VarDecl initializer. Alternatively, these variables could be marked as implicit, but that results in other diagnostic problems: capture list variables that are never used produce warnings, but these warnings aren't normally emitted for implicit variables. Other assertions in the compiler also misfire when these variables are treated as implicit. Another alternative would be to walk up the AST and determine whether the `VarDecl`, but there doesn't appear to be a way to do so.
1 parent 474096b commit 4108e1d

19 files changed

+152
-117
lines changed

‎include/swift/AST/Decl.h‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -232,13 +232,13 @@ class alignas(1 << DeclAlignInBits) Decl {
232232
/// \brief Whether we've already performed early attribute validation.
233233
/// FIXME: This is ugly.
234234
unsigned EarlyAttrValidation : 1;
235-
235+
236236
/// \brief Whether this declaration is currently being validated.
237237
unsigned BeingValidated : 1;
238238
};
239239
enum { NumDeclBits = 11 };
240240
static_assert(NumDeclBits <= 32, "fits in an unsigned");
241-
241+
242242
class PatternBindingDeclBitfields {
243243
friend class PatternBindingDecl;
244244
unsigned : NumDeclBits;
@@ -284,7 +284,7 @@ class alignas(1 << DeclAlignInBits) Decl {
284284
class VarDeclBitfields {
285285
friend class VarDecl;
286286
unsigned : NumAbstractStorageDeclBits;
287-
287+
288288
/// \brief Whether this property is a type property (currently unfortunately
289289
/// called 'static').
290290
unsigned IsStatic : 1;
@@ -293,11 +293,14 @@ class alignas(1 << DeclAlignInBits) Decl {
293293
/// once (either in its declaration, or once later), making it immutable.
294294
unsigned IsLet : 1;
295295

296+
/// \brief Whether this declaration was an element of a capture list.
297+
unsigned IsCaptureList : 1;
298+
296299
/// \brief Whether this vardecl has an initial value bound to it in a way
297300
/// that isn't represented in the AST with an initializer in the pattern
298301
/// binding. This happens in cases like "for i in ...", switch cases, etc.
299302
unsigned HasNonPatternBindingInit : 1;
300-
303+
301304
/// \brief Whether this is a property used in expressions in the debugger.
302305
/// It is up to the debugger to instruct SIL how to access this variable.
303306
unsigned IsDebuggerVar : 1;
@@ -306,9 +309,9 @@ class alignas(1 << DeclAlignInBits) Decl {
306309
/// a.storage for lazy var a is a decl that cannot be accessed.
307310
unsigned IsUserAccessible : 1;
308311
};
309-
enum { NumVarDeclBits = NumAbstractStorageDeclBits + 5 };
312+
enum { NumVarDeclBits = NumAbstractStorageDeclBits + 6 };
310313
static_assert(NumVarDeclBits <= 32, "fits in an unsigned");
311-
314+
312315
class EnumElementDeclBitfields {
313316
friend class EnumElementDecl;
314317
unsigned : NumValueDeclBits;
@@ -4074,13 +4077,14 @@ class VarDecl : public AbstractStorageDecl {
40744077
protected:
40754078
llvm::PointerUnion<PatternBindingDecl*, Stmt*> ParentPattern;
40764079

4077-
VarDecl(DeclKind Kind, bool IsStatic, bool IsLet, SourceLoc NameLoc,
4078-
Identifier Name, Type Ty, DeclContext *DC)
4079-
: AbstractStorageDecl(Kind, DC, Name, NameLoc)
4080+
VarDecl(DeclKind Kind, bool IsStatic, bool IsLet, bool IsCaptureList,
4081+
SourceLoc NameLoc, Identifier Name, Type Ty, DeclContext *DC)
4082+
: AbstractStorageDecl(Kind, DC, Name, NameLoc)
40804083
{
40814084
VarDeclBits.IsUserAccessible = true;
40824085
VarDeclBits.IsStatic = IsStatic;
40834086
VarDeclBits.IsLet = IsLet;
4087+
VarDeclBits.IsCaptureList = IsCaptureList;
40844088
VarDeclBits.IsDebuggerVar = false;
40854089
VarDeclBits.HasNonPatternBindingInit = false;
40864090
setType(Ty);
@@ -4092,9 +4096,10 @@ class VarDecl : public AbstractStorageDecl {
40924096
Type typeInContext;
40934097

40944098
public:
4095-
VarDecl(bool IsStatic, bool IsLet, SourceLoc NameLoc, Identifier Name,
4096-
Type Ty, DeclContext *DC)
4097-
: VarDecl(DeclKind::Var, IsStatic, IsLet, NameLoc, Name, Ty, DC) { }
4099+
VarDecl(bool IsStatic, bool IsLet, bool IsCaptureList, SourceLoc NameLoc,
4100+
Identifier Name, Type Ty, DeclContext *DC)
4101+
: VarDecl(DeclKind::Var, IsStatic, IsLet, IsCaptureList, NameLoc, Name, Ty,
4102+
DC) {}
40984103

40994104
SourceRange getSourceRange() const;
41004105

@@ -4105,7 +4110,7 @@ class VarDecl : public AbstractStorageDecl {
41054110
bool isUserAccessible() const {
41064111
return VarDeclBits.IsUserAccessible;
41074112
}
4108-
4113+
41094114
TypeLoc &getTypeLoc() { return typeLoc; }
41104115
TypeLoc getTypeLoc() const { return typeLoc; }
41114116

@@ -4185,7 +4190,7 @@ class VarDecl : public AbstractStorageDecl {
41854190
return PBD->getPatternEntryForVarDecl(this).getInit();
41864191
return nullptr;
41874192
}
4188-
4193+
41894194
VarDecl *getOverriddenDecl() const {
41904195
return cast_or_null<VarDecl>(AbstractStorageDecl::getOverriddenDecl());
41914196
}
@@ -4204,6 +4209,9 @@ class VarDecl : public AbstractStorageDecl {
42044209
bool isLet() const { return VarDeclBits.IsLet; }
42054210
void setLet(bool IsLet) { VarDeclBits.IsLet = IsLet; }
42064211

4212+
/// Is this an element in a capture list?
4213+
bool isCaptureList() const { return VarDeclBits.IsCaptureList; }
4214+
42074215
/// Return true if this vardecl has an initial value bound to it in a way
42084216
/// that isn't represented in the AST with an initializer in the pattern
42094217
/// binding. This happens in cases like "for i in ...", switch cases, etc.

‎lib/AST/Decl.cpp‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3743,7 +3743,10 @@ void VarDecl::emitLetToVarNoteIfSimple(DeclContext *UseDC) const {
37433743

37443744
// Besides self, don't suggest mutability for explicit function parameters.
37453745
if (isa<ParamDecl>(this)) return;
3746-
3746+
3747+
// Don't suggest any fixes for capture list elements.
3748+
if (isCaptureList()) return;
3749+
37473750
// If this is a normal variable definition, then we can change 'let' to 'var'.
37483751
// We even are willing to suggest this for multi-variable binding, like
37493752
// "let (a,b) = "
@@ -3764,18 +3767,18 @@ ParamDecl::ParamDecl(bool isLet,
37643767
SourceLoc letVarInOutLoc, SourceLoc argumentNameLoc,
37653768
Identifier argumentName, SourceLoc parameterNameLoc,
37663769
Identifier parameterName, Type ty, DeclContext *dc)
3767-
: VarDecl(DeclKind::Param, /*IsStatic=*/false, isLet, parameterNameLoc,
3768-
parameterName, ty, dc),
3770+
: VarDecl(DeclKind::Param, /*IsStatic*/false, /*IsLet*/isLet,
3771+
/*IsCaptureList*/false, parameterNameLoc, parameterName, ty, dc),
37693772
ArgumentName(argumentName), ArgumentNameLoc(argumentNameLoc),
37703773
LetVarInOutLoc(letVarInOutLoc) {
37713774
}
37723775

37733776
/// Clone constructor, allocates a new ParamDecl identical to the first.
37743777
/// Intentionally not defined as a copy constructor to avoid accidental copies.
37753778
ParamDecl::ParamDecl(ParamDecl *PD)
3776-
: VarDecl(DeclKind::Param, /*IsStatic=*/false, PD->isLet(), PD->getNameLoc(),
3777-
PD->getName(), PD->hasType() ? PD->getType() : Type(),
3778-
PD->getDeclContext()),
3779+
: VarDecl(DeclKind::Param, /*IsStatic*/false, /*IsLet*/PD->isLet(),
3780+
/*IsCaptureList*/false, PD->getNameLoc(), PD->getName(),
3781+
PD->hasType() ? PD->getType() : Type(), PD->getDeclContext()),
37793782
ArgumentName(PD->getArgumentName()),
37803783
ArgumentNameLoc(PD->getArgumentNameLoc()),
37813784
LetVarInOutLoc(PD->getLetVarInOutLoc()),

‎lib/ClangImporter/ImportDecl.cpp‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,10 @@ createVarWithPattern(ASTContext &cxt, DeclContext *dc, Identifier name, Type ty,
123123
Accessibility setterAccessibility) {
124124
// Create a variable to store the underlying value.
125125
auto var = new (cxt) VarDecl(
126-
/*static*/ false,
127-
/*IsLet*/ isLet, SourceLoc(), name, ty, dc);
126+
/*IsStatic*/false,
127+
/*IsLet*/isLet,
128+
/*IsCaptureList*/false,
129+
SourceLoc(), name, ty, dc);
128130
if (isImplicit)
129131
var->setImplicit();
130132
var->setInterfaceType(ty);
@@ -1191,8 +1193,8 @@ static void makeStructRawValuedWithBridge(
11911193
//
11921194
// Create a computed value variable
11931195
auto computedVar = new (cxt) VarDecl(
1194-
/*static*/ false,
1195-
/*IsLet*/ false, SourceLoc(), computedVarName, bridgedType, structDecl);
1196+
/*IsStatic*/false, /*IsLet*/false, /*IsCaptureList*/false,
1197+
SourceLoc(), computedVarName, bridgedType, structDecl);
11961198
computedVar->setInterfaceType(bridgedType);
11971199
computedVar->setImplicit();
11981200
computedVar->setAccessibility(Accessibility::Public);
@@ -1496,8 +1498,8 @@ static bool addErrorDomain(NominalTypeDecl *swiftDecl,
14961498

14971499
// Make the property decl
14981500
auto errorDomainPropertyDecl = new (C) VarDecl(
1499-
isStatic,
1500-
/*IsLet=*/false, SourceLoc(), C.Id_nsErrorDomain, stringTy, swiftDecl);
1501+
/*IsStatic*/isStatic, /*IsLet*/false, /*IsCaptureList*/false,
1502+
SourceLoc(), C.Id_nsErrorDomain, stringTy, swiftDecl);
15011503
errorDomainPropertyDecl->setInterfaceType(stringTy);
15021504
errorDomainPropertyDecl->setAccessibility(Accessibility::Public);
15031505

@@ -2195,7 +2197,8 @@ namespace {
21952197
// Create the _nsError member.
21962198
// public let _nsError: NSError
21972199
auto nsErrorType = nsErrorDecl->getDeclaredInterfaceType();
2198-
auto nsErrorProp = new (C) VarDecl(/*static*/ false, /*IsLet*/ true,
2200+
auto nsErrorProp = new (C) VarDecl(/*IsStatic*/false, /*IsLet*/true,
2201+
/*IsCaptureList*/false,
21992202
loc, C.Id_nsError, nsErrorType,
22002203
errorWrapper);
22012204
nsErrorProp->setImplicit();
@@ -2261,10 +2264,10 @@ namespace {
22612264
auto rawValueConstructor = makeEnumRawValueConstructor(Impl, enumDecl);
22622265

22632266
auto varName = C.Id_rawValue;
2264-
auto rawValue = new (C) VarDecl(/*static*/ false,
2265-
/*IsLet*/ false,
2266-
SourceLoc(), varName,
2267-
underlyingType, enumDecl);
2267+
auto rawValue = new (C) VarDecl(/*IsStatic*/false, /*IsLet*/ false,
2268+
/*IsCaptureList*/false,
2269+
SourceLoc(), varName, underlyingType,
2270+
enumDecl);
22682271
rawValue->setImplicit();
22692272
rawValue->setAccessibility(Accessibility::Public);
22702273
rawValue->setSetterAccessibility(Accessibility::Private);
@@ -2789,7 +2792,8 @@ namespace {
27892792
// Map this indirect field to a Swift variable.
27902793
auto result = Impl.createDeclWithClangNode<VarDecl>(decl,
27912794
Accessibility::Public,
2792-
/*static*/ false, /*IsLet*/ false,
2795+
/*IsStatic*/false, /*IsLet*/ false,
2796+
/*IsCaptureList*/false,
27932797
Impl.importSourceLoc(decl->getLocStart()),
27942798
name, type, dc);
27952799
result->setInterfaceType(type);
@@ -2982,7 +2986,8 @@ namespace {
29822986

29832987
auto result =
29842988
Impl.createDeclWithClangNode<VarDecl>(decl, Accessibility::Public,
2985-
/*static*/ false, /*IsLet*/ false,
2989+
/*IsStatic*/ false, /*IsLet*/ false,
2990+
/*IsCaptureList*/false,
29862991
Impl.importSourceLoc(decl->getLocation()),
29872992
name, type, dc);
29882993
result->setInterfaceType(type);
@@ -3055,8 +3060,10 @@ namespace {
30553060
isStatic = true;
30563061

30573062
auto result = Impl.createDeclWithClangNode<VarDecl>(decl,
3058-
Accessibility::Public, isStatic,
3059-
Impl.shouldImportGlobalAsLet(decl->getType()),
3063+
Accessibility::Public,
3064+
/*IsStatic*/isStatic,
3065+
/*IsLet*/Impl.shouldImportGlobalAsLet(decl->getType()),
3066+
/*IsCaptureList*/false,
30603067
Impl.importSourceLoc(decl->getLocation()),
30613068
name, type, dc);
30623069
result->setInterfaceType(type);
@@ -4115,8 +4122,8 @@ namespace {
41154122

41164123
auto result = Impl.createDeclWithClangNode<VarDecl>(decl,
41174124
getOverridableAccessibility(dc),
4118-
decl->isClassProperty(), /*IsLet*/ false,
4119-
Impl.importSourceLoc(decl->getLocation()),
4125+
/*IsStatic*/decl->isClassProperty(), /*IsLet*/false,
4126+
/*IsCaptureList*/false, Impl.importSourceLoc(decl->getLocation()),
41204127
name, type, dc);
41214128
result->setInterfaceType(dc->mapTypeOutOfContext(type));
41224129

@@ -4989,8 +4996,8 @@ SwiftDeclConverter::getImplicitProperty(ImportedName importedName,
49894996
return nullptr;
49904997

49914998
auto property = Impl.createDeclWithClangNode<VarDecl>(
4992-
getter, Accessibility::Public, isStatic,
4993-
/*isLet=*/false, SourceLoc(), propertyName, swiftPropertyType, dc);
4999+
getter, Accessibility::Public, /*IsStatic*/isStatic, /*isLet*/false,
5000+
/*IsCaptureList*/false, SourceLoc(), propertyName, swiftPropertyType, dc);
49945001
property->setInterfaceType(swiftPropertyType);
49955002

49965003
// Note that we've formed this property.
@@ -7041,11 +7048,13 @@ ClangImporter::Implementation::createConstant(Identifier name, DeclContext *dc,
70417048
VarDecl *var = nullptr;
70427049
if (ClangN) {
70437050
var = createDeclWithClangNode<VarDecl>(ClangN, Accessibility::Public,
7044-
isStatic, /*IsLet*/ false,
7045-
SourceLoc(), name, type, dc);
7051+
/*IsStatic*/isStatic, /*IsLet*/false,
7052+
/*IsCaptureList*/false, SourceLoc(),
7053+
name, type, dc);
70467054
} else {
70477055
var = new (SwiftContext)
7048-
VarDecl(isStatic, /*IsLet*/ false, SourceLoc(), name, type, dc);
7056+
VarDecl(/*IsStatic*/isStatic, /*IsLet*/false, /*IsCaptureList*/false,
7057+
SourceLoc(), name, type, dc);
70497058
}
70507059

70517060
var->setInterfaceType(type);
@@ -7149,7 +7158,9 @@ createUnavailableDecl(Identifier name, DeclContext *dc, Type type,
71497158

71507159
// Create a new VarDecl with dummy type.
71517160
auto var = createDeclWithClangNode<VarDecl>(ClangN, Accessibility::Public,
7152-
isStatic, /*IsLet*/ false,
7161+
/*IsStatic*/isStatic,
7162+
/*IsLet*/false,
7163+
/*IsCaptureList*/false,
71537164
SourceLoc(), name, type, dc);
71547165
var->setInterfaceType(type);
71557166
markUnavailable(var, UnavailableMessage);

‎lib/Parse/ParseExpr.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2154,23 +2154,23 @@ parseClosureSignatureIfPresent(SmallVectorImpl<CaptureListEntry> &captureList,
21542154
// the initializer expression is evaluated before the closure is formed.
21552155
auto *VD = new (Context) VarDecl(/*isStatic*/false,
21562156
/*isLet*/ownershipKind !=Ownership::Weak,
2157+
/*isCaptureList*/true,
21572158
nameLoc, name, Type(), CurDeclContext);
2159+
21582160
// Attributes.
21592161
if (ownershipKind != Ownership::Strong)
21602162
VD->getAttrs().add(new (Context) OwnershipAttr(ownershipKind));
2161-
2163+
21622164
auto pattern = new (Context) NamedPattern(VD, /*implicit*/true);
2163-
2165+
21642166
auto *PBD = PatternBindingDecl::create(Context, /*staticloc*/SourceLoc(),
21652167
StaticSpellingKind::None,
21662168
nameLoc, pattern, initializer,
21672169
CurDeclContext);
2168-
2169-
2170-
2170+
21712171
captureList.push_back(CaptureListEntry(VD, PBD));
21722172
} while (consumeIf(tok::comma));
2173-
2173+
21742174
// The capture list needs to be closed off with a ']'.
21752175
if (!consumeIf(tok::r_square)) {
21762176
diagnose(Tok, diag::expected_capture_list_end_rsquare);

‎lib/Parse/ParsePattern.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -819,8 +819,9 @@ ParserResult<Pattern> Parser::parsePattern() {
819819

820820
Pattern *Parser::createBindingFromPattern(SourceLoc loc, Identifier name,
821821
bool isLet) {
822-
auto var = new (Context) VarDecl(/*static*/ false, /*IsLet*/ isLet,
823-
loc, name, Type(), CurDeclContext);
822+
auto var = new (Context) VarDecl(/*IsStatic*/false, /*IsLet*/isLet,
823+
/*IsCaptureList*/false, loc, name, Type(),
824+
CurDeclContext);
824825
return new (Context) NamedPattern(var);
825826
}
826827

‎lib/Parse/ParseStmt.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -914,9 +914,9 @@ static void parseGuardedPattern(Parser &P, GuardedPattern &result,
914914
P.Tok.isAny(tok::l_brace, tok::kw_where)) {
915915
auto loc = P.Tok.getLoc();
916916
auto errorName = P.Context.Id_error;
917-
auto var = new (P.Context) VarDecl(/*static*/ false, /*IsLet*/true,
918-
loc, errorName, Type(),
919-
P.CurDeclContext);
917+
auto var = new (P.Context) VarDecl(/*IsStatic*/false, /*IsLet*/true,
918+
/*IsCaptureList*/false, loc, errorName,
919+
Type(), P.CurDeclContext);
920920
var->setImplicit();
921921
auto namePattern = new (P.Context) NamedPattern(var);
922922
auto varPattern = new (P.Context) VarPattern(loc, /*isLet*/true,

‎lib/Sema/CSDiag.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -670,8 +670,10 @@ static void diagnoseSubElementFailure(Expr *destExpr,
670670
std::string message = "'";
671671
message += VD->getName().str().str();
672672
message += "'";
673-
674-
if (VD->isImplicit())
673+
674+
if (VD->isCaptureList())
675+
message += " is an immutable capture";
676+
else if (VD->isImplicit())
675677
message += " is immutable";
676678
else if (VD->isLet())
677679
message += " is a 'let' constant";
@@ -684,7 +686,7 @@ static void diagnoseSubElementFailure(Expr *destExpr,
684686
}
685687
TC.diagnose(loc, diagID, message)
686688
.highlight(immInfo.first->getSourceRange());
687-
689+
688690
// If this is a simple variable marked with a 'let', emit a note to fixit
689691
// hint it to 'var'.
690692
VD->emitLetToVarNoteIfSimple(CS.DC);

0 commit comments

Comments
 (0)