[SR-12085] Clean up TypeCheckType so it never returns Type()

[CodaFi]

Previous ID	SR-12085
Radar	rdar://58879303
Original Reporter	@CodaFi
Type	Improvement
Status	Resolved
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Improvement, StarterBug, TypeChecker
Assignee	dfsweeney (JIRA)
Priority	Medium

md5: 3ff0e16b54d9cecb5d8f2423279ab61e

Issue Description:

Returning the null Type() is a legacy behavior that used to drive the now-dead-and-gone IterativeTypeChecker. That we return the null Type at all just serves to cause crashes in downstream components. All of these paths should diagnose and return ErrorType instead.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

I just ran through this–it looks like there are 15 return Type() s in lib/Sema/TypeCheckType.cpp. I replaced them with return ErrorType::get(ctx) with whatever the context is at the time. That's pretty straightforward.

I am learning so I want to make sure this is right.

Three questions:

1. there are some callers of the 15 functions with changes that have some guards on them like if (!type) {handle error} and then often just return the type returned. After the change they'll propagate ErrorType up to their callers. That's correct right? We should be able to remove the guards now.

2. Also there are a couple of upstream calls that also return Type() like if (!type) return Type(). Those should be fixed up as well right? TypeCheckRequestFunctions.cpp has a couple of these that I can patch up.

3. I don't see direct unit tests for TypeCheckType. I was thinking about how to add tests for this–either figure out how to exercise this with the AST tests, or add a new test module to test the type checker, or leave it alone and just fix the code. Which is right?

I can send a pull request with just the changes to TypeCheckType.cpp it feels like it needs some more testing. Thanks!

[CodaFi]

1. It depends on the callsite. A canonical example of a callsite that should never see a null type is TypeResolver::resolveOpaqueReturnType. There, you can strip the guard out.

2. Any clients of TypeCheckType should be corrected not to expect the null type, yes. You should add assertions if possible.

3. There aren't direct unit tests. TypeCheckType is more of a service that gets consumed by the rest of the semantic machinery, and it's pretty hard to test in isolation. The validation test suite includes a number of compiler crashers that exercise this path that we use instead.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Thanks! I will work through the client calls I can find.

For the validation test suite, I can run the validation test suite with lit.py right? Or should I run it with build-script?

[CodaFi]

You don't need build-script to test stuff. I wrote up a little workflow advice on the swift forums that should help you out here:

https://forums.swift.org/t/need-a-workflow-advice/12536/14?u=codafi

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Thanks again! I will work through it. I'm trying to make sure I actually exercise as much as I can. I will reread your advice in the link (I read it once before, I realize now that I read it again) and work through it.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Another question–for the compiler crashers, will something like:

./llvm-project/llvm/utils/lit/lit.py ./build/Xcode-ReleaseAssert/swift-macosx-x86_64/validation-test-macosx-x86_64/

validate using the swiftc compiler in

./build/Xcode-ReleaseAssert/swift-macosx-x86_64/Release/bin? or do I need to add some more configuration to make sure?

I'm still learning the build system and feel sort of insecure now that I have more than one compiler around.

[CodaFi]

tl;dr It'll automatically use the just-built compiler.

The long answer is that precisely which compiler is used is controlled by the expansions in the RUN lines. You'll see stuff like `%target-typecheck-verify-swift` or `%target-swift-frontend` etc, which will automatically expand to use the appropriate compiler for the given target and build configuration. The Lit.cfg files in the build directory and in the main repository can give you more information on what those aliases mean exactly.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Thanks. When I run

 ./llvm-project/llvm/utils/lit/lit.py ./build/Xcode-DebugAssert/swift-macosx-x86_64/validation-test-macosx-x86_64       


