[SR-1052] Implement SE-0047 - Defaulting non-Void functions so they warn on unused results

[lattner]

Previous ID	SR-1052
Radar	None
Original Reporter	@lattner
Type	Task
Status	Closed
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Compiler, Standard Library
Labels	Task, StarterBug
Assignee	tanadeau (JIRA)
Priority	Medium

md5: 77cf91f85074b4a3b8d2ded18cca58e5

relates to:

• SR-1681 spurious "unused result" warning in Swift 3

Issue Description:

SE-0047 has been accepted:
https://github.com/apple/swift-evolution/blob/master/proposals/0047-nonvoid-warn.md

I suggest implementing this as a series of independent patches:

1) Introduce the @discardableResult declaration attribute.
2) Change the standard library to use it on various mutating methods that return values.
3) Change the clang importer to auto-attach the attribute to imported declarations (ones that are not marked with the clang "attribute((warn_unused_result))").
4) Parse and recognize the new doc comments (mutableVariant, etc.). See comment from Chris Lattner below.
5) Change the compiler to start listening to the @discardableResult attribute.
6) Change the @warn_unused_result attribute to be recognized, and produce an error + fixit to remove it.

[swift-ci]

Comment by Trent Nadeau (JIRA)

By "independent patches", do you mean separate commits or completely separate PRs?

[lattner]

I'd prefer separate PR's. Thanks!

[swift-ci]

Comment by Trent Nadeau (JIRA)

Created PR for part 1 (parsing): #1887

[lattner]

Another todo I forgot: we need to parse and recognize the doc comment extensions (MutableVariant🙂 etc.

[swift-ci]

Comment by Trent Nadeau (JIRA)

Chris, can my part 1 PR get reviewed and merged? I'd like to do more work on this issue this weekend, but this first step is blocking the rest.

[lattner]

Weird, for some reason, I didn't get the email about this. I'll take a look right now!

[swift-ci]

Comment by Trent Nadeau (JIRA)

Thanks, Chris!

[swift-ci]

Comment by Trent Nadeau (JIRA)

Per Dmitri Gribenko, it may be useful to support adding the new attribute on subscripts and properties. It may not be necessary on properties though as it appears to be checked via other means and is an error (see GitHub comment).

Per Chris Lattner, allowing the new attribute on new declaration types should only be added if there's demand.

[swift-ci]

Comment by Trent Nadeau (JIRA)

Created PR for Part 2 (stdlib): #2044

[swift-ci]

Comment by Trent Nadeau (JIRA)

Created PR for Part 3 (clang importer): #2103

[swift-ci]

Comment by Trent Nadeau (JIRA)

SE-0047 mentions new mutatingVariant and nonmutatingVariant doc keywords. However, that camel-case style seems to be inconsistent with the other two-word doc keyword (recommendedover).

Should I add it as in the proposal or to be consistent with the existing doc keyword naming?

[lattner]

My understanding is that these doc comments are case insensitive. The stdlib folks prefer camel case for them.

[swift-ci]

Comment by Trent Nadeau (JIRA)

Created PR for Part 4 (doc comment parsing): #2204

[swift-ci]

Comment by Trent Nadeau (JIRA)

I'm starting to look at Part 5 (having the compiler use the new attr). What I have so far appears to be working with the exception of signatures like this:

public mutating func withMutableCharacters<R>(_ body: (inout CharacterView) -> R) -> R

where the return type is generic.

For regular functions, I can skip the warning if the result type in the decl is Void.

Any ideas on how to tell if a call to a generic function like withMutableCharacters has a return type of Void in context?

[swift-ci]

Comment by Trent Nadeau (JIRA)

Created PR for Part 5 (use @discardableResult): #2459