Skip to content

[SR-1052][Parser] Warning + FixIt for @warn_unused_result #2760

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
Jun 8, 2016
Merged

[SR-1052][Parser] Warning + FixIt for @warn_unused_result #2760

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
3 changes: 0 additions & 3 deletions include/swift/AST/Attr.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,6 @@ DECL_ATTR(_alignment, Alignment, OnStruct | OnEnum | UserInaccessible, 56)
SIMPLE_DECL_ATTR(rethrows, Rethrows,
OnFunc | OnConstructor | RejectByParser, 57)

DECL_ATTR(warn_unused_result, WarnUnusedResult,
OnFunc | OnConstructor | LongAttribute, 58)

DECL_ATTR(_swift_native_objc_runtime_base, SwiftNativeObjCRuntimeBase,
OnClass | UserInaccessible, 59)

Expand Down
53 changes: 0 additions & 53 deletions include/swift/AST/Attr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1079,59 +1079,6 @@ class SynthesizedProtocolAttr : public DeclAttribute {
}
};

/// The @warn_unused_result attribute, which specifies that we should
/// receive a warning if a function is called but its result is
/// unused.
///
/// The @warn_unused_result attribute can optionally be provided with
/// a message and a mutable variant. For example:
///
/// \code
/// struct X {
/// @warn_unused_result(message: "this string affects your health")
/// func methodA() -> String { ... }
///
/// @warn_unused_result(mutable_variant: "jumpInPlace")
/// func jump() -> X { ... }
///
/// mutating func jumpInPlace() { ... }
/// }
/// \endcode
class WarnUnusedResultAttr : public DeclAttribute {
/// The optional message.
const StringRef Message;

/// An optional name of the mutable variant of the given method,
/// which will be suggested as a replacement if it can be called.
const StringRef MutableVariant;

public:
/// A @warn_unused_result attribute with no options.
WarnUnusedResultAttr(SourceLoc atLoc, SourceLoc attrLoc, bool implicit)
: DeclAttribute(DAK_WarnUnusedResult, atLoc, SourceRange(atLoc, attrLoc),
implicit),
Message(""), MutableVariant("") { }

/// A @warn_unused_result attribute with a message or mutable variant.
WarnUnusedResultAttr(SourceLoc atLoc, SourceLoc attrLoc, SourceLoc lParenLoc,
StringRef message, StringRef mutableVariant,
SourceLoc rParenLoc, bool implicit)
: DeclAttribute(DAK_WarnUnusedResult, attrLoc,
SourceRange(atLoc, rParenLoc),
implicit),
Message(message), MutableVariant(mutableVariant) { }

/// Retrieve the message associated with this attribute.
StringRef getMessage() const { return Message; }

/// Retrieve the name of the mutable variant of this method.
StringRef getMutableVariant() const { return MutableVariant; }

static bool classof(const DeclAttribute *DA) {
return DA->getKind() == DAK_WarnUnusedResult;
}
};

