Skip to content

[ConstraintSystem] (mostly) Finish up solver type cache implementation #13664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 14, 2018
Merged

[ConstraintSystem] (mostly) Finish up solver type cache implementation #13664

merged 2 commits into from
Feb 14, 2018

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jan 1, 2018

Based on all of the great work @rudkx has done, solver should only apply final types
to the expression tree when the solution is deduced. This patch makes it so and fixes
tests (mostly in IDE code completion) along the way.

@xedin xedin requested a review from rudkx January 1, 2018 20:29
@xedin
Copy link
Contributor Author

xedin commented Jan 1, 2018

@rudkx There is one thing left which is going to fix 2 crashers related to explicit casts - we still use getCastTypeLoc().setType in CSGen to set opened generic type.

@rudkx
Copy link
Contributor

rudkx commented Jan 2, 2018 •
edited
Loading

@xedin I didn't do any work on TypeLoc types yet. That seemed small enough to make it a separate task.

I haven't looked at this for a while, but I believe you also need to clear the type on the Expr when it is cached to ensure that we aren't accidentally reading it in places. That was the real hold up the last time I looked at this.

Can you hold off trying to merge this until I get done with the IUO changes? I suspect I've introduced some new ->getType() and such that will need to be cleaned up with these changes in place, and I don't want to slow that work down at all.

@xedin
Copy link
Contributor Author

xedin commented Jan 2, 2018

@rudkx Sure, there is no rush in merging this, just wanted to get it out since tests are fixed. I has planning to take care of getCastTypeLoc before merging anyway.

@xedin xedin force-pushed the finalize-expr-type-cache branch from 3cecd97 to 5d10771 Compare January 7, 2018 05:45
@xedin
Copy link
Contributor Author

xedin commented Jan 7, 2018

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Jan 7, 2018

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Feb 12, 2018

/cc @nathawes @benlangmuir

@xedin
Copy link
Contributor Author

xedin commented Feb 13, 2018

/cc @rudkx

rudkx
rudkx requested changes Feb 13, 2018
View reviewed changes
Copy link
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a better understanding of some of the changes and why we see behavioral changes here before we merge anything for this.

lib/Sema/CSDiag.cpp

auto type = loc.getType();
if (!type)
type = cs.getType(loc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this directly calling loc.getType() and then only if getting null calling cs.getType(loc)?

test/Compatibility/tuple_arguments.swift Outdated
var a = 3
var b = 4
var c = (3)
var d = (a, b)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there is something wrong if this is resulting in this type of diagnostic change.

test/IDE/complete_constructor.swift
@@ -119,7 +119,6 @@ func testExplicitConstructors3P() {
// EXPLICIT_CONSTRUCTORS_3P: Begin completions
// EXPLICIT_CONSTRUCTORS_3P-DAG: Decl[Constructor]/CurrNominal: ['(']{#(a): Int#}[')'][#ExplicitConstructors3#]{{; name=.+$}}
// EXPLICIT_CONSTRUCTORS_3P-DAG: Decl[Constructor]/CurrNominal: ['(']{#a: Int#}, {#b: Float#}[')'][#ExplicitConstructors3#]{{; name=.+$}}
// EXPLICIT_CONSTRUCTORS_3P-DAG: Decl[FreeFunction]/CurrModule/NotRecommended/TypeRelation[Invalid]: freeFunc()[#Void#]{{; name=.+$}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am wondering how the code completion changes (which I haven't looked at closely) are related to the type cache. I would expect no behavioral changes from making the type checker store types in a different place.

lib/Sema/CSDiag.cpp
@@ -2153,6 +2153,8 @@ static void eraseOpenedExistentials(Expr *&expr, ConstraintSystem &CS) {
return type;
});
CS.setType(expr, type);
// Set new type to the expression directly.
expr->setType(type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's non-obvious why this is the right thing to do. This seems like an indication that something that should be reading from the type cache isn't.

lib/Sema/CSApply.cpp
@@ -7481,6 +7481,7 @@ namespace {
} else {
// For other closures, type-check the body once we've finished with
// the expression.
cs.setExprTypes(closure);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances do we need to do this? We should be type checking these bodies independently. They can refer to captured data, but that data should already have a type, right?

rudkx
rudkx approved these changes Feb 13, 2018
View reviewed changes
Copy link
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a chance to sync up on this and everything looks okay for now. It definitely moves things forward re: fixing some crashers and no longer writing types all over the place. We still need to go back and clear expression types, etc. to finish this up.

@xedin xedin changed the title [WIP] [ConstraintSystem] Finish up solver type cache implementation [ConstraintSystem] (mostly) Finish up solver type cache implementation Feb 13, 2018
xedin added 2 commits February 13, 2018 00:08
@xedin
[ConstraintSystem] Don't apply types to expressions until solution is…
c0f5711 
… found

Resolves: rdar://problem/36744895
@xedin
[ConstraintSystem] Add TypeLoc caching to constraint system
8c17b92 
This is useful for explicit casts and type expressions, where
type loc and expression types might be different, and allows
constraint solver to avoid setting opened types to expressions
which resolves multiple crashes.
@xedin xedin force-pushed the finalize-expr-type-cache branch from 5d10771 to 8c17b92 Compare February 13, 2018 08:09
@xedin
Copy link
Contributor Author

xedin commented Feb 13, 2018

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Feb 13, 2018

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Feb 13, 2018

@rudkx looks like everything is green, going to merge a bit later today

@xedin
Copy link
Contributor Author

xedin commented Feb 14, 2018

@swift-ci please smoke test

@xedin xedin merged commit 79d7064 into swiftlang:master Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rudkx rudkx rudkx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xedin @rudkx