SR-3359: Add a fix-it to remove @discardableResult on functions returning Void or Never

[matthewcarroll]

This commit adds a fix-it to remove @discardableResult on functions that return Void. The fix-it is at the warning level. A test was added to verify that the fix-it removes the @discardableResult. This issue was reported in SR-3359:
https://bugs.swift.org/browse/SR-3359

Changes:
TypeCheckDecl.cpp: Added a test to add a fix-it for an @discardableResult declared on functions that return Void.

DiagnosticsSema.def: Added a warning with a diagnostic message.

LoggingWrappers.swift.gyb, HashedCollections.swift.gyb: Removed @discardableResult on functions that return Void.

fixits-apply-all.swift, fixits-apply-all.swift.result: Added a test to verify that @discardableResult is removed from a function that returns Void.

[matthewcarroll]

@swift-ci Please smoke test

[xwu]

The spelling, here as elsewhere, should be @discardableResult (also, Void). Note also indentation and capitalization of this line (align quotation mark, lowercase f). I also wonder if the right message is "cannot be"--perhaps just "@discardableResult redundant for function returning Void"?

[xwu]

While you're at it, wouldn't it be wise to do the same for functions that return Never (or in fact, any uninhabited enum, since the core team has apparently decided that Never should work like any other uninhabited enum)? I believe it's simply a matter of adding a call to isUninhabited().

[jrose-apple]

This looks great, Matthew! I agree with Xiaodi's comments here both on diagnostic phrasing and on supporting Never as well. Mind making those changes?

[CodaFi]

I wonder if this would be more appropriate as part of the EarlyAttributeChecker visitor.

[matthewcarroll]

Thanks @jrose-apple!

Thanks for your comments @xwu. I'll make the changes you suggested.

[jrose-apple]

@CodaFi: Ah, IIRC EarlyAttributeChecker runs before things have even been type-checked, so we can't do it there.

[jrose-apple]

…but it might make sense in the regular AttributeChecker, so the point is valid anyway!

[matthewcarroll]

Jordan and Robert,

I left the test for @discardableResult on functions returning Void or Never in validateAttributes. It seems like a good place for it to me since @objc and @nonobjc declared on functions are handled there. But I'm happy to move it. Just let me know.

Thanks!
Matthew

[jrose-apple]

Oops?

[jrose-apple]

I think validateAttributes is for attributes that have to be resolved even when the declarations are only being used (perhaps from another file), rather than diagnosed only in the file that declared them. That makes AttributeChecker a better choice for this new diagnostic.

(I should beef up the doc comment to explain that.)

[matthewcarroll]

I moved the diagnostic into AttributeChecker::visitDiscardableResultAttr, and squashed the commits into d2f9768.

[jrose-apple]

Looks good to me! @xwu, any remaining comments?

@swift-ci Please smoke test

[xwu]

No other comments--looks good!

[jrose-apple]

Thanks, Matthew!