lit.py: /Users/dfs/swift-source/build/Xcode-DebugAssert/swift-macosx-x86_64/validation-test-macosx-x86_64/lit.site.cfg:95: note: Adding to path: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using swift: /usr/bin/swift
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using swiftc: /usr/bin/swiftc
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'sil-opt' program, try setting SIL_OPT in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'sil-func-extractor' program, try setting SIL_FUNC_EXTRACTOR in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'sil-llvm-gen' program, try setting SIL_LLVM_GEN in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'sil-nm' program, try setting SIL_NM in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'sil-passpipeline-dumper' program, try setting SIL_PASSPIPELINE_DUMPER in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'lldb-moduleimport-test' program, try setting LLDB_MODULEIMPORT_TEST in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-ide-test' program, try setting SWIFT_IDE_TEST in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-syntax-test' program, try setting SWIFT_SYNTAX_TEST in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-reflection-dump' program, try setting SWIFT_REFLECTION_DUMP in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-remoteast-test' program, try setting SWIFT_REMOTEAST_TEST in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-indent' program, try setting SWIFT_INDENT in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-symbolgraph-extract' program, try setting SWIFT_SYMBOLGRAPH_EXTRACT in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using clang: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/clang
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using llvm-link: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/llvm-link
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-llvm-opt' program, try setting SWIFT_LLVM_OPT in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using llvm-profdata: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/llvm-profdata
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using llvm-cov: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/llvm-cov
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using llvm-strings: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/llvm-strings
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using FileCheck: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/FileCheck
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using llvm-dwarfdump: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/llvm-dwarfdump
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:247: note: using llvm-dis: /Users/dfs/swift-source/build/Xcode-DebugAssert/llvm-macosx-x86_64/Debug/bin/llvm-dis
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'sourcekitd-test' program, try setting SOURCEKITD_TEST in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'complete-test' program, try setting COMPLETE_TEST in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-api-digester' program, try setting SWIFT_API_DIGESTER in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-refactor' program, try setting SWIFT_REFACTOR in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-demangle-yamldump' program, try setting SWIFT_DEMANGLE_YAMLDUMP in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'Benchmark_O' program, try setting BENCHMARK_O in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'Benchmark_Driver' program, try setting BENCHMARK_DRIVER in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:318: note: Using resource dir: /usr/lib/swift
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:350: note: Compiling with -swift-version 4
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:373: note: Using Clang module cache: /Users/dfs/swift-source/build/Xcode-DebugAssert/swift-macosx-x86_64/$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:374: note: Using test results dir: /Users/dfs/swift-source/build/Xcode-DebugAssert/swift-macosx-x86_64/$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)/swift-test-results/x86_64-apple-macosx10.9
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:379: note: Using code completion cache: /Users/dfs/swift-source/build/Xcode-DebugAssert/swift-macosx-x86_64/$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)/swift-test-results/x86_64-apple-macosx10.9/completion-cache
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:891: note: Testing OS X x86_64-apple-macosx10.9
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:947: note: Running tests on Mac OS X version 10.15.3 (19D76)
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:1548: note: Using platform module dir: /usr/lib/swift/macosx
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:1573: note: Testing with the just-built libraries at /usr/lib/swift/macosx
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:250: warning: couldn't find 'swift-reflection-test-macosx-x86_64' program, try setting SWIFT_REFLECTION_TEST_MACOSX_X86_64 in your environment
lit.py: /Users/dfs/swift-source/swift/test/lit.cfg:1816: note: Available features: CMAKE_GENERATOR=Xcode, CODEGENERATOR=AArch64, CODEGENERATOR=ARM, CODEGENERATOR=Mips, CODEGENERATOR=PowerPC, CODEGENERATOR=SystemZ, CODEGENERATOR=X86, CPU=x86_64, OS=macosx, PTRSIZE=64, SWIFT_VERSION=4, VENDOR=apple, asserts, crash-recovery, executable_test, foundation, libdispatch, no_asan, no_lto, nonexecutable_test, objc_interop, sftp_server, shell, sourcekit, swift-remoteast-test, swift_interpreter, swift_repl, swift_stable_abi, swift_stdlib_asserts, swift_test_mode_optimize_none, swift_test_mode_optimize_none_x86_64, tools-debuginfo
-- Testing: 6800 tests, 8 threads --
 

I get

 Expected Passes    : 5813
  Expected Failures  : 5
  Unsupported Tests  : 275
  Unexpected Failures: 707
 
21 warning(s) in tests.

Did I miss a step somewhere? I built the Xcode ALL_BUILD scheme beforehand in Xcode for cmark, llvm. and swift.

Otherwise I have a commit that I can make a pull request for, but I think I also need to do the CI part first right? Following https://github.com/apple/swift/blob/master/docs/ContinuousIntegration.md ?

[swift-ci]

Comment by Daniel Sweeney (JIRA)

never mind the above, if I build with ninja the tests run OK.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Cleared up one of the errors in Parse/operators.swift. Still getting

 /Users/dfs/swift-source/swift/test/Parse/operators.swift:125:5: error: unexpected error produced: broken standard library: cannot find intrinsic operations on Optional<T> foo??bar // expected-error{{broken standard library}} expected-error{{consecutive statements}} {{6-6=;}}

The pattern of returning Type() and then checking the result for null is pretty common. I thought I got all the clients of the original 15 in TypeCheckType but they propagate up and down the call chain. In general replacing

if (!type) 

with

if (!type || type->hasError()) 

keeps the logic in the same flow, but it's pretty easy to miss that and then let the execution run into code that does other things. Sometimes you get a diagnostic that triggers a test failure, but I'm not sure there's 100% coverage.

If I can figure out where I missed a check that's causing the one remaining test failure the patch would run clean, but I'm a little worried that this might cause more problems at a distance.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

@CodaFi I lost some momentum on this but want to get back to it. The pull request is still open at #29780 with an open question/comment at #29780 (comment) . The semantics of returning ErrorType (in place of nullptr Type()) to indicate a branch-not-to-take feel a little off.

[CodaFi]

Resolved by #32321

Thanks for your hard work and patience dfsweeney (JIRA User). We got there in the end!