Skip to content
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.

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

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

Closed
lattner mannequin opened this issue Mar 24, 2016 · 17 comments
Closed
Labels
compiler The Swift compiler in itself good first issue Good for newcomers standard library Area: Standard library umbrella task

Comments

@lattner
Copy link
Mannequin

lattner mannequin commented Mar 24, 2016

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
Copy link
Collaborator

Comment by Trent Nadeau (JIRA)

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

@lattner
Copy link
Mannequin Author

lattner mannequin commented Mar 24, 2016

I'd prefer separate PR's. Thanks!

@swift-ci
Copy link
Collaborator

Comment by Trent Nadeau (JIRA)

Created PR for part 1 (parsing): #1887

@lattner
Copy link
Mannequin Author

lattner mannequin commented Mar 31, 2016

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

@swift-ci
Copy link
Collaborator

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
Copy link
Mannequin Author

lattner mannequin commented Apr 3, 2016

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

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 3, 2016

Comment by Trent Nadeau (JIRA)

Thanks, Chris!

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 3, 2016

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
Copy link
Collaborator

swift-ci commented Apr 3, 2016

Comment by Trent Nadeau (JIRA)

Created PR for Part 2 (stdlib): #2044

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 8, 2016

Comment by Trent Nadeau (JIRA)

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

@swift-ci
Copy link
Collaborator

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
Copy link
Mannequin Author

lattner mannequin commented Apr 15, 2016

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

@swift-ci
Copy link
Collaborator

Comment by Trent Nadeau (JIRA)

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

@swift-ci
Copy link
Collaborator

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
Copy link
Collaborator

Comment by Trent Nadeau (JIRA)

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

@swift-ci
Copy link
Collaborator

Comment by Trent Nadeau (JIRA)

Created PR for Part 6 (remove @warn_unused_result): #2760

@swift-ci
Copy link
Collaborator

swift-ci commented Jun 8, 2016

Comment by Trent Nadeau (JIRA)

All PRs merged.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler The Swift compiler in itself good first issue Good for newcomers standard library Area: Standard library umbrella task
Projects
None yet
Development

No branches or pull requests

1 participant