/// The @swift3_migration attribute which describes the transformations
/// required to migrate the given Swift 2.x API to Swift 3.
class Swift3MigrationAttr : public DeclAttribute {
Expand Down
12 changes: 2 additions & 10 deletions include/swift/AST/DiagnosticsParse.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,16 +1258,8 @@ ERROR(attr_unowned_expected_rparen,none,
"expected ')' after specifier for 'unowned'", ())

// warn_unused_result
WARNING(attr_warn_unused_result_expected_name,none,
"expected parameter 'message' or 'mutable_variant'", ())
WARNING(attr_warn_unused_result_duplicate_parameter,none,
"duplicate '%0' parameter; previous value will be ignored", (StringRef))
ERROR(attr_warn_unused_result_expected_colon,none,
"expected ':' following '%0' parameter", (StringRef))
ERROR(attr_warn_unused_result_expected_string,none,
"expected a string following ':' for '%0' parameter", (StringRef))
WARNING(attr_warn_unused_result_unknown_parameter,none,
"unknown parameter '%0' in 'warn_unused_result' attribute", (StringRef))
WARNING(attr_warn_unused_result_removed,none,
"'warn_unused_result' attribute behavior is now the default", ())
ERROR(attr_warn_unused_result_expected_rparen,none,
"expected ')' after 'warn_unused_result' attribute", ())

Expand Down
2 changes: 0 additions & 2 deletions include/swift/AST/PrintOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,6 @@ struct PrintOptions {
result.SkipPrivateStdlibDecls = true;
result.SkipUnderscoredStdlibProtocols = true;
result.SkipDeinit = true;
result.ExcludeAttrList.push_back(DAK_WarnUnusedResult);
result.ExcludeAttrList.push_back(DAK_DiscardableResult);
result.EmptyLineBetweenMembers = true;
result.ElevateDocCommentFromConformance = true;
Expand Down Expand Up @@ -432,7 +431,6 @@ struct PrintOptions {
PO.PrintDocumentationComments = false;
PO.ExcludeAttrList.push_back(DAK_Available);
PO.ExcludeAttrList.push_back(DAK_Swift3Migration);
PO.ExcludeAttrList.push_back(DAK_WarnUnusedResult);
PO.SkipPrivateStdlibDecls = true;
PO.ExplodeEnumCaseDecls = true;
return PO;
Expand Down
8 changes: 0 additions & 8 deletions include/swift/Serialization/ModuleFormat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1376,14 +1376,6 @@ namespace decls_block {
BCBlob // rename, followed by message
>;

using WarnUnusedResultDeclAttrLayout = BCRecordLayout<
WarnUnusedResult_DECL_ATTR,
BCFixed<1>, // implicit flag
BCVBR<6>, // index at the end of the message,
BCBlob // blob contains the message and mutating-version
// strings, separated by the prior index
>;

using SpecializeDeclAttrLayout = BCRecordLayout<
Specialize_DECL_ATTR,
BCArray<TypeIDField> // concrete types
Expand Down
23 changes: 0 additions & 23 deletions lib/AST/Attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,27 +446,6 @@ bool DeclAttribute::printImpl(ASTPrinter &Printer, const PrintOptions &Options)
break;
}

case DAK_WarnUnusedResult: {
Printer.printAttrName("@warn_unused_result");
auto *attr = cast<WarnUnusedResultAttr>(this);
bool printedParens = false;
if (!attr->getMessage().empty()) {
Printer << "(message: \"" << attr->getMessage() << "\"";
printedParens = true;
}
if (!attr->getMutableVariant().empty()) {
if (printedParens)
Printer << ", ";
else
Printer << "(";
Printer << "mutable_variant: \"" << attr->getMutableVariant() << "\"";
printedParens = true;
}
if (printedParens)
Printer << ")";
break;
}

case DAK_Specialize: {
Printer << "@" << getAttrName() << "(";
auto *attr = cast<SpecializeAttr>(this);
Expand Down Expand Up @@ -585,8 +564,6 @@ StringRef DeclAttribute::getAttrName() const {
return "<<synthesized protocol>>";
case DAK_Swift3Migration:
return "swift3_migration";
case DAK_WarnUnusedResult:
return "warn_unused_result";
case DAK_Specialize:
return "_specialize";
}
Expand Down
6 changes: 1 addition & 5 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6314,11 +6314,7 @@ void ClangImporter::Implementation::importAttributes(
}

// Map __attribute__((warn_unused_result)).
if (ClangDecl->hasAttr<clang::WarnUnusedResultAttr>()) {
MappedDecl->getAttrs().add(new (C) WarnUnusedResultAttr(SourceLoc(),
SourceLoc(),
false));
} else {
if (!ClangDecl->hasAttr<clang::WarnUnusedResultAttr>()) {
if (auto MD = dyn_cast<FuncDecl>(MappedDecl)) {
if (!MD->getResultType()->isVoid()) {
MD->getAttrs().add(new (C) DiscardableResultAttr(/*implicit*/true));
Expand Down
148 changes: 35 additions & 113 deletions lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,119 +1087,6 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
break;
}

case DAK_WarnUnusedResult: {
// @warn_unused_result with no arguments.
if (Tok.isNot(tok::l_paren)) {
auto attr = new (Context) WarnUnusedResultAttr(AtLoc, Loc, false);
Attributes.add(attr);
break;
}

// @warn_unused_result with arguments.
StringRef message;
StringRef mutableVariant;
SourceLoc lParenLoc = consumeToken();
bool invalid = false;
do {
// If we see a closing parenthesis,
if (Tok.is(tok::r_paren))
break;

if (Tok.isNot(tok::identifier)) {
diagnose(Tok, diag::attr_warn_unused_result_expected_name);
if (Tok.isNot(tok::r_paren))
skipUntil(tok::r_paren);
invalid = true;
break;
}

// Consume the identifier.
StringRef name = Tok.getText();
SourceLoc nameLoc = consumeToken();

// Figure out which parameter it is.
enum class KnownParameter {
Message,
MutableVariant
};

Optional<KnownParameter> known
= llvm::StringSwitch<Optional<KnownParameter>>(name)
.Case("message", KnownParameter::Message)
.Case("mutable_variant", KnownParameter::MutableVariant)
.Default(None);

// If we don't have the ':', try the deprecated '='. If that doesn't
// exist neither, complain.
if (findAttrValueDelimiter()) {
consumeToken();
} else {
if (known)
diagnose(Tok, diag::attr_warn_unused_result_expected_colon, name);
else
diagnose(Tok, diag::attr_warn_unused_result_unknown_parameter, name);
continue;
}

// If we don't have a string, complain.
if (Tok.isNot(tok::string_literal)) {
diagnose(Tok, diag::attr_warn_unused_result_expected_string, name);
if (Tok.isNot(tok::r_paren))
skipUntil(tok::r_paren);
invalid = true;
break;
}

// Dig out the string.
auto string = getStringLiteralIfNotInterpolated(*this, nameLoc, Tok,
name.str());
consumeToken(tok::string_literal);
if (!string) {
continue;
}

if (!known) {
diagnose(nameLoc, diag::attr_warn_unused_result_unknown_parameter,
name);
continue;
}

StringRef *whichParam;
switch (*known) {
case KnownParameter::Message:
whichParam = &message;
break;

case KnownParameter::MutableVariant:
whichParam = &mutableVariant;
break;
}

if (!whichParam->empty()) {
diagnose(nameLoc, diag::attr_warn_unused_result_duplicate_parameter,
name);
}

*whichParam = *string;
} while (consumeIf(tok::comma));

// Parse the closing ')'.
SourceLoc rParenLoc;
if (Tok.isNot(tok::r_paren)) {
parseMatchingToken(tok::r_paren, rParenLoc,
diag::attr_warn_unused_result_expected_rparen,
lParenLoc);
}
if (Tok.is(tok::r_paren)) {
rParenLoc = consumeToken();
}

Attributes.add(new (Context) WarnUnusedResultAttr(AtLoc, Loc, lParenLoc,
message, mutableVariant,
rParenLoc, false));
break;
}

case DAK_Swift3Migration: {
if (Tok.isNot(tok::l_paren)) {
diagnose(Loc, diag::attr_expected_lparen, AttrName,
Expand Down Expand Up @@ -1460,6 +1347,41 @@ bool Parser::parseDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc) {
.fixItReplace(Tok.getLoc(), "available");
}


if (DK == DAK_Count && Tok.getText() == "warn_unused_result") {
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 I'd rather leave the attribute kind in the list and then just not actually create one, rather than text-matching on the syntax of former attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping the attribute kind in the list causes it to have to show up in a lot of places in several files. It also still requires actually parsing it, validating the args, and keeping all of the associated information. I did that at first, and I found it to be much uglier.

Also I believe this is the first case after availability where an attribute was totally removed instead of just renaming it or moving it to a new position so it seemed to belong here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It requires having a switch case for it, but you can throw out everything else the same way you do here. I do see that it's annoying in the rest of the compiler; maybe we should have a new case in Attr.def for removed attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be okay with my adding that new "removed attribute" case in this PR? I'd rather do that than re-introduce those cases everywhere. Alternatively, if you would like to add the attribute, I'd be happy to use it.

If my proposal to remove @noescape is accepted, there will be at least one other attribute in a very similar situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'm fine with adding that attribute as part of this PR. It's not really much more of a change than removing all the existing cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this more, I'm not sure that having a new attribute macro for removed attributes is going to help much. It will still require having the actual token somewhere in code and someplace to insert a fixit (like this block does now); however, in addition, it will require checks in most of the places where I touched, some of which will still need to use the DAK_<AttrName>.

One of the nice parts of doing it this way is that I could search/grep for "WarnUnusedResult` and find all the places I needed to change. After the change, the only places with that string are this block and tests for this block.

Copy link
Contributor

Choose a reason for hiding this comment

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

All right. I guess every removed attribute shares the same cleanup code here: parse the base name, and optionally parse balanced parentheses.

// The behavior created by @warn_unused_result is now the default. Emit a
// Fix-It to remove.
SourceLoc attrLoc = consumeToken();

// @warn_unused_result with no arguments.
if (Tok.isNot(tok::l_paren)) {
diagnose(AtLoc, diag::attr_warn_unused_result_removed)
.fixItRemove(SourceRange(AtLoc, attrLoc));

return true;
}

// @warn_unused_result with arguments.
SourceLoc lParenLoc = consumeToken();
skipUntil(tok::r_paren);

// Parse the closing ')'.
SourceLoc rParenLoc;
if (Tok.isNot(tok::r_paren)) {
parseMatchingToken(tok::r_paren, rParenLoc,
diag::attr_warn_unused_result_expected_rparen,
lParenLoc);
}
if (Tok.is(tok::r_paren)) {
rParenLoc = consumeToken();
}

diagnose(AtLoc, diag::attr_warn_unused_result_removed)
.fixItRemove(SourceRange(AtLoc, rParenLoc));

return true;
}

if (DK != DAK_Count && !DeclAttribute::shouldBeRejectedByParser(DK))
return parseNewDeclAttribute(Attributes, AtLoc, DK);

Expand Down
39 changes: 0 additions & 39 deletions lib/Sema/TypeCheckAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ class AttributeEarlyChecker : public AttributeVisitor<AttributeEarlyChecker> {
IGNORED_ATTR(UIApplicationMain)
IGNORED_ATTR(UnsafeNoObjCTaggedPointer)
IGNORED_ATTR(Versioned)
IGNORED_ATTR(WarnUnusedResult)
IGNORED_ATTR(ShowInInterface)
IGNORED_ATTR(DiscardableResult)
#undef IGNORED_ATTR
Expand Down Expand Up @@ -692,7 +691,6 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
void visitPostfixAttr(PostfixAttr *attr) { checkOperatorAttribute(attr); }
void visitPrefixAttr(PrefixAttr *attr) { checkOperatorAttribute(attr); }

void visitWarnUnusedResultAttr(WarnUnusedResultAttr *attr);
void visitSpecializeAttr(SpecializeAttr *attr);
};
} // end anonymous namespace
Expand Down Expand Up @@ -1344,43 +1342,6 @@ AttributeChecker::visitSetterAccessibilityAttr(SetterAccessibilityAttr *attr) {
visitAbstractAccessibilityAttr(attr);
}

void AttributeChecker::visitWarnUnusedResultAttr(WarnUnusedResultAttr *attr) {
if (!attr->getMutableVariant().empty()) {
// mutable_variant only makes sense for non-mutating methods where
// it's possible to have a mutating variant.
auto func = dyn_cast<FuncDecl>(D);
if (!func) {
TC.diagnose(attr->getLocation(),
diag::attr_warn_unused_result_mutable_variable, 0);
return;
}

if (!func->getDeclContext()->isTypeContext()) {
TC.diagnose(attr->getLocation(),
diag::attr_warn_unused_result_mutable_variable, 1);
return;
}

if (!func->isInstanceMember()) {
TC.diagnose(attr->getLocation(),
diag::attr_warn_unused_result_mutable_variable, 2);
return;
}

if (func->getExtensionType()->getClassOrBoundGenericClass()) {
TC.diagnose(attr->getLocation(),
diag::attr_warn_unused_result_mutable_variable, 3);
return;
}

if (func->isMutating()) {
TC.diagnose(attr->getLocation(),
diag::attr_warn_unused_result_mutable_variable, 4);
return;
}
}
}

/// Check that the @_specialize type list has the correct number of entries.
/// Resolve each type in the list to a concrete type.
/// Create a Substitution list mapping each nested archetype to a concrete
Expand Down
1 change: 0 additions & 1 deletion lib/Sema/TypeCheckDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5376,7 +5376,6 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
UNINTERESTING_ATTR(SILStored)
UNINTERESTING_ATTR(Testable)

UNINTERESTING_ATTR(WarnUnusedResult)
UNINTERESTING_ATTR(WarnUnqualifiedAccess)
UNINTERESTING_ATTR(DiscardableResult)

Expand Down
Loading