Sema: Don't construct naked ErrorTypes in valid code #8642
Conversation
| @@ -1345,8 +1345,10 @@ namespace { | |||
| // We may, alternatively, want to use a type variable in that case, | |||
| // and possibly infer the type of the variable that way. | |||
| CS.getTypeChecker().validateDecl(E->getDecl()); | |||
| if (E->getDecl()->isInvalid()) | |||
| if (E->getDecl()->isInvalid()) { | |||
| CS.setType(E, E->getDecl()->getInterfaceType()); | |||
There was a problem hiding this comment.
The fixme notes spawning and solving a type variable here would potentially provide better diagnostics instead of no diagnostics.
There was a problem hiding this comment.
If I understand the code correctly, this will only happen if the variable's declaration is erroneous, and the idea is that we might be able to guess the type here so we can add it to the diagnosis there. But that seems really difficult to accomplish—by this point, the error at the declaration site should already have been diagnosed. Even if we did decide to do it, I think that's probably better done as a separate patch; I'm just trying to maintain the status quo in this PR.
| @@ -1751,19 +1752,19 @@ void ConstraintSystem::shrink(Expr *expr) { | |||
| if (auto array = dyn_cast<ArraySliceType>(base)) { | |||
| auto elementType = array->getBaseType(); | |||
| // If base type is invalid let's return error type. | |||
There was a problem hiding this comment.
Should this be asserted as an invariant @slavapestov?
There was a problem hiding this comment.
Why would it be an invariant? An array slice type can end up with an error type inside of it
|
This looks good, but I want @rudkx to take a look at the Expr::setType() calls and advise which ones need to use the typemap instead. |
|
@swift-ci Please smoke test |
|
@breantdax Do you want to rebase and we can merge this in? |
It is useful to set a breakpoint on ErrorType::get(ASTContext) to see what is going wrong immediately instead of after the fact when an ErrorType pops up where you don't expect it. Unfortunately associated type inference, domain shrinking and the type cleanup RAII utility would all build ErrorTypes even with valid code. Refactor things a bit so that this is no longer the case; at least now the standard library and overlays build with 'assert(false)' inserted into ErrorType::get(ASTContext). ErrorType::get(Type) is still expected to come up in associated type inference since it is used as a signal while sorting through potential type witness candidates.
Fixes crash in test/ClangImporter/objc_bridging_generics.swift.
As well as anywhere else we `coerceToMaterializable`. Fixes: * test/IDE/complete_crashes.swift * test/IDE/complete_enum_elements.swift * test/IDE/complete_pattern.swift * test/IDE/complete_unresolved_members.swift * test/IDE/complete_value_expr.swift * test/Parse/implicit_getter_incomplete.swift * test/Parse/invalid.swift * test/Parse/switch.swift * test/SourceKit/Indexing/index_enum_case.swift
This prevents one invalid declaration from causing cascades of subsequent errors. Fixes: * test/NameBinding/scope_map_lookup.swift * test/decl/var/variables.swift
When the initialization expression on a variable declaration fails, give it an ErrorType so Swift does not redundantly diagnose its un-inferrable type. Fixes: * test/NameBinding/stdlib.swift * test/decl/typealias/protocol.swift Would also have redundantly fixed the same tests as 2e875e7e, but the targeted solution there is cleaner.
A malformed initializer or increment expression would come out of TypeChecker::typeCheckExpression() with no type, which `checkIgnoredExpr()` was not expecting. Fixes: * test/Parse/foreach.swift * test/Parse/recovery.swift * test/Parse/toplevel_library_invalid.swift * test/IDE/complete_stmt_controlling_expr.swift * validation-test/compiler_crashers_fixed/01736-void.swift * validation-test/compiler_crashers_fixed/00587-swift-pattern-foreachvariable.swift
|
@slavapestov Rebased. Test failures on my machine are the same as on master. |
|
@swift-ci Please smoke test and merge |
This is an update of #8341 to fix the tests it broke. To quote @slavapestov there:
The initial patch tripped several assertions in places where AST nodes which were previously assigned
ErrorTypewere nownull. In some places, I propagated already-constructedErrorTypes which were the underlying cause of the problem; in others, I assignedErrorTypes later; in still others, I simply made the code toleratenulltypes. I've also rebased it on top of a more recent commit in master to make it easier to merge.This all seems to work on macOS, but I'm not set up to test on Linux.