[SR-1553] [QOI] Warn on discarded binding of () result

[ddunbar]

Previous ID	SR-1553
Radar	None
Original Reporter	@ddunbar
Type	Improvement

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Improvement, DiagnosticsQoI
Assignee	zsalloum (JIRA)
Priority	Medium

md5: a541dcf4281d920a0880df5d3eabc2fa

Issue Description:

Swift should warn on the code below:

$ cat x.swift
func f0() -> () {}
let _ = f0()

$ swiftc -c x.swift
$

This is somewhat related to SE-0047, in that if you use let _ = ... to discard results, and then later refactor the code to not return a result, Swift won't help you find the other places in the code base to update.

[ddunbar]

Any opinion here @lattner?

[belkadan]

Note that there are two ways to discard results:

let _ = foo()
_ = foo()

The second is considered canonical. (The first is more useful when destructuring tuples.)

[lattner]

I don't understand the rationale for this. Why would we warn about this, and what message would make sense? This doesn't appear to indicate a likely bug (though admittedly, I can't imagine a reason someone would want to use this).

A corner case you should consider is a generic function that can devolve into returning void in some cases.

-Chris

[ddunbar]

The rationale was that due to SE-0047 I was tempted to quickly slap {{_ = }} on a bunch of things that I thought ultimately probably should be factored into different functions. However, then it occurred to me that once those functions were refactored I probably wouldn't have any easy time of finding all the places where that had been done.

[lattner]

Ok, I can see a warning along the lines of "ignoring the result of a void-producing function is pointless" with a fixit to remove the "_ =".

[swift-ci]

Comment by Zeyad Salloum (JIRA)

I would love to start contributing to swift with this task. i've got all the source code building on my machine. can you point me to where i can do the fix? Thanks

[CodaFi]

Hello zsalloum (JIRA User). Here's how I would approach this:

Because the diagnostic should be emitted when we know the types of everything in the AST, CSApply seems the appropriate place to include it. If you look specifically in ExprRewriter::visitAssignExpr there you should be able to do something roughly like this to emit the diagnostic right after we compute the destination type:

if (!SuppressDiagnostics && isa<DiscardAssignmentExpr>(destExpr)) {
  if (auto tupleTy = destTy->getAs<TupleType>()) {
    if (tupleTy->getNumElements() == 0) {
      cs.getTypeChecker()
        .diagnose(destExpr->getLoc(), diag::discard_expr_discards_void_result);
      // NOTE GOES HERE
    }
  }
}

Take a look around DiagnosticsSema.def for a place to stick the diagnostic text and then write a NOTE (also see DiagnosticsSema.def for how that would work) that corresponds to the removal of the '_ =' and use the DiagnosticEngine::fixItRemove* family of methods to chop it out.

If you have any questions you can ping me here or on Twitter (@CodaFi_) or Github

[rudkx]

The inner two if here can just be replaced with:

if (destTy->isVoid()) { ...

I don't think CSApply is a good place for this. I know we already emit some diagnostics there, but that's really not ideal as that it should really only be applying the results of type checking.

We already have code to deal with bad usage of DiscardAssignmentExpr in diagSyntacticUseRestrictions, and that seems like a much better place for this diagnostic.

[swift-ci]

Comment by Zeyad Salloum (JIRA)

@rudkx thanks for the comment. true that definitely looks like a better place.

[swift-ci]

Comment by Zeyad Salloum (JIRA)

can someone point me to a fast way to work/debug. i know build-toolchain should create a swift toolchain that i can then make Xcode work with so i can test. however Xcode doesn't see the toolchain after the script is done. is there something i should be doing differently?

[belkadan]

Sure. Most of us use build-script as described in README.md, then running tests (and writing tests) as described in docs/Testing.rst.

[belkadan]

If you do want to use build-toolchain, you need to install the result into /Library/Developer/Toolchains or ~/Library/Developer/Toolchains. I'm not sure if there's any additional work you'd need to do beyond that.

[LucianoPAlmeida]

Auditing a some old type checker issue to see if they are valid. This looks to me still relevant and a good first issue. So I'm marking as Starter Bug.