Skip to content

Commit 8923a12

Browse files
milsemanmilseman
committed
[ASTPrinter] Switch to new ParameterTypeFlags
Switch printing off of using Function's ExtInfo for autoclosure and escaping, and onto the ParameterTypeFlags, which let us do precise and accurate context-sensitive printing of these parameter type attributes. This fixes a huge list of issues where we were printing @escaping for things like optional ObjC completion handlers, among many others. We now correctly print @escaping in more places, and don't print it when it's not correct. Also updates the dumper to be consistent and give a good view of the AST as represented in memory. Tests updated, more involved testing coming soon.
1 parent ed2522b commit 8923a12

File tree

13 files changed

+1286
-1235
lines changed

13 files changed

+1286
-1235
lines changed

‎include/swift/AST/PrintOptions.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,9 @@ struct PrintOptions {
245245
/// protocol requirements.
246246
bool SkipOverrides = false;
247247

248+
/// Whether to skip parameter type attributes
249+
bool SkipParameterTypeAttributes = false;
250+
248251
/// Whether to print a long attribute like '\@available' on a separate line
249252
/// from the declaration or other attributes.
250253
bool PrintLongAttrsOnSeparateLines = false;

‎lib/AST/ASTDumper.cpp

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2628,6 +2628,15 @@ namespace {
26282628
return OS;
26292629
}
26302630

2631+
void dumpParameterFlags(ParameterTypeFlags paramFlags) {
2632+
if (paramFlags.isVariadic())
2633+
printFlag("vararg");
2634+
if (paramFlags.isAutoClosure())
2635+
printFlag("autoclosure");
2636+
if (paramFlags.isEscaping())
2637+
printFlag("escaping");
2638+
}
2639+
26312640
public:
26322641
PrintType(raw_ostream &os, unsigned indent) : OS(os), Indent(indent) { }
26332642

@@ -2691,6 +2700,7 @@ namespace {
26912700

26922701
void visitParenType(ParenType *T, StringRef label) {
26932702
printCommon(T, label, "paren_type");
2703+
dumpParameterFlags(T->getParameterFlags());
26942704
printRec(T->getUnderlyingType());
26952705
OS << ")";
26962706
}
@@ -2705,9 +2715,7 @@ namespace {
27052715
PrintWithColorRAII(OS, TypeFieldColor) << "tuple_type_elt";
27062716
if (elt.hasName())
27072717
printField("name", elt.getName().str());
2708-
if (elt.isVararg())
2709-
printFlag("vararg");
2710-
2718+
dumpParameterFlags(elt.getParameterFlags());
27112719
printRec(elt.getType());
27122720
OS << ")";
27132721
}
@@ -2918,10 +2926,7 @@ namespace {
29182926
}
29192927

29202928
printFlag(T->isAutoClosure(), "autoclosure");
2921-
2922-
// Dump out either @noescape or @escaping
2923-
printFlag(!T->isNoEscape(), "@escaping");
2924-
2929+
printFlag(!T->isNoEscape(), "escaping");
29252930
printFlag(T->throws(), "throws");
29262931

29272932
printRec("input", T->getInput());

‎lib/AST/ASTPrinter.cpp

Lines changed: 100 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,20 @@ void StreamPrinter::printText(StringRef Text) {
989989
OS << Text;
990990
}
991991

992+
/// Whether we will be printing a TypeLoc by using the TypeRepr printer
993+
static bool willUseTypeReprPrinting(TypeLoc tyLoc,
994+
const PrintOptions &options) {
995+
// Special case for when transforming archetypes
996+
if (options.TransformContext && tyLoc.getType() &&
997+
options.TransformContext->transform(tyLoc.getType()))
998+
return false;
999+
1000+
// Otherwise, whether we have a type repr and prefer that
1001+
return ((options.PreferTypeRepr && tyLoc.hasLocation()) ||
1002+
tyLoc.getType().isNull()) &&
1003+
tyLoc.getTypeRepr();
1004+
}
1005+
9921006
namespace {
9931007
/// \brief AST pretty-printer.
9941008
class PrintAST : public ASTVisitor<PrintAST> {
@@ -1184,8 +1198,11 @@ class PrintAST : public ASTVisitor<PrintAST> {
11841198
// is null.
11851199
if ((Options.PreferTypeRepr && TL.hasLocation()) ||
11861200
TL.getType().isNull()) {
1187-
if (auto repr = TL.getTypeRepr())
1201+
if (auto repr = TL.getTypeRepr()) {
1202+
llvm::SaveAndRestore<bool> SPTA(Options.SkipParameterTypeAttributes,
1203+
true);
11881204
repr->print(Printer, Options);
1205+
}
11891206
return;
11901207
}
11911208

@@ -1230,10 +1247,10 @@ class PrintAST : public ASTVisitor<PrintAST> {
12301247
/// \returns true if anything was printed.
12311248
bool printASTNodes(const ArrayRef<ASTNode> &Elements, bool NeedIndent = true);
12321249

1233-
void printOneParameter(const ParamDecl *param, bool Curried,
1234-
bool ArgNameIsAPIByDefault);
1250+
void printOneParameter(const ParamDecl *param, ParameterTypeFlags paramFlags,
1251+
bool Curried, bool ArgNameIsAPIByDefault);
12351252

1236-
void printParameterList(ParameterList *PL, bool isCurried,
1253+
void printParameterList(ParameterList *PL, Type paramListTy, bool isCurried,
12371254
std::function<bool()> isAPINameByDefault);
12381255

12391256
/// \brief Print the function parameters in curried or selector style,
@@ -2520,6 +2537,14 @@ static bool isStructOrClassContext(DeclContext *dc) {
25202537
return false;
25212538
}
25222539

2540+
static void printParameterFlags(ASTPrinter &printer, PrintOptions options,
2541+
ParameterTypeFlags flags) {
2542+
if (!options.excludeAttrKind(TAK_autoclosure) && flags.isAutoClosure())
2543+
printer << "@autoclosure ";
2544+
if (!options.excludeAttrKind(TAK_escaping) && flags.isEscaping())
2545+
printer << "@escaping ";
2546+
}
2547+
25232548
void PrintAST::visitVarDecl(VarDecl *decl) {
25242549
printDocumentationComment(decl);
25252550
// Print @sil_stored when the attribute is not already
@@ -2556,7 +2581,8 @@ void PrintAST::visitParamDecl(ParamDecl *decl) {
25562581
visitVarDecl(decl);
25572582
}
25582583

2559-
void PrintAST::printOneParameter(const ParamDecl *param, bool Curried,
2584+
void PrintAST::printOneParameter(const ParamDecl *param,
2585+
ParameterTypeFlags paramFlags, bool Curried,
25602586
bool ArgNameIsAPIByDefault) {
25612587
Printer.callPrintStructurePre(PrintStructureKind::FunctionParameter, param);
25622588
SWIFT_DEFER {
@@ -2610,7 +2636,25 @@ void PrintAST::printOneParameter(const ParamDecl *param, bool Curried,
26102636
// Set and restore in-parameter-position printing of types
26112637
{
26122638
llvm::SaveAndRestore<bool> savePrintParam(Options.PrintAsInParamType, true);
2639+
// FIXME: don't do if will be using type repr printing
2640+
printParameterFlags(Printer, Options, paramFlags);
2641+
2642+
// Special case, if we're not going to use the type repr printing, peek
2643+
// through the paren types so that we don't print excessive @escapings
2644+
unsigned numParens = 0;
2645+
if (!willUseTypeReprPrinting(TheTypeLoc, Options)) {
2646+
while (auto parenTy =
2647+
dyn_cast<ParenType>(TheTypeLoc.getType().getPointer())) {
2648+
++numParens;
2649+
TheTypeLoc = TypeLoc::withoutLoc(parenTy->getUnderlyingType());
2650+
}
2651+
}
2652+
2653+
for (unsigned i = 0; i < numParens; ++i)
2654+
Printer << "(";
26132655
printTypeLoc(TheTypeLoc);
2656+
for (unsigned i = 0; i < numParens; ++i)
2657+
Printer << ")";
26142658
}
26152659

26162660
if (param->isVariadic())
@@ -2641,28 +2685,68 @@ void PrintAST::printOneParameter(const ParamDecl *param, bool Curried,
26412685
}
26422686
}
26432687

2644-
void PrintAST::printParameterList(ParameterList *PL, bool isCurried,
2645-
std::function<bool()> isAPINameByDefault) {
2688+
void PrintAST::printParameterList(ParameterList *PL, Type paramListTy,
2689+
bool isCurried,
2690+
std::function<bool()> isAPINameByDefault) {
2691+
SmallVector<ParameterTypeFlags, 4> paramFlags;
2692+
if (paramListTy) {
2693+
if (auto parenTy = dyn_cast<ParenType>(paramListTy.getPointer())) {
2694+
paramFlags.push_back(parenTy->getParameterFlags());
2695+
} else if (auto tupleTy = paramListTy->getAs<TupleType>()) {
2696+
for (auto elt : tupleTy->getElements())
2697+
paramFlags.push_back(elt.getParameterFlags());
2698+
} else {
2699+
paramFlags.push_back({});
2700+
}
2701+
} else {
2702+
// Malformed AST, just use default flags
2703+
paramFlags.resize(PL->size());
2704+
}
2705+
26462706
Printer << "(";
26472707
for (unsigned i = 0, e = PL->size(); i != e; ++i) {
26482708
if (i > 0)
26492709
Printer << ", ";
26502710

2651-
printOneParameter(PL->get(i), isCurried, isAPINameByDefault());
2711+
printOneParameter(PL->get(i), paramFlags[i], isCurried,
2712+
isAPINameByDefault());
26522713
}
26532714
Printer << ")";
26542715
}
26552716

26562717
void PrintAST::printFunctionParameters(AbstractFunctionDecl *AFD) {
26572718
auto BodyParams = AFD->getParameterLists();
2719+
auto curTy = AFD->hasType() ? AFD->getType() : nullptr;
26582720

26592721
// Skip over the implicit 'self'.
2660-
if (AFD->getImplicitSelfDecl())
2722+
if (AFD->getImplicitSelfDecl()) {
26612723
BodyParams = BodyParams.slice(1);
2724+
if (curTy)
2725+
if (auto funTy = curTy->getAs<AnyFunctionType>())
2726+
curTy = funTy->getResult();
2727+
}
2728+
2729+
SmallVector<Type, 4> parameterListTypes;
2730+
for (auto i = 0; i < BodyParams.size(); ++i) {
2731+
if (curTy) {
2732+
if (auto funTy = curTy->getAs<AnyFunctionType>()) {
2733+
parameterListTypes.push_back(funTy->getInput());
2734+
if (i < BodyParams.size() - 1)
2735+
curTy = funTy->getResult();
2736+
} else {
2737+
parameterListTypes.push_back(curTy);
2738+
}
2739+
}
2740+
}
26622741

26632742
for (unsigned CurrPattern = 0, NumPatterns = BodyParams.size();
26642743
CurrPattern != NumPatterns; ++CurrPattern) {
2665-
printParameterList(BodyParams[CurrPattern], /*Curried=*/CurrPattern > 0,
2744+
// Be extra careful in the event of printing mal-formed ASTs
2745+
auto paramListType = parameterListTypes.size() > CurrPattern
2746+
? parameterListTypes[CurrPattern]
2747+
: nullptr;
2748+
printParameterList(BodyParams[CurrPattern], paramListType,
2749+
/*Curried=*/CurrPattern > 0,
26662750
[&]()->bool {
26672751
return CurrPattern > 0 || AFD->argumentNameIsAPIByDefault();
26682752
});
@@ -2915,7 +2999,8 @@ void PrintAST::visitSubscriptDecl(SubscriptDecl *decl) {
29152999
recordDeclLoc(decl, [&]{
29163000
Printer << "subscript";
29173001
}, [&] { // Parameters
2918-
printParameterList(decl->getIndices(), /*Curried=*/false,
3002+
printParameterList(decl->getIndices(), decl->getIndicesType(),
3003+
/*Curried=*/false,
29193004
/*isAPINameByDefault*/[]()->bool{return false;});
29203005
});
29213006
Printer << " -> ";
@@ -3642,6 +3727,7 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
36423727

36433728
void visitParenType(ParenType *T) {
36443729
Printer << "(";
3730+
printParameterFlags(Printer, Options, T->getParameterFlags());
36453731
visit(T->getUnderlyingType());
36463732
Printer << ")";
36473733
}
@@ -3671,8 +3757,10 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
36713757
if (TD.isVararg()) {
36723758
visit(TD.getVarargBaseTy());
36733759
Printer << "...";
3674-
} else
3760+
} else {
3761+
printParameterFlags(Printer, Options, TD.getParameterFlags());
36753762
visit(EltType);
3763+
}
36763764
}
36773765
Printer << ")";
36783766
}
@@ -3804,13 +3892,6 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
38043892
if (Options.SkipAttributes)
38053893
return;
38063894

3807-
if (info.isAutoClosure() && !Options.excludeAttrKind(TAK_autoclosure)) {
3808-
Printer.printSimpleAttr("@autoclosure") << " ";
3809-
}
3810-
if (inParameterPrinting && !info.isNoEscape() &&
3811-
!Options.excludeAttrKind(TAK_escaping)) {
3812-
Printer.printSimpleAttr("@escaping") << " ";
3813-
}
38143895

38153896
if (Options.PrintFunctionRepresentationAttrs &&
38163897
!Options.excludeAttrKind(TAK_convention) &&

‎lib/AST/TypeRepr.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -282,10 +282,12 @@ void AttributedTypeRepr::printAttrs(ASTPrinter &Printer,
282282
return Attrs.has(K);
283283
};
284284

285-
if (hasAttr(TAK_autoclosure))
286-
Printer.printSimpleAttr("@autoclosure") << " ";
287-
if (hasAttr(TAK_escaping))
288-
Printer.printSimpleAttr("@escaping") << " ";
285+
if (!Options.SkipParameterTypeAttributes) {
286+
if (hasAttr(TAK_autoclosure))
287+
Printer.printSimpleAttr("@autoclosure") << " ";
288+
if (hasAttr(TAK_escaping))
289+
Printer.printSimpleAttr("@escaping") << " ";
290+
}
289291

290292
if (hasAttr(TAK_thin))
291293
Printer.printSimpleAttr("@thin") << " ";

‎test/Constraints/diagnostics.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ f1(
6060
)
6161

6262
f3(
63-
f2 // expected-error {{cannot convert value of type '((@escaping (Int) -> Int)) -> Int' to expected argument type '(@escaping (Int) -> Float) -> Int'}}
63+
f2 // expected-error {{cannot convert value of type '(@escaping ((Int) -> Int)) -> Int' to expected argument type '(@escaping (Int) -> Float) -> Int'}}
6464
)
6565

6666
f4(i, d) // expected-error {{extra argument in call}}
@@ -672,7 +672,7 @@ func overloadSetResultType(_ a : Int, b : Int) -> Int {
672672
// https://twitter.com/_jlfischer/status/712337382175952896
673673
// TODO: <rdar://problem/27391581> QoI: Nonsensical "binary operator '&&' cannot be applied to two 'Bool' operands"
674674
return a == b && 1 == 2 // expected-error {{binary operator '&&' cannot be applied to two 'Bool' operands}}
675-
// expected-note @-1 {{expected an argument list of type '(Bool, @autoclosure () throws -> Bool)'}}
675+
// expected-note @-1 {{expected an argument list of type '(Bool, () throws -> Bool)'}}
676676
}
677677

678678
// <rdar://problem/21523291> compiler error message for mutating immutable field is incorrect
@@ -736,7 +736,7 @@ extension Foo23752537 {
736736
// TODO: <rdar://problem/27391581> QoI: Nonsensical "binary operator '&&' cannot be applied to two 'Bool' operands"
737737
// expected-error @+1 {{binary operator '&&' cannot be applied to two 'Bool' operands}}
738738
return (self.title != other.title && self.message != other.message)
739-
// expected-note @-1 {{expected an argument list of type '(Bool, @autoclosure () throws -> Bool)'}}
739+
// expected-note @-1 {{expected an argument list of type '(Bool, () throws -> Bool)'}}
740740
}
741741
}
742742

‎test/IDE/complete_override.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -508,12 +508,12 @@ class Deprecated2 : Deprecated1 {
508508
// DEPRECATED_1: Decl[InstanceMethod]/Super/NotRecommended: deprecated() {|};
509509

510510
class EscapingBase {
511-
func method(_ x: @escaping (@escaping ()->()) -> (@escaping ()->())) -> (@escaping (@escaping ()->() )->()) { }
511+
func method(_ x: @escaping (@escaping ()->()) -> (()->())) -> (@escaping (@escaping ()->() )->()) { }
512512
}
513513
class Escaping : EscapingBase {
514514
override func #^ESCAPING_1^#
515515
}
516-
// ESCAPING_1: Decl[InstanceMethod]/Super: method(_ x: @escaping (@escaping () -> ()) -> (@escaping () -> ())) -> ((@escaping () -> ()) -> ()) {|};
516+
// ESCAPING_1: Decl[InstanceMethod]/Super: method(_ x: @escaping (@escaping () -> ()) -> (() -> ())) -> ((@escaping () -> ()) -> ()) {|};
517517

518518
class OverrideBase {
519519
init(x: Int) {}

‎test/IDE/print_clang_bool_bridging.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ func testCBoolFnToBlock(_: @escaping @convention(c) (Bool) -> Bool) -> (Bool) ->
3838
func testObjCBoolFnToBlock(_: @escaping @convention(c) (ObjCBool) -> ObjCBool) -> (Bool) -> Bool
3939
func testDarwinBooleanFnToBlock(_: @escaping @convention(c) (DarwinBoolean) -> DarwinBoolean) -> (Bool) -> Bool
4040

41-
func testCBoolFnToBlockTypedef(_: CBoolFn) -> CBoolBlock
42-
func testObjCBoolFnToBlockTypedef(_: ObjCBoolFn) -> ObjCBoolBlock
43-
func testDarwinBooleanFnToBlockTypedef(_: DarwinBooleanFn) -> DarwinBooleanBlock
41+
func testCBoolFnToBlockTypedef(_: @escaping CBoolFn) -> CBoolBlock
42+
func testObjCBoolFnToBlockTypedef(_: @escaping ObjCBoolFn) -> ObjCBoolBlock
43+
func testDarwinBooleanFnToBlockTypedef(_: @escaping DarwinBooleanFn) -> DarwinBooleanBlock
4444

4545
typealias CBoolFnToBlockType = (CBoolFn) -> CBoolBlock
4646
typealias ObjCCBoolFnToBlockType = (ObjCBoolFn) -> (ObjCBool) -> ObjCBool
@@ -71,9 +71,9 @@ class Test : NSObject {
7171
func testObjCBoolFn(toBlock fp: @escaping @convention(c) (ObjCBool) -> ObjCBool) -> (Bool) -> Bool
7272
func testDarwinBooleanFn(toBlock fp: @escaping @convention(c) (DarwinBoolean) -> DarwinBoolean) -> (Bool) -> Bool
7373

74-
func produceCBoolBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@escaping @convention(block) (Bool) -> Bool)?>)
75-
func produceObjCBoolBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@escaping @convention(block) (ObjCBool) -> ObjCBool)?>)
76-
func produceDarwinBooleanBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@escaping @convention(block) (DarwinBoolean) -> DarwinBoolean)?>)
74+
func produceCBoolBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@convention(block) (Bool) -> Bool)?>)
75+
func produceObjCBoolBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@convention(block) (ObjCBool) -> ObjCBool)?>)
76+
func produceDarwinBooleanBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@convention(block) (DarwinBoolean) -> DarwinBoolean)?>)
7777

7878
init()
7979
}

‎test/IDE/print_clang_decls.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,8 @@
120120
// CHECK-NULLABILITY: class SomeClass {
121121
// CHECK-NULLABILITY: class func methodA(_ obj: SomeClass?) -> Any{{$}}
122122
// CHECK-NULLABILITY: func methodA(_ obj: SomeClass?) -> Any{{$}}
123-
// CHECK-NULLABILITY: class func methodB(_ block: (@escaping (Int32, Int32) -> Int32)? = nil) -> Any{{$}}
124-
// CHECK-NULLABILITY: func methodB(_ block: (@escaping (Int32, Int32) -> Int32)? = nil) -> Any{{$}}
123+
// CHECK-NULLABILITY: class func methodB(_ block: ((Int32, Int32) -> Int32)? = nil) -> Any{{$}}
124+
// CHECK-NULLABILITY: func methodB(_ block: ((Int32, Int32) -> Int32)? = nil) -> Any{{$}}
125125
// CHECK-NULLABILITY: func methodC() -> Any?
126126
// CHECK-NULLABILITY: var property: Any?
127127
// CHECK-NULLABILITY: func stringMethod() -> String{{$}}

‎test/IDE/print_omit_needless_words.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@
8888
// CHECK-FOUNDATION-NEXT: case binary
8989

9090
// Note: Make sure NSURL works in various places
91-
// CHECK-FOUNDATION: open(_: NSURL!, completionHandler: (@escaping (Bool) -> Void)!)
91+
// CHECK-FOUNDATION: open(_: NSURL!, completionHandler: ((Bool) -> Void)!)
9292

9393
// Note: property name stripping property type.
9494
// CHECK-FOUNDATION: var uppercased: String
@@ -131,11 +131,11 @@
131131
// CHECK-FOUNDATION: static var reverse: NSEnumerationOptions
132132

133133
// Note: usingBlock -> body
134-
// CHECK-FOUNDATION: func enumerateObjects(_: (@escaping (Any?, Int, UnsafeMutablePointer<ObjCBool>?) -> Void)!)
135-
// CHECK-FOUNDATION: func enumerateObjects(options: NSEnumerationOptions = [], using: (@escaping (Any?, Int, UnsafeMutablePointer<ObjCBool>?) -> Void)!)
134+
// CHECK-FOUNDATION: func enumerateObjects(_: ((Any?, Int, UnsafeMutablePointer<ObjCBool>?) -> Void)!)
135+
// CHECK-FOUNDATION: func enumerateObjects(options: NSEnumerationOptions = [], using: ((Any?, Int, UnsafeMutablePointer<ObjCBool>?) -> Void)!)
136136

137137
// Note: WithBlock -> body, nullable closures default to nil.
138-
// CHECK-FOUNDATION: func enumerateObjectsRandomly(block: (@escaping (Any?, Int, UnsafeMutablePointer<ObjCBool>?) -> Void)? = nil)
138+
// CHECK-FOUNDATION: func enumerateObjectsRandomly(block: ((Any?, Int, UnsafeMutablePointer<ObjCBool>?) -> Void)? = nil)
139139

140140
// Note: id<Proto> treated as "Proto".
141141
// CHECK-FOUNDATION: func doSomething(with: NSCopying)

0 commit comments

Comments
 (0)