NFC for SR-12085 Clean up TypeCheckType so it never returns Type(). #29780

dfsweeney · 2020-02-12T04:09:29Z

Added ErrorType::get(Context) to TypeCheckType.cpp and clients.
Replaced assertions checking for null pointers with type->hasError() checks.
Note that getDynamicBridgedThroughObjCClass can still return null pointers so
the assert checks are still there for callers.

No functional change. This cleans up some instances of return Type() in TypeCheckType.cpp. There are a lot of other instances of return Type() but this gets the 15 of them in TypeCheckType.cpp plus a few others in direct clients to the functions in TypeCheckType.cpp that returned Type(). @CodaFi helped me with this.

This is my first one so I'm not sure I'm doing it right. Running all the validation tests gives me:

Expected Passes    : 6490
Expected Failures  : 5
Unsupported Tests  : 100
Unexpected Failures: 205

both before and after this change when I build with ninja. A lot of the validation test failures are in stdlib--I am not sure why.

The two validation test failures in Sema are:

Swift(macosx-x86_64) :: Sema/type_checker_perf/fast/rdar42672946.swift
Swift(macosx-x86_64) :: Sema/type_checker_perf/slow/rdar23327871.swift.gyb

I checked out swift-DEVELOPMENT-SNAPSHOT-2020-02-08-a and then put the commit directly on top of that. I hope that is OK--I tried a couple of things but that seemed to build clean.

Resolves SR-12085.

owenv · 2020-02-12T04:55:40Z

@dfsweeney Welcome to the project! Depending on your build setup, it's not unusual for many of the validation tests to fail locally trying to import StdlibCollectionUnitTest, which may be the case here. Those two Sema tests are both performance tests, so it's possible those are local-only failures too, but I'm less sure.

Let's try running tests on CI and see what happens.

owenv · 2020-02-12T04:55:49Z

@swift-ci please smoke test

dfsweeney · 2020-02-13T02:22:44Z

I found a bug I introduced that caused the smoke test to fail in 4 tests. Note I still have one failure in tests, in Parse/operators.swift. I'm working on that one. A lot of the validation tests are failing on import StdlibCollectionUnittest where the module is not found. I probably need to add something to the build-script to figure that out.

dfsweeney · 2020-02-14T03:11:29Z

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.

CodaFi · 2020-02-14T16:33:55Z

You can assert as a postcondition inside the APIs you convert then. Asserting in the clients is gonna be a game of whack-a-mole, as you’ve already discovered.

dfsweeney · 2020-02-15T15:16:07Z

I cleared the last warning in the tests.

@swift-ci please smoke test

dfsweeney · 2020-02-15T15:17:09Z

