[Diagnostics] Type-check return of the multi-statement closure without applying solutions #6481

xedin · 2016-12-23T07:21:15Z

Obtain type of the result expression without applying solutions,
because otherwise this might result in leaking of type variables,
since we are not reseting result statement and if expression is
sucessfully type-checked it's type cleanup is going to be disabled
(we are allowing unresolved types), and as a side-effect it might
also be transformed e.g. OverloadedDeclRefExpr -> DeclRefExpr.

xedin · 2016-12-23T07:21:42Z

/cc @rudkx @DougGregor @slavapestov

Resolves 8 related crashers.

slavapestov · 2016-12-23T07:57:46Z

You're a beast!

slavapestov · 2016-12-23T07:58:00Z

@swift-ci Please smoke test

erg · 2016-12-23T07:59:22Z

lib/Sema/CSDiag.cpp

+      // Obtain type of the result expression without applying solutions,
+      // because otherwise this might result in leaking of type variables,
+      // since we are not reseting result statement and if expression is
+      // sucessfully type-checked it's type cleanup is going to be disabled


it's -> its

Sorry, i just can't spell... :(

…t apply solutions Obtain type of the result expression without applying solutions, because otherwise this might result in leaking of type variables, since we are not reseting result statement and if expression is sucessfully type-checked its type cleanup is going to be disabled (we are allowing unresolved types), and as a side-effect it might also be transformed e.g. OverloadedDeclRefExpr -> DeclRefExpr.

xedin · 2016-12-23T08:04:45Z

@slavapestov @erg Fixed typo, sorry about that...

slavapestov · 2016-12-23T08:07:23Z

I just let @practicalswift fix all my typos

xedin · 2016-12-23T08:13:40Z

@slavapestov That sounds great! :) Can you please re-trigger tests, since I had to push force?...

rudkx · 2016-12-23T08:19:55Z

@swift-ci Please smoke test

xedin · 2016-12-23T08:21:55Z

Thanks, @rudkx!

rudkx · 2016-12-23T08:24:11Z

@xedin I think this is a fine fix but a bit of a buzzkill since my (mostly but not totally complete) changes for keeping a separate side map for types in the constraint system would have fixed these! :)

xedin · 2016-12-23T08:25:07Z

@rudkx ah, I can close this and wait for your changes, that's totally fine

rudkx · 2016-12-23T08:26:08Z

@xedin Ha no, let's let this go through. My changes will potentially fix other cases and keep these from leaking in the future.

slavapestov · 2016-12-23T08:29:15Z

CI is down right now. I'll trigger tests tomorrow.

slavapestov · 2016-12-23T08:32:49Z

@rudkx Does your change make CSApple completely idempotent or does it still insert expressions for conversions? If the latter then not applying the solution unnecessarily when all you need is a type still makes sense IMO.

xedin · 2016-12-23T08:34:05Z

@rudkx Sounds good! And I actually didn't expect that there are going to be so many crashers fixed by this... :)

@slavapestov thanks!

xedin · 2016-12-23T08:38:43Z

@slavapestov I think that one of the biggest problems with diagnostics right now is that typeCheckChildIndendently and friends are mutating AST, it makes (at least) diagnosing of ApplyExpr way harder than it could be, because sometimes you need to have original expression as well as mutated expression like in case of contextual conversion diagnostics...

slavapestov · 2016-12-23T08:41:56Z

Yeah. Also another big issue is that lazy vars without a declared type have the initializer type checker twice, once to determine the type and another time when they're re-parented into the getter. I added a hack recently to fix one common crash but I still haven't figured out a better solution. Two ideas so far:

Emit trivial accessors in SILGen, just like we do with materializeForSet. This might be independently good, since it's simpler, less memory usage, etc. SILGen already knows how to emit a reference to a variable initializer.
Figure out a way to infer the type of a variable from an initializer without applying a solution to the initializer. Then we can special case lazy to do that since the solution is applied as part of type checking the getter.

