Fix crash when considering invalid binary operator function for resolution <rdar://23719809&23720006>

[jtbandes]

This fixes compiler crashes resulting from an invalid FuncDecl which is later considered while resolving a binary application. <rdar://23719809&23720006>

Examples:

• func ~=(){} (empty tuple)
• func ~=(_:Int){} (one argument which cannot be cast to a tuple)

These are considered when type-checking something like switch 0 { case 0: ... }.

Some questions I had while poking around to figure out this patch:

• I've already submitted test cases at practicalswift/swift-compiler-crashes#98, so presumably, they will eventually be pulled into validation-test. Should I still add a test in test/decl/operators.swift, or perhaps somewhere under test/Sema?
• Should DeclChecker::bindFuncDeclToOperator have called FD->setInvalid(true) when it emits error diagnostics for the function definitions, which happens earlier, to avoid getting this far into operator resolution with a bad operator function? I'm not familiar enough with the code to understand who's expected to call setInvalid, and who's expected to check isInvalid.
• As of f00e5bc, FuncDecl::isBinaryOperator() returns true if the argument tuple has 1 element with an ellipsis. I'm concerned that favorMatchingBinaryOperators (the function being modified here) doesn't support this case properly. Should it?

[gribozavr]

Please add a test in the same commit that makes the fix.

[jtbandes]

Where would you like the test to be added? I don't see any particularly relevant files in the Sema tests. I could add one though.

[gribozavr]

validation-test/compiler_crashers_2_fixed/ would be an OK place.

[jtbandes]

Test case added. Still I recognize that this might not be the right way of fixing the bug. If there's a better approach, let me know!

[gribozavr]

@cwillmor Would you mind reviewing this patch?

[cwillmor]

LGTM. I'm currently seeing if it fixes any other compiler crashers in the suite. Thanks for looking into this!

[jtbandes]

My pleasure 😃

Before it's merged, however, it would make sense for someone who really knows this code to look at the questions I posed in the original PR comment — I'm mildly worried that this patch might be sweeping other potential problems under the rug.

[cwillmor]

The RUN line here should be // RUN: not %target-swift-frontend %s -parse (note the not). Adding "not" asserts that the parse will fail but not crash.

[jtbandes]

OK, updated. (Is there a way to run individual tests using the build-script?)

[cwillmor]

To answer your questions:

• This issue has more to do with what happens downstream of an improper declaration of =~ at the switch statement, so you could consider adding something to test/stmt/statements.swift.
• Calling setInvalid() on an operator implementation with the wrong number of args is probably correct. Try it and see? Worst that happens is that we have to update some diagnostics in the test suite.
• If we allow single-variadic-arg funcs to implement binary operators, we don't do it very well. Probably worth filing a bug! The following code currently fails to compile:

infix operator *** {}
func ***(x: Int...) {}
func f(x: Int) { x *** x }

EDIT: re the last point, @DougGregor and I just spoke and we don't think this should be allowed; it should raise a diagnostic at func ***(x: Int...) {}.

[jtbandes]

Calling setInvalid() didn't break anything when I tried it (though I didn't run the full test suite), but it's not clear that it's any better than what's currently being done. While diagnostics are emitted, It still seems that invalid declarations are allowed to flow through the rest of the constraint system (rather than failing fast), which I'm sure is a design choice. It appears the downstream client is responsible for checking isInvalid(), yet I don't see as many checks as I might expect.

I'm sure there is more to understand here; as an outsider I'm just not that familiar with the codebase yet. I'll defer to y'all to merge this patch if you see fit, or recommend more changes. 🎱

[cwillmor]

This is good to merge — the RUN line was the only real issue. Feel free to follow up on the other comments. Thanks!

[jtbandes]

🎉 !

[lattner]

Nice!