(I'm not sure if I can trigger a smoke test myself, if not can one of you please start it? Thanks!)

theblixguy · 2020-02-15T15:19:26Z

Only people with commit access can.

@swift-ci please test

dfsweeney · 2020-02-15T15:20:39Z

Thanks!

dfsweeney · 2020-02-17T14:47:47Z

I rebased these on top of the swift-5.2-branch locally. (There was one merge conflict that I had to clear up. it builds and all the tests pass.) Can I push that to the branch for this pull request or will things really get weird? I think it would change the base branch for the pull request and I don't know if that's OK or not in GitHub land. Any ideas?

CodaFi · 2020-02-17T19:13:04Z

I rebased these on top of the swift-5.2-branch locally.

You shouldn't rebase on top of a release branch. Master is where this patch is going to land, and if we need to cherry-pick it, we'll do it from there. 5.2 is missing a whole heckuva lot of commits.

lib/Sema/TypeCheckType.cpp

dfsweeney · 2020-02-17T20:00:25Z

Thanks for reviewing! I replied to each of the above, I'm pretty sure I understand but it helps to write it out. Sorry for the message generation. For the basing/rebasing, should I rebase this on to master and push it to the branch associated with the pull request? I was having some trouble getting the master branch to build.

I will work through the changes above.

CodaFi · 2020-02-18T17:02:32Z

For the basing/rebasing, should I rebase this on to master and push it to the branch associated with the pull request?

You shouldn't need to keep rebasing unless there's merge conflicts you have to resolve. But if you need to, pull and rebase onto master. I usually set a remote called upstream that points to Apple's copy of swift and run

$ git pull --rebase upstream master --autostash

every so often to keep in sync.

I was having some trouble getting the master branch to build.

Do you have any logs or errors to share?

dfsweeney · 2020-02-18T17:15:21Z

Thanks again! wrt the build problems, I was not using the swift/master branch of llvm, which made a lot of stuff flip out. At some point I just checked out the master branch (without swift/). Just one of those things getting used to a project, finding things you can break by accident. I am doing something like what you have above with an upstream branch. I am pushing through the rest of the above.

dfsweeney · 2020-02-18T23:02:35Z

ok that's not what I wanted to happen, I think I left something out of the git push.

CodaFi

My high-level comment remains the same: The asserts about structural properties need to come out. If they cannot be removed with confidence, perhaps we should break this PR down into smaller pieces and work from the leaf functions up incrementally.

dfsweeney · 2020-03-11T19:24:15Z

I cleared the structural assertions and pushed up but something looks weird about this--now it says that I want to merge 945 commits, which looks wrong. Anyway, can somebody fire off the checks at least? I will try to figure out what I did or did not do. If necessary I can close this PR and open a new one but then the history will be lost.

CodaFi · 2020-03-11T23:03:52Z

I believe you pulled without a rebase. Try

git pull --rebase upstream master

Where upstream is a remote entry pointing to apple/swift

CodaFi · 2020-03-30T22:42:07Z

There's some good cleanups in here. If we chunk this apart, I think we can merge them in this order:

The Parameter cleanup in TypeCheckType
The known stdlib decls bits

The rest require more infrastructure be converted first - I don't feel comfortable doing this completely piecemeal. We should work from the leaves (like the known stdlib decls parts) up.

dfsweeney · 2020-03-31T15:54:23Z

I agree with you, there's a lot of infrastructure going on. I will leave in the Parameter cleanup and the changes that don't depend on client behavior and take out the changes that rely too much on what the callers do, if that makes sense.

dfsweeney · 2020-04-02T17:38:09Z

I rebuilt this with git reset/git add -p and force-updated the branch. This should have only the straightforward changes and not the other stuff we've been talking about. @CodaFi please let me know if I left anything in there that you don't expect or vice versa. Thanks!

CodaFi · 2020-04-22T23:12:38Z

@swift-ci test

swift-ci · 2020-04-23T00:39:01Z

Build failed
Swift Test OS X Platform
Git Sha - 38c3cf350cc89d4de5e5685c441e574ded81d281

dfsweeney · 2020-04-23T03:14:15Z

This looks like an infrastructure failure although those are the most famous of last words. Can we restart the test or see what happened? Part of the build from Hudson is below: —Dan PASS: Swift(iphonesimulator-i386) :: Interpreter/dynamic_replacement_without_previous_calls.swift (11077 of 13130) PASS: Swift(iphonesimulator-i386) :: Interpreter/dynamic_cast_set_conditional_type_metadata.swift (11078 of 13130) FATAL: command execution failed java.io.EOFException at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2735) at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3210) at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:895) at java.io.ObjectInputStream.<init>(ObjectInputStream.java:357) at hudson.remoting.ObjectInputStreamEx.<init>(ObjectInputStreamEx.java:49) at hudson.remoting.Command.readFrom(Command.java:140) at hudson.remoting.Command.readFrom(Command.java:126) at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:36) at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:63) Caused: java.io.IOException: Unexpected termination of the channel at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:77) Caused: java.io.IOException: Backing channel 'macOS-60' is disconnected. at hudson.remoting.RemoteInvocationHandler.channelOrFail(RemoteInvocationHandler.java:214) at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:283) at com.sun.proxy.$Proxy73.isAlive(Unknown Source)

