[CSGen] Turn invalid decls into holes #34584
Conversation
Upon attempt to generate constraints for invalid declaration reference let's turn that into a hole and record this new fix to diagnose it once solution has been reached.
If AST node is a reference to an invalid declaration let's diagnose it as such unless some errors have been emitted (invalid declaration itself should have a diagnostic describing a reason why it's invalid).
Instead of failing constraint generation upon encountering an invalid declaration, let's turn that declaration into a potential hole and keep going. Doing so enables the solver to reach a solution and diagnose any other issue with expression.
|
@nathawes I believe that this is going to help code completion, maybe we have a few test-cases we could add to showcase that? |
|
@swift-ci please test |
| // If no errors have been emitted yet, let's emit one | ||
| // about reference to an invalid declaration. | ||
|
|
||
| emitDiagnostic(diag::reference_to_invalid_decl, decl->getName()); |
There was a problem hiding this comment.
When would the solver actually emit this error?
There was a problem hiding this comment.
In cases when actual problem with declaration hasn't been diagnosed due to a bug (hopefully never).
There was a problem hiding this comment.
I was thinking about adding bit about asking to report it just like we do with actual fallback diagnostic.
| @@ -1227,9 +1227,11 @@ namespace { | |||
| if (knownType) { | |||
There was a problem hiding this comment.
Just above this, within the CS.getVarType(VD) call it's getting the type of VD and replacing any ErrorTypes it contains with holes when CS.isForCodeCompletion is true. Does this change make that logic redundant?
There was a problem hiding this comment.
I think for code completion we'd want to preserve as much structure as possible so replacing everything with a single hole might not be the best way... I was thinking more of a second case where E->getDecl()->isInvalid() is true.
There was a problem hiding this comment.
If it is, could we remove it and make the below behave the same way, preserving any wrapping types? (e.g. [ErrorType] becomes [<hole>] rather than just <hole>?
There was a problem hiding this comment.
Didn't see your reply before I sent that, sorry.
There was a problem hiding this comment.
I guess I could try to unify logic here to preserve some more structure for diagnostics, I'll give it a try!
There was a problem hiding this comment.
I looked into this some more and it looks like for solver and diagnostics in particular it make more sense to keep invalid declarations represented as type variables so they could assume any time from context instead otherwise we'd risk diagnosing issues which are effectively already diagnosed while validating declarations...
Instead of failing constraint generation upon encountering
an invalid declaration, let's turn that declaration into a
potential hole and keep going. Doing so enables the solver
to reach a solution and diagnose any other issue with
expression.
Resolves: rdar://70880670