Skip to content

Preserve default argument text through serialization #18579

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
Aug 9, 2018
Merged

Preserve default argument text through serialization #18579

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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ CHANGELOG
Swift 5.0
---------

* [SR-2608][]

Default arguments are now printed in SourceKit-generated interfaces for Swift
modules, instead of just using a placeholder `default`.

* Notable bug fix: unowned and unowned(unsafe) variables now support optional
types.

Expand Down Expand Up @@ -7151,3 +7156,4 @@ Swift 1.0
[SR-2131]: <https://bugs.swift.org/browse/SR-2131>
[SR-2388]: <https://bugs.swift.org/browse/SR-2388>
[SR-2394]: <https://bugs.swift.org/browse/SR-2394>
[SR-2608]: <https://bugs.swift.org/browse/SR-2608>
12 changes: 12 additions & 0 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4694,6 +4694,7 @@ class ParamDecl : public VarDecl {
struct StoredDefaultArgument {
Expr *DefaultArg = nullptr;
Initializer *InitContext = nullptr;
StringRef StringRepresentation;
};

/// The default value, if any, along with whether this is varargs.
Expand Down Expand Up @@ -4757,6 +4758,17 @@ class ParamDecl : public VarDecl {

void setDefaultArgumentInitContext(Initializer *initContext);

/// Returns a saved string representation of the parameter's default value.
///
/// This should only be called if the default value expression is absent or
/// doesn't have a valid source range; otherwise, clients should extract the
/// source text from that range.
///
/// \sa getDefaultValue
StringRef getDefaultValueStringRepresentation() const;

void setDefaultValueStringRepresentation(StringRef stringRepresentation);

/// Whether or not this parameter is varargs.
bool isVariadic() const { return DefaultValueAndIsVariadic.getInt(); }
void setVariadic(bool value = true) {DefaultValueAndIsVariadic.setInt(value);}
Expand Down
4 changes: 0 additions & 4 deletions include/swift/AST/DefaultArgumentKind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,6 @@ enum class DefaultArgumentKind : uint8_t {
};
enum { NumDefaultArgumentKindBits = 4 };

/// Retrieve the spelling of this default argument in source code, or
/// an empty string if it has none.
llvm::StringRef getDefaultArgumentSpelling(DefaultArgumentKind kind);

} // end namespace swift

#endif // LLVM_SWIFT_DEFAULTARGUMENTKIND_H
Expand Down
15 changes: 8 additions & 7 deletions include/swift/Serialization/ModuleFormat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const uint16_t VERSION_MAJOR = 0;
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
/// Don't worry about adhering to the 80-column limit for this line.
const uint16_t VERSION_MINOR = 431; // Last change: eliminate PARAMETERLIST_ELT
const uint16_t VERSION_MINOR = 432; // Last change: default argument text

using DeclIDField = BCFixed<31>;

Expand Down Expand Up @@ -1003,13 +1003,14 @@ namespace decls_block {

using ParamLayout = BCRecordLayout<
PARAM_DECL,
IdentifierIDField, // argument name
IdentifierIDField, // parameter name
DeclContextIDField, // context decl
IdentifierIDField, // argument name
IdentifierIDField, // parameter name
DeclContextIDField, // context decl
VarDeclSpecifierField, // specifier
TypeIDField, // interface type
BCFixed<1>, // isVariadic?
DefaultArgumentField // default argument
TypeIDField, // interface type
BCFixed<1>, // isVariadic?
DefaultArgumentField, // default argument kind
BCBlob // default argument text
>;

using FuncLayout = BCRecordLayout<
Expand Down
5 changes: 3 additions & 2 deletions lib/AST/ASTPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2327,8 +2327,8 @@ void PrintAST::printOneParameter(const ParamDecl *param,
Printer << "...";

if (param->isDefaultArgument()) {
auto defaultArgStr
= getDefaultArgumentSpelling(param->getDefaultArgumentKind());
StringRef defaultArgStr = param->getDefaultValueStringRepresentation();

if (defaultArgStr.empty()) {
if (Options.PrintDefaultParameterPlaceholder)
Printer << " = " << tok::kw_default;
Expand All @@ -2341,6 +2341,7 @@ void PrintAST::printOneParameter(const ParamDecl *param,
case DefaultArgumentKind::Column:
case DefaultArgumentKind::Function:
case DefaultArgumentKind::DSOHandle:
case DefaultArgumentKind::NilLiteral:
Printer.printKeyword(defaultArgStr);
break;
default:
Expand Down
1 change: 0 additions & 1 deletion lib/AST/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ add_swift_library(swiftAST STATIC
Decl.cpp
DeclContext.cpp
DeclNameLoc.cpp
DefaultArgumentKind.cpp
DiagnosticConsumer.cpp
DiagnosticEngine.cpp
DiagnosticList.cpp
Expand Down
37 changes: 37 additions & 0 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4830,6 +4830,43 @@ void ParamDecl::setDefaultArgumentInitContext(Initializer *initContext) {
DefaultValueAndIsVariadic.getPointer()->InitContext = initContext;
}

StringRef ParamDecl::getDefaultValueStringRepresentation() const {
switch (getDefaultArgumentKind()) {
case DefaultArgumentKind::None:
llvm_unreachable("called on a ParamDecl with no default value");
case DefaultArgumentKind::Normal:
assert(DefaultValueAndIsVariadic.getPointer() &&
"default value not provided yet");
return DefaultValueAndIsVariadic.getPointer()->StringRepresentation;
case DefaultArgumentKind::Inherited:
// FIXME: This needs /some/ kind of textual representation, but this isn't
// a great one.
return "super";
case DefaultArgumentKind::File: return "#file";
case DefaultArgumentKind::Line: return "#line";
case DefaultArgumentKind::Column: return "#column";
case DefaultArgumentKind::Function: return "#function";
case DefaultArgumentKind::DSOHandle: return "#dsohandle";
case DefaultArgumentKind::NilLiteral: return "nil";
case DefaultArgumentKind::EmptyArray: return "[]";
case DefaultArgumentKind::EmptyDictionary: return "[:]";
}
}

void
ParamDecl::setDefaultValueStringRepresentation(StringRef stringRepresentation) {
assert(getDefaultArgumentKind() == DefaultArgumentKind::Normal);
assert(!stringRepresentation.empty());

if (!DefaultValueAndIsVariadic.getPointer()) {
DefaultValueAndIsVariadic.setPointer(
getASTContext().Allocate<StoredDefaultArgument>());
}

DefaultValueAndIsVariadic.getPointer()->StringRepresentation =
stringRepresentation;
}

void DefaultArgumentInitializer::changeFunction(
DeclContext *parent, ParameterList *paramList) {
if (parent->isLocalContext()) {
Expand Down
39 changes: 0 additions & 39 deletions lib/AST/DefaultArgumentKind.cpp
View file
Open in desktop

This file was deleted.

19 changes: 13 additions & 6 deletions lib/Parse/ParsePattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,9 @@ void Parser::DefaultArgumentInfo::setFunctionContext(
}
}

static ParserStatus parseDefaultArgument(Parser &P,
Parser::DefaultArgumentInfo *defaultArgs,
unsigned argIndex,
Expr *&init,
Parser::ParameterContextKind paramContext) {
static ParserStatus parseDefaultArgument(
Parser &P, Parser::DefaultArgumentInfo *defaultArgs, unsigned argIndex,
Expr *&init, Parser::ParameterContextKind paramContext) {
SyntaxParsingContext DefaultArgContext(P.SyntaxContext,
SyntaxKind::InitializerClause);
SourceLoc equalLoc = P.consumeToken(tok::equal);
Expand Down Expand Up @@ -576,8 +574,17 @@ mapParsedParameters(Parser &parser,
paramContext == Parser::ParameterContextKind::Initializer ||
paramContext == Parser::ParameterContextKind::EnumElement) &&
"Default arguments are only permitted on the first param clause");
result->setDefaultArgumentKind(getDefaultArgKind(param.DefaultArg));
DefaultArgumentKind kind = getDefaultArgKind(param.DefaultArg);
result->setDefaultArgumentKind(kind);
result->setDefaultValue(param.DefaultArg);
if (kind == DefaultArgumentKind::Normal) {
SourceRange defaultArgRange = param.DefaultArg->getSourceRange();
CharSourceRange charRange =
Lexer::getCharSourceRangeFromSourceRange(parser.SourceMgr,
defaultArgRange);
StringRef defaultArgText = parser.SourceMgr.extractText(charRange);
result->setDefaultValueStringRepresentation(defaultArgText);
}
}

elements.push_back(result);
Expand Down
5 changes: 4 additions & 1 deletion lib/Serialization/Deserialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3057,8 +3057,11 @@ ModuleFile::getDeclCheckedImpl(DeclID DID, Optional<DeclContext *> ForcedContext

// Decode the default argument kind.
// FIXME: Default argument expression, if available.
if (auto defaultArg = getActualDefaultArgKind(rawDefaultArg))
if (auto defaultArg = getActualDefaultArgKind(rawDefaultArg)) {
param->setDefaultArgumentKind(*defaultArg);
if (!blobData.empty())
param->setDefaultValueStringRepresentation(blobData);
Copy link
Member

@rintaro rintaro Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Probably I'm missing something. But, where the blobData come from? Isn't this StringRef backed by SmallVector<uint64_t, 64> scratch? Is it safe to store it in AST nodes as a StringRef?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I read the source of BitstreamReader, and I understand it now :)

}
break;
}

Expand Down
9 changes: 8 additions & 1 deletion lib/Serialization/Serialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3168,6 +3168,12 @@ void Serializer::writeDecl(const Decl *D) {
auto contextID = addDeclContextRef(param->getDeclContext());
Type interfaceType = param->getInterfaceType();

// Only save the text for normal default arguments, not any of the special
// ones.
StringRef defaultArgumentText;
if (param->getDefaultArgumentKind() == swift::DefaultArgumentKind::Normal)
defaultArgumentText = param->getDefaultValueStringRepresentation();

unsigned abbrCode = DeclTypeAbbrCodes[ParamLayout::Code];
ParamLayout::emitRecord(Out, ScratchRecord, abbrCode,
addDeclBaseNameRef(param->getArgumentName()),
Expand All @@ -3176,7 +3182,8 @@ void Serializer::writeDecl(const Decl *D) {
getRawStableVarDeclSpecifier(param->getSpecifier()),
addTypeRef(interfaceType),
param->isVariadic(),
getRawStableDefaultArgumentKind(param->getDefaultArgumentKind()));
getRawStableDefaultArgumentKind(param->getDefaultArgumentKind()),
defaultArgumentText);

if (interfaceType->hasError()) {
param->getDeclContext()->dumpContext();
Expand Down
4 changes: 2 additions & 2 deletions test/IDE/print_ast_tc_decls.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,10 @@ struct d0100_FooStruct {
// PASS_COMMON-NEXT: {{^}} func instanceFunc3(a: Int, b: Double){{$}}

func instanceFuncWithDefaultArg1(a: Int = 0) {}
// PASS_COMMON-NEXT: {{^}} func instanceFuncWithDefaultArg1(a: Int = default){{$}}
// PASS_COMMON-NEXT: {{^}} func instanceFuncWithDefaultArg1(a: Int = 0){{$}}

func instanceFuncWithDefaultArg2(a: Int = 0, b: Double = 0) {}
// PASS_COMMON-NEXT: {{^}} func instanceFuncWithDefaultArg2(a: Int = default, b: Double = default){{$}}
// PASS_COMMON-NEXT: {{^}} func instanceFuncWithDefaultArg2(a: Int = 0, b: Double = 0){{$}}

func varargInstanceFunc0(v: Int...) {}
// PASS_COMMON-NEXT: {{^}} func varargInstanceFunc0(v: Int...){{$}}
Expand Down
4 changes: 2 additions & 2 deletions test/IDE/print_type_interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ extension D {
// RUN: %target-swift-ide-test -print-type-interface -usr=_TtGSaSS_ -module-name print_type_interface -source-filename %s | %FileCheck %s -check-prefix=TYPE5
// TYPE5-DAG: public func prefix(_ maxLength: Int) -> ArraySlice<String>
// TYPE5-DAG: public func suffix(_ maxLength: Int) -> ArraySlice<String>
// TYPE5-DAG: public func split(separator: String, maxSplits: Int = default, omittingEmptySubsequences: Bool = default) -> [ArraySlice<String>]
// TYPE5-DAG: public func split(separator: String, maxSplits: Int = Int.max, omittingEmptySubsequences: Bool = true) -> [ArraySlice<String>]
// TYPE5-DAG: public func formIndex(_ i: inout Int, offsetBy distance: Int)
// TYPE5-DAG: public func distance(from start: Int, to end: Int) -> Int
// TYPE5-DAG: public func joined(separator: String = default) -> String
// TYPE5-DAG: public func joined(separator: String = "") -> String
15 changes: 15 additions & 0 deletions test/Serialization/function-default-args.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module -o %t/Test~partial.swiftmodule -module-name Test -primary-file %s
// RUN: %target-swift-frontend -merge-modules -emit-module -o %t/Test.swiftmodule %t/Test~partial.swiftmodule
// RUN: %target-swift-ide-test -print-module -module-to-print=Test -source-filename=x -I %t | %FileCheck %s

// CHECK-LABEL: func testDefaultArguments(
public func testDefaultArguments(
// CHECK-SAME: normal: Int = 0
normal: Int = 0,
// CHECK-SAME: multiToken: Int = Int.max
multiToken: Int = Int.max,
// CHECK-SAME: special: Int = #line
special: Int = #line
) {}
// CHECK-SAME: )
16 changes: 7 additions & 9 deletions test/SourceKit/CursorInfo/cursor_info.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,8 +328,8 @@ enum E7: String {

// RUN: %sourcekitd-test -req=cursor -pos=31:7 %s -- -F %S/../Inputs/libIDE-mock-sdk -I %t.tmp %mcp_opt %s | %FileCheck -check-prefix=CHECK13 %s
// CHECK13: source.lang.swift.decl.function.free (31:6-31:37)
// CHECK13: <Declaration>func testDefaultParam(arg1: <Type usr="s:Si">Int</Type> = default)</Declaration>
// CHECK13: <decl.function.free><syntaxtype.keyword>func</syntaxtype.keyword> <decl.name>testDefaultParam</decl.name>(<decl.var.parameter><decl.var.parameter.argument_label>arg1</decl.var.parameter.argument_label>: <decl.var.parameter.type><ref.struct usr="s:Si">Int</ref.struct></decl.var.parameter.type> = <syntaxtype.keyword>default</syntaxtype.keyword></decl.var.parameter>)</decl.function.free>
// CHECK13: <Declaration>func testDefaultParam(arg1: <Type usr="s:Si">Int</Type> = 0)</Declaration>
// CHECK13: <decl.function.free><syntaxtype.keyword>func</syntaxtype.keyword> <decl.name>testDefaultParam</decl.name>(<decl.var.parameter><decl.var.parameter.argument_label>arg1</decl.var.parameter.argument_label>: <decl.var.parameter.type><ref.struct usr="s:Si">Int</ref.struct></decl.var.parameter.type> = 0</decl.var.parameter>)</decl.function.free>

// RUN: %sourcekitd-test -req=cursor -pos=34:4 %s -- -F %S/../Inputs/libIDE-mock-sdk -I %t.tmp %mcp_opt %s | %FileCheck -check-prefix=CHECK14 %s
// CHECK14: source.lang.swift.ref.function.free ({{.*}}Foo.framework/Frameworks/FooSub.framework/Headers/FooSub.h:4:5-4:16)
Expand Down Expand Up @@ -652,13 +652,11 @@ enum E7: String {
// CHECK73-SAME: = <syntaxtype.keyword>#file</syntaxtype.keyword>
// CHECK73-SAME: = <syntaxtype.keyword>#line</syntaxtype.keyword>
// CHECK73-SAME: = <syntaxtype.keyword>#column</syntaxtype.keyword>
// FIXME: []
// CHECK73-SAME: = <syntaxtype.keyword>default</syntaxtype.keyword>
// FIXME: [:]
// CHECK73-SAME: = <syntaxtype.keyword>default</syntaxtype.keyword>
// FIXME: keyword nil
// CHECK73-SAME: = <syntaxtype.keyword>default</syntaxtype.keyword>
// CHECK73-SAME: = <syntaxtype.keyword>default</syntaxtype.keyword>
// CHECK73-SAME: = []
// CHECK73-SAME: = [:]
// FIXME: should be <syntaxtype.keyword>nil</syntaxtype.keyword>
// CHECK73-SAME: = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why this isn't getting the keyword treatment? Is it coming through as a "normal" default argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, any explicitly-written non-magic-literal expression is still "normal". I suppose we could special-case "the single token 'nil'" and see if that works, but it didn't seem too important to me when other default arguments are also not syntax-highlighted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, so are DefaultArgumentKind::NilLiteral (and the empty literals too I guess) unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're used by the importer, since synthesizing instructions and type-checking them would have been a pain. It also mattered back before default arguments were inlinable, because you definitely don't want to call a function just to get nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experimented with the "see if the expression is a NilLiteralExpr" approach and it fell over in Sema. It's probably still doable, but I think I'm just going to leave it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

// CHECK73-SAME: = 1

// RUN: %sourcekitd-test -req=cursor -pos=162:8 %s -- -F %S/../Inputs/libIDE-mock-sdk -I %t.tmp %mcp_opt %s | %FileCheck %s -check-prefix=CHECK74
// CHECK74: source.lang.swift.decl.function.method.instance (162:8-162:20)
Expand Down