[Diagnostics] Adding assignments directly to CS and diagnosing from there. #18950

gregomni · 2018-08-24T14:40:36Z

Always CSGen assignment statements by making a typeVar, binding the dest to @lvalue-typeVar and binding the source to convertible-to-typeVar, and then let the CS figure things out, rather than the previous implementation, which was trying to eagerly figure out types. This makes most assignment failures now TreatRValueAsLValue FixConstraints, which can be figured out via the new diagnostics system and in some cases produce better, more specific errors.

WIP:

Assignments via key path diagnoses are more specific, but are right now misleadingly so in some cases: need to add a specific non-writable keyPath diagnostic.
Recomputing assignment dest type in CSApply should be unnecessary now, need to rip that out.
Bunch of assignment-related code in old CSDiag is now dead, but not all of it, need to figure out how much I can rip out.

gregomni · 2018-08-24T14:44:26Z

@xedin This is the stuff I'm working on mentioned in my comment in #18948

Perhaps especially of interest, see previouslyDiagnosed added to CSFix here: in several cases LValue fixes happen because of some other fix in the assignment source or destination, and at least for this sort of fix, if a subexpr has already diagnosed something, further diagnosis up at the assignment expr level is unhelpful.

gregomni · 2018-08-24T14:44:38Z

@swift-ci Please smoke test.

gregomni · 2018-08-24T15:30:54Z

Squashed them merged. I had been working from a branch still passing solution to ConstraintFix diagnose() so lots of conflicts, but simple ones.

@swift-ci Please smoke test.

slavapestov · 2018-08-24T16:01:45Z

lib/Sema/CSGen.cpp

-
-      // Compute the type to which the source must be converted to allow
-      // assignment to the destination.
-      auto destTy = CS.computeAssignDestType(expr->getDest(), expr->getLoc());


What happens to computeAssignDestType()?

After I kill the other caller in CSApply, which shouldn't need it any more because any conversion restrictions should already be in the CS, then it gets deleted.

gregomni · 2018-08-24T17:20:12Z

@swift-ci Please smoke test.

xedin

Overall this makes sense to me, thank you!

But previousDiagnosed needs some more thought, I'd very much like to keep diagnostic interface free from flags like that. I think logic like that should live in constraint system - recordFixes should identify situations where paling up fixes is not ideal and not record them just like you mentioned.

gregomni · 2018-08-24T18:57:34Z

@xedin Well, I still want the fix to happen, because I want the solution to be reached rather than the system be unresolved. It's just the diagnosis that should be omitted. Another possible alternative for my use would be some sort of bool hasFixInExpr(Expr *expr) that walks the subexpr tree under a given expression to see if other fixes have been done. I could go in that direction instead.

xedin · 2018-08-24T19:08:06Z

Solver can go on even without recording a fix if it's redundant (like we do right now if the locators are the same), it's just a matter of what recordFixes returns.

xedin · 2018-08-24T19:09:26Z

I think it makes sense to either add that logic to recordFixes or applySolutionFixes as you mentioned.

gregomni · 2018-08-24T19:12:20Z

Ah, that's true. Can you think of a good name for a function like bool shouldRecordFixIfAlreadyExistingFixInSubExpr(FixKind kind) that would be callable from recordFixes?

xedin · 2018-08-24T19:14:52Z

Maybe something like bool supersedesExistingFixes(ConstraintFix *fix) and record only if it does?

gregomni · 2018-08-24T19:16:11Z

That's a much better name, but supersedes implies being able to discard the existing one(s). augmentsExistingFixes?

xedin · 2018-08-24T19:17:28Z

yeah, probably augments or complements or similar is better :) Sorry, I'm not great at languages...

xedin · 2018-08-24T20:57:12Z

Or maybe even as simple as bool isUsefulFix(ConstraintFix *) because how it determines usefulness is implementation detail...

