If it is unused then check if is a valid return type
If it is a valid return type then proceed with fix-it
Anything that should be different? Also, I have seen that the existing diagnostic missing return last expression is emitted in SIL, but typeChecking occurs in Sema, how should I approach this?
I filed the bug in response to reports from users that they were not getting helpful error messages when they forgot returns. Because Nate's pass runs as a SIL diagnostic, it would have the shortcoming that it won't show the error or fixit if there are any other type errors in the file, which is likely when working incrementally on a file. I think that an AST-level check would probably be more robust and give a better experience.
Sorry about not posting this earlier, I think you're approach makes sense. In fact its what is currently implemented in the SIL pass. Since Joe expressed interest in moving to an AST-level check, I think ripping out the logic from SIL is okay. Some things to think about: in the bug there is some interest in unused last expressions in nested brace statements (like if statements) and potentially being able to diagnose those as well. This would require walking those brace statements as well (which shouldn't be too bad...), but (to me) it might make sense to check a broad variety of cases like:
func add(_ x: Int?, _ y: Int?) -> Int? {
guard let x = x, let y = y else {
// Should we diagnose that this needs a return statement?
nil // error: nil requires a contextual type
}
x + y // error: missing return ...
}
There's a lot of different cases that could benefit from this diagnostic, but maybe we only want to restrict to a few of the more common cases.
This may be out of scope for this specific starter bug, but I wonder if we are really working around the true issue at hand: we exit too soon from type checking errors causing SIL diagnostics to have phase ordering problems.
As a simple straw man example, what if instead of just bailing in Sema when we fail to type check an AST, what if instead we had a visitor walk the AST and mark nodes that b/c of type errors in the program we can not SILGen/run diagnostics on safely. Then SILGen would just skip those bad AST nodes and SILGen parts of the program that were valid (and of course emit SIL diagnostics for that valid code).
The other interesting thing is consider if we could defer certain types of Sema checks (or sema error emission) to the SIL level. As another straw man example, what if when one encountered some sort of sema error (perhaps in the body of a function we are type checking), rather than emitting the error there, we SILGen some representation of that error (perhaps using undef or the like). Then we could emit other diagnostics and have a SIL pass that emits the actual error later. One nice thing about this approach is that if the error was not reachable, we could avoid emitting the error creating a SFINAE sort of ability to conditionally compile code.
It's true that there's no fundamental reason we shouldn't be able to still run SILGen on diagnostics on the well-typed declarations within a module even if there are type errors. I still think that an AST-level check would probably be a better fit for this diagnostic, since the condition it's looking for is syntactic—you have a multi-statement block with an ignored expr as the last statement that's of the same type as the function return type.