I think we might want both for independent reasons. Thoughts?

rudkx · 2016-12-23T08:46:23Z

@slavapestov Yes, that's one of the reasons I still think we should take this (although it's not just CSApply that modifies expressions - that can happen for things that fail to type check as well).

slavapestov · 2016-12-23T08:49:10Z

But with the side table we should be able to get the type of something without mutating it right?

Also it's not clear to me if CSApply can fail even after the solver succeeds. I know of one case where we don't implement a tuple conversion, and a few random cases can fail when stdlib declarations are missing, but we should tighten this up as much as possible.

xedin · 2016-12-23T08:50:21Z

@slavapestov I think I know exactly what you are talking about (I mentioned that I the other issue):

protocol A {
   func foo()
}

struct B {
  lazy var a = A.foo
}

slavapestov · 2016-12-23T08:55:23Z

That's a nice reduction!

slavapestov · 2016-12-23T08:56:40Z

Well, A.foo is broken for other reasons too. It's missing SILGen support to generate the right thunk that opens the 'self' parameter when A is a protocol. Maybe a good project if you want to get into SILGen :-)

xedin · 2016-12-23T09:06:05Z

@slavapestov I can definitely take a look at that! Another question I have is should we be producing diagnostic in this case, which says something like - "cannot use result of assignment operation as a value"?

var x = 0
x = x = 42

Because that is completely broken for closure parameters e.g.

{ $0 = $0 = 0}

Because $0 are marked as equivalent in type system, and my old friend computeAssignDestType makes a binding Lvalue type variable it erases connections in the constraint system...

practicalswift · 2016-12-23T11:28:25Z

Good stuff! The number of unresolved crashers are quickly approaching zero.

Must scale up my fuzzing efforts to dig deeper! :-)

xedin · 2016-12-24T11:22:00Z

@slavapestov @rudkx Looks like there were 7 more tests which this change fixed, which were marked as 'REQUIRES: deterministic-behavior' (all related to return statement type-checking), I've marked all of them as fixed in separate commit.

…as fixed

practicalswift · 2016-12-24T14:42:30Z

@xedin That's great! The non-deterministic crashers are the worst!

rudkx · 2016-12-24T17:24:08Z

@xedin Yeah that's not too surprising. All of the ones I had looked at were leaking type variables.

slavapestov · 2016-12-24T21:50:03Z

Awesome!

slavapestov · 2016-12-24T22:19:27Z

Hey @xedin, two fun ones are 28594 and 28501. In both cases, we have an erroneously-placed _=nil expression, and we fail to produce a diagnostic, which triggers verifier asserts (we get a decl with an ErrorType and an UnresolvedDeclRefExpr, respectively). Since you're getting pretty good at CSDiag, do you want to take a look?

xedin · 2016-12-24T22:30:33Z

@slavapestov I will work on that next then, thanks!

… unresolved parameters If return expression uses closure parameters, which have/are type variables, such means that we won't be be able to type-check result correctly and, unfornutately, we are going to leak type variables from the parent constraint system through declaration types.

xedin · 2016-12-28T10:18:45Z

@slavapestov I've fixed two more crashers in context of this PR (pushed a separate commit) - 28449 and 28473, short story is - trying to type-check return expression which involves closure parameters is going to leak type variables from the parent system in some cases...

xedin · 2017-01-03T23:34:30Z

@slavapestov @rudkx Looks like CI is back in business, can you please trigger it?

slavapestov · 2017-01-03T23:40:44Z

You can't merge yet... let's wait for the more important fixes to go in before we deal with the crasher-fix-backlog :-)

xedin · 2017-01-03T23:42:53Z

@slavapestov Absolutely and sorry...

slavapestov · 2017-01-03T23:48:13Z

No worries! Just give us a couple of hours to clean up.

slavapestov · 2017-01-04T01:03:50Z

@swift-ci Please smoke test

xedin · 2017-01-04T01:45:43Z