…

On Apr 22, 2020, at 6:39 PM, swift-ci ***@***.***> wrote: Build failed Swift Test OS X Platform <https://ci.swift.org/job/swift-PR-osx/20082/> Git Sha - 38c3cf3 <38c3cf3> — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#29780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAWZBCPCCLTM7T66MRZFM3RN6E3HANCNFSM4KTSIRWQ>.

CodaFi · 2020-04-23T17:22:01Z

Builder died

@swift-ci test macOS

varungandhi-apple · 2020-04-30T06:10:40Z

@CodaFi is this good to merge?

varungandhi-apple · 2020-05-01T01:19:57Z

Oof, there's a merge conflict now @dfsweeney 😬

dfsweeney · 2020-05-01T01:38:29Z

I’ll take a look at this one again. —Dan

…

On Apr 30, 2020, at 7:20 PM, Varun Gandhi ***@***.***> wrote: Oof, there's a merge conflict now @dfsweeney <https://github.com/dfsweeney> 😬 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAWZBDALVZJ4VZHHSM6MTDRPIPUVANCNFSM4KTSIRWQ>.

CodaFi · 2020-05-02T07:13:46Z

@dfsweeney Apologies for leaving this by the wayside - and for causing all of these merge conflicts. I believe I'm done messing around with type resolution for the time being. If you'd like, I can cherry-pick and shepherd this patch through myself and have you review the changes.

CodaFi · 2020-05-02T07:15:35Z

To get some context down: I'm in the process of converting the mutating usages of TypeLoc into either requests or just breaking apart the TypeLoc. I was hoping we could merge this patch as a capstone to that refactoring process, but there have been intervening issues in TypeCheckType that required my immediate attention lately.

dfsweeney · 2020-05-02T15:17:39Z

Hi @CodaFi it's all good, a lot of things are going on.

I just force-pushed again to clear the merge conflict. There are a couple of llvm_unreachables in TypeCheckType.cpp that I have been hoping I would think of a better solution for.

If you could cherry-pick it through that would be great! Let me know what you need me to do. Thanks!

CodaFi · 2020-05-07T01:17:26Z

@swift-ci test

Replaced "return Type();" in TypeChecker::getXType() methods in TypeCheckType.cpp with "return ErrorType.get(ctx);" along with appropriate diagnostics. Type() is equivalent to nullptr. Cleaned up clients to test for Type->hasError rather than nullptr.

dfsweeney · 2020-05-07T01:59:36Z

I fixed that one spot where we crossed wires on the ASTContext parameter from TypeChecker::validateType.

swift-ci · 2020-05-07T02:25:47Z

Build failed
Swift Test OS X Platform
Git Sha - 82c5d64cf1d5b57bf57c004739ed4d033999e7cc

theblixguy · 2020-05-07T06:05:12Z

@swift-ci please test

swift-ci · 2020-05-07T06:05:34Z

Build failed
Swift Test Linux Platform
Git Sha - 82c5d64cf1d5b57bf57c004739ed4d033999e7cc

CodaFi · 2020-06-11T20:07:49Z

Hey @dfsweeney, I finally cleared the blocker to doing this refactoring. I've taken this patch as the basis for a cleanup in #32321. I'm gonna close this out. Please feel free to review the linked PR if you have some time.

dfsweeney · 2020-06-11T20:40:39Z

Thanks! I'm glad this finally came in for a landing.

owenv requested a review from CodaFi February 12, 2020 04:56

This comment has been minimized.

Sign in to view

CodaFi reviewed Feb 17, 2020

View reviewed changes

lib/Sema/TypeCheckType.cpp Outdated Show resolved Hide resolved

CodaFi reviewed Feb 17, 2020

View reviewed changes