More specific diagnoses for assigning to read-only keypaths. 'computeAssignDestType' is dead code now. ConstraintFix shouldRecordFix()

gregomni · 2018-08-25T03:39:49Z

@swift-ci Please smoke test

gregomni · 2018-08-25T03:48:43Z

Did the bits in "WIP" in the first comment above, and decided to go with shouldRecordFix() for the name of the FixConstraint method. Think this is good to go.

xedin

I think once we get the story straight with shouldRecordFix, it's indeed going to be good to go!

xedin · 2018-08-25T06:50:52Z

lib/Sema/CSFix.cpp

@@ -44,6 +44,18 @@ void ConstraintFix::print(llvm::raw_ostream &Out) const {

 void ConstraintFix::dump() const {print(llvm::errs()); }

+bool ConstraintFix::constraintSystemContainsFixInExpr(Expr *expr) const {


I think you can just fold this into new TreatRValueAsLValue::shouldRecordFix overload you've added, since it's only used there anyway, it could be extracted later if needed...

Actually on the second thought there is a kind attached to each fix, I think this could be moved to recordFix because it's kind of weird that fix itself is trying to determine if it should be recorded or not. I think it shouldn't even worry about it, that's what constraint system is for...

xedin · 2018-08-25T06:59:07Z

lib/Sema/CSDiag.cpp

@@ -689,6 +689,17 @@ resolveImmutableBase(Expr *expr, ConstraintSystem &CS) {
        return { expr, member };
    }

+    if (auto tupleExpr = dyn_cast<TupleExpr>(SE->getIndex())) {


Is there no better way to check this, @jckarter ?

Have SubscriptDecls been sufficiently parameter-list-ified that we can look at the argument labels and parameter list instead? cc @slavapestov

Yeah, but here we're looking at the arguments to the call, not the subscript declaration.

xedin · 2018-08-25T07:16:16Z

lib/Sema/CSGen.cpp

+        return TupleType::get(destTupleTypes, CS.getASTContext());
+      } else {
+        Type destTy = CS.createTypeVariable(CS.getConstraintLocator(expr));
+        CS.addConstraint(ConstraintKind::Bind, CS.getType(expr), LValueType::get(destTy),


Nit - I think this should be reversed - LValueType::get(destType) bind CS.getType(expr) because we'd rather always bind to something we are going to return (which also happens to be a type variable always).

xedin · 2018-08-25T07:16:54Z

lib/Sema/CSSimplify.cpp


-    if (type2->is<LValueType>() && !isTypeVarOrMember1) {
+  if (attemptFixes && type2->is<LValueType>() && !isTypeVarOrMember1) {


Nice catch!

…ind.

gregomni · 2018-08-25T18:42:20Z

@xedin Is this more like what you were thinking for recordFix?

xedin

Yes! I think this makes more sense.

xedin · 2018-08-25T19:28:39Z

lib/Sema/CSSimplify.cpp

@@ -4986,6 +4987,15 @@ ConstraintSystem::simplifyRestrictedConstraint(
  llvm_unreachable("Unhandled SolutionKind in switch.");
 }

+static bool isAugmentingFix(ConstraintFix *fix) {
+  switch (fix->getKind()) {
+    case swift::constraints::FixKind::TreatRValueAsLValue:


Does it really need to be qualified completely like that here?

Nope. I didn't notice the overly helpful autocomplete.

xedin · 2018-08-25T19:52:16Z

@swift-ci please test source compatibility

xedin · 2018-08-25T22:13:00Z

@swift-ci please test

xedin · 2018-08-25T22:13:21Z

If the tests are green, this should be good to go.

swift-ci · 2018-08-25T22:14:51Z

Build failed
Swift Test OS X Platform
Git Sha - 17f3c0d0e7d7be1bd9d8c34c902fb8cfaf48154c

swift-ci · 2018-08-25T22:16:29Z

Build failed
Swift Test Linux Platform
Git Sha - 17f3c0d0e7d7be1bd9d8c34c902fb8cfaf48154c

gregomni · 2018-08-26T16:12:28Z

@swift-ci Please smoke test.

slavapestov · 2018-08-28T00:35:19Z

lib/Sema/CSDiag.cpp

@@ -689,6 +689,17 @@ resolveImmutableBase(Expr *expr, ConstraintSystem &CS) {
        return { expr, member };
    }

+    if (auto tupleExpr = dyn_cast<TupleExpr>(SE->getIndex())) {
+      if (tupleExpr->getNumElements() == 1 && tupleExpr->getElementName(0).str() == "keyPath") {


I would at least compare Identifiers and not strings, so tupleExpr->getElementName(0) == ctx.getIdentifier("keyPath").

However I imagine this check ("is this a subscript that's really a keypath application") must appear elsewhere in the compiler. Can we extract it into a common method?

gregomni force-pushed the rvalue-as-lvalue branch from 17f3c0d to 35b5647 Compare August 24, 2018 15:29

slavapestov reviewed Aug 24, 2018

View reviewed changes

xedin reviewed Aug 24, 2018

View reviewed changes

Make assignments and assignment failure diagnoses directly in the CS.

821f63f

More specific diagnoses for assigning to read-only keypaths. 'computeAssignDestType' is dead code now. ConstraintFix shouldRecordFix()

gregomni force-pushed the rvalue-as-lvalue branch from 5730fbd to 821f63f Compare August 25, 2018 03:39

gregomni changed the title ~~WIP [Diagnostics] Adding assignments directly to CS and diagnosing from there.~~ [Diagnostics] Adding assignments directly to CS and diagnosing from there. Aug 25, 2018

xedin requested changes Aug 25, 2018

View reviewed changes

Move all shouldRecordFix logic back into CS itself keying off of FixK…

31ca8db

…ind.

xedin approved these changes Aug 25, 2018

View reviewed changes

Remove excess namespacing

6d5a69c

gregomni merged commit c95cfc6 into swiftlang:master Aug 26, 2018

slavapestov reviewed Aug 28, 2018

View reviewed changes

		@@ -44,6 +44,18 @@ void ConstraintFix::print(llvm::raw_ostream &Out) const {

		void ConstraintFix::dump() const {print(llvm::errs()); }

		bool ConstraintFix::constraintSystemContainsFixInExpr(Expr *expr) const {


		if (type2->is<LValueType>() && !isTypeVarOrMember1) {
		if (attemptFixes && type2->is<LValueType>() && !isTypeVarOrMember1) {

[Diagnostics] Adding assignments directly to CS and diagnosing from there. #18950

[Diagnostics] Adding assignments directly to CS and diagnosing from there. #18950

Uh oh!

Conversation

gregomni commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregomni commented Aug 24, 2018

Uh oh!

gregomni commented Aug 24, 2018

Uh oh!

gregomni commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregomni commented Aug 24, 2018

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

gregomni commented Aug 24, 2018

Uh oh!

xedin commented Aug 24, 2018

Uh oh!

xedin commented Aug 24, 2018

Uh oh!

gregomni commented Aug 24, 2018

Uh oh!

xedin commented Aug 24, 2018

Uh oh!

gregomni commented Aug 24, 2018

Uh oh!

xedin commented Aug 24, 2018

Uh oh!

xedin commented Aug 24, 2018

Uh oh!

gregomni commented Aug 25, 2018

Uh oh!

gregomni commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregomni commented Aug 25, 2018

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Aug 25, 2018

Uh oh!

xedin commented Aug 25, 2018

Uh oh!

xedin commented Aug 25, 2018

Uh oh!

swift-ci commented Aug 25, 2018

Uh oh!

swift-ci commented Aug 25, 2018

gregomni commented Aug 24, 2018 •

edited

Loading

gregomni commented Aug 25, 2018 •

edited

Loading