@slavapestov I didn't report any test failures but build-script itself failed without any details on Linux :(

slavapestov · 2017-01-04T02:13:06Z

@swift-ci Please smoke test Linux

DougGregor · 2017-01-05T05:36:19Z

This is fantastic, thanks!

rudkx · 2017-01-05T06:19:52Z

@xedin I wonder if this might have fixed some of the crashers currently marked non-deterministic. E.g. I cannot seem to reproduce the crash in validation-test/compiler_crashers/28447-result-case-not-implemented-failed.swift.

Does anybody want to volunteer to try to re-enable all those tests and run through them N times (where N is high enough that we don't accidentally break CI) before submitting a PR to move the ones that appear fixed to compiler_crashers_fixed?

xedin · 2017-01-05T06:25:53Z

@rudkx I thought I got most of the ones marked as non-deterministic but apparently not :) I'll have another go through all of the remaining crashers, thanks!

xedin · 2017-01-09T02:22:08Z

@rudkx I've tried all three remaining tests marked as "deterministic-behavior" and they seem legit.

rudkx · 2017-01-09T06:29:31Z

@xedin You mean legitimately non-deterministic?

xedin · 2017-01-09T06:41:07Z

@rudkx Yes, I run them in the loop 50 times, sometimes they fail with escaped type var or invalid declaration, or segfault and sometimes not at all. The one you mentioned - 28447 - was marked resolved by @practicalswift in separate commit, that's why I didn't include it...

rudkx · 2017-01-09T06:43:07Z

@xedin Ah, thanks for the clarification!

xedin · 2017-01-09T06:44:58Z

@rudkx No problem! :)

erg reviewed Dec 23, 2016

View reviewed changes

xedin force-pushed the diagnose-closure-return branch from 0702cb1 to 7e9af10 Compare December 23, 2016 08:01

xedin force-pushed the diagnose-closure-return branch from 7e9af10 to f42fa36 Compare December 23, 2016 08:02

xedin force-pushed the diagnose-closure-return branch from 9e9cadd to 546a8f5 Compare December 24, 2016 11:34

[Diagnostics] Mark all closure result type-checking related crashers …

3a61fba

…as fixed

xedin force-pushed the diagnose-closure-return branch from 546a8f5 to 3a61fba Compare December 24, 2016 11:34

xedin mentioned this pull request Dec 29, 2016

Real compilers never crash #6484

Closed

slavapestov self-assigned this Jan 4, 2017

slavapestov merged commit 5da7e10 into swiftlang:master Jan 4, 2017

jtbandes mentioned this pull request Feb 1, 2017

[swiftc (37 vs. 5451)] Add crasher in swift::ASTVisitor #7163

Merged

[Diagnostics] Type-check return of the multi-statement closure without applying solutions #6481

[Diagnostics] Type-check return of the multi-statement closure without applying solutions #6481

Uh oh!

Conversation

xedin commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

erg Dec 23, 2016

Choose a reason for hiding this comment

Uh oh!

xedin Dec 23, 2016

Choose a reason for hiding this comment

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

rudkx commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

rudkx commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

rudkx commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

rudkx commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

slavapestov commented Dec 23, 2016

Uh oh!

xedin commented Dec 23, 2016

Uh oh!

practicalswift commented Dec 23, 2016

Uh oh!

xedin commented Dec 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Dec 24, 2016

Uh oh!

rudkx commented Dec 24, 2016

Uh oh!

slavapestov commented Dec 24, 2016

Uh oh!

slavapestov commented Dec 24, 2016

Uh oh!

xedin commented Dec 24, 2016

Uh oh!

xedin commented Dec 28, 2016

Uh oh!

xedin commented Jan 3, 2017

Uh oh!

slavapestov commented Jan 3, 2017

Uh oh!

xedin commented Jan 3, 2017

Uh oh!

xedin commented Dec 23, 2016 •

edited

Loading

xedin commented Dec 24, 2016 •

edited

Loading