lib/Sema/TypeCheckType.cpp Outdated Show resolved Hide resolved

CodaFi reviewed Feb 17, 2020

View reviewed changes

lib/Sema/TypeCheckType.cpp Outdated Show resolved Hide resolved

CodaFi reviewed Feb 17, 2020

View reviewed changes

lib/Sema/TypeCheckType.cpp Outdated Show resolved Hide resolved

dfsweeney closed this Feb 18, 2020

dfsweeney deleted the sr12085_2 branch February 18, 2020 23:01

dfsweeney restored the sr12085_2 branch February 18, 2020 23:02

dfsweeney reopened this Feb 18, 2020

CodaFi requested changes Mar 9, 2020

View reviewed changes

dfsweeney force-pushed the sr12085_2 branch from 9b5e1d8 to cb05494 Compare March 12, 2020 02:52

dfsweeney force-pushed the sr12085_2 branch from b1473ed to 38c3cf3 Compare April 2, 2020 17:34

dfsweeney requested a review from CodaFi April 2, 2020 17:36

dfsweeney force-pushed the sr12085_2 branch from 38c3cf3 to 7bfafcd Compare May 1, 2020 01:46

dfsweeney force-pushed the sr12085_2 branch from 7bfafcd to 82c5d64 Compare May 2, 2020 15:11

swiftlang deleted a comment from swift-ci May 7, 2020

dfsweeney force-pushed the sr12085_2 branch from 82c5d64 to 2b7545d Compare May 7, 2020 01:58

CodaFi mentioned this pull request Jun 11, 2020

[NFC] TypeCheckType No Longer Returns Null #32321

Merged

CodaFi closed this Jun 11, 2020

swift-ci mentioned this pull request Mar 9, 2020

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

Closed

NFC for SR-12085 Clean up TypeCheckType so it never returns Type(). #29780

NFC for SR-12085 Clean up TypeCheckType so it never returns Type(). #29780

Uh oh!

Conversation

dfsweeney commented Feb 12, 2020

Uh oh!

owenv commented Feb 12, 2020

Uh oh!

owenv commented Feb 12, 2020

Uh oh!

dfsweeney commented Feb 13, 2020

Uh oh!

dfsweeney commented Feb 14, 2020

Uh oh!

CodaFi commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfsweeney commented Feb 15, 2020

Uh oh!

dfsweeney commented Feb 15, 2020

Uh oh!

theblixguy commented Feb 15, 2020

Uh oh!

dfsweeney commented Feb 15, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

dfsweeney commented Feb 17, 2020

Uh oh!

CodaFi commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfsweeney commented Feb 17, 2020

Uh oh!

CodaFi commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfsweeney commented Feb 18, 2020

Uh oh!

dfsweeney commented Feb 18, 2020

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

dfsweeney commented Mar 11, 2020

Uh oh!

CodaFi commented Mar 11, 2020

Uh oh!

CodaFi commented Mar 30, 2020

Uh oh!

dfsweeney commented Mar 31, 2020

Uh oh!

dfsweeney commented Apr 2, 2020

Uh oh!

CodaFi commented Apr 22, 2020

Uh oh!

swift-ci commented Apr 23, 2020

Uh oh!

dfsweeney commented Apr 23, 2020 via email

Uh oh!

CodaFi commented Apr 23, 2020

Uh oh!

varungandhi-apple commented Apr 30, 2020

Uh oh!

varungandhi-apple commented May 1, 2020

Uh oh!

dfsweeney commented May 1, 2020 via email

Uh oh!

CodaFi commented May 2, 2020

Uh oh!

CodaFi commented May 2, 2020

Uh oh!

dfsweeney commented May 2, 2020

Uh oh!

CodaFi commented May 7, 2020

Uh oh!

CodaFi commented Feb 14, 2020 •

edited

Loading

CodaFi commented Feb 17, 2020 •

edited

Loading

CodaFi commented Feb 18, 2020 •

edited

Loading