[SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning #27895

LucianoPAlmeida · 2019-10-26T16:48:25Z

Re-apply reverted PR with fixes.
Re-opening the PR fixing the issues we had.
Also, a point to continue the discussion started in #27881 and #27877
cc @xedin @hborla

LucianoPAlmeida · 2019-10-26T17:03:53Z

The significant changes are in 6a49ea3ac3c4b7277bcb9232e121930db0e756cf and 5816a59cb479a8802b6495e68aefd452f706e8cd. The rest is from the original PR :)

lib/Sema/CSFix.cpp

LucianoPAlmeida · 2019-11-04T18:51:36Z

cc @xedin
Pinging you just to know if this is a valid approach... or at least something we can tune up to a ideal solution. This was the most simple way I could make it work.
The significant changes are in all commits after 255ff8a2602a2f43fd5101d10f08057053e3f880
Can you take a look when you have some time? :)

LucianoPAlmeida · 2019-11-09T17:12:37Z

A couple of changes because of #28166 :)

xedin

Thank you for working on this, @LucianoPAlmeida! I have left some comments inline, sorry it took me a bit to take a look at this...

lib/Sema/ConstraintSystem.h

lib/Sema/CSBindings.cpp

lib/Sema/CSFix.cpp

LucianoPAlmeida · 2019-11-12T03:03:22Z

Thank you for working on this, @LucianoPAlmeida! I have left some comments inline, sorry it took me a bit to take a look at this...

No worries, Thank you for taking the time to give feedback on this :)
I'll resume the work here as soon as I can and ping you back when we have something 👍

LucianoPAlmeida · 2019-12-04T02:57:13Z

More detailed notes:

I struggled to get the source locator into the assignFixedType, and when I get it there, the way it all was implemented (passed as the locator of the bind constraint) impacted a diagnostic that relied on the locator of the expression to emit the correct diagnostic. I don’t know maybe it is something that I was doing incorrectly. This was in 9e85a180f1cf805b1114e54369a2f11d78467766, but I end up reverting … the way it is implemented now is working but, not sure if its a good approach.
I did remove the restriction for DeclRefExpr, so now the diagnostics are being emitted for almost any coercion.
The tests that broke after removing the restriction under swift/tests are fixed and passing … but I didn’t run validation yet.
A bunch of cases was added as tests on as_coerce.swift to handle the cases that were restricted before.
Since we had to revert last time and although I tried to be safe because applying some restrictions because this is a diagnostic where a false positive may not look good, there are any other tests that we can add to make sure we don’t break and find any regression on that?
And last but not least, @xedin thank you again for the reviews and for taking your time for help with this one :)

xedin · 2019-12-11T08:45:31Z

Sorry for the wait, @LucianoPAlmeida! This is in my queue for tomorrow.

LucianoPAlmeida · 2019-12-11T14:03:57Z

Sorry for the wait, @LucianoPAlmeida! This is in my queue for tomorrow.

No problem @xedin, I know you guys are very busy :))
And no need to rush, I'll probably not be able to address anything until the weekend, so no worries .
Also, I still thinking about the place where the recording of locators is being done, but couldn't think in something better and had issues trying to do in assignFixedType. Maybe you have some insight about this :)

xedin

Great progress! Changes follow the direction I had in mind the only additional thing we need is a new flag which would suppress warnings like that so tests are less noisy because some of them introduce redundant coercions intentionally.

xedin · 2019-12-11T20:56:34Z

lib/Sema/CSSimplify.cpp

+      // binding, meaning the fixed type can come from a constraint.
+      if (!hasRecordedTypeVariableBindingLocator(typeVar1))
+        recordTypeVariableBindingLocator(typeVar1,
+                                         getConstraintLocator(locator));


typeVar1 haven't been bound yet, why are we recording locator for it here? I think what you really trying to do here is has to be added to assignFixedType maybe...

Humm.. so we only record the binding on assignFixedType or just if we didn't record it already on TypeVariableBinding::attempt, I remember when I tried to record only all in assignFixedType I had problems because we don't have the source locator there. It works if we pass the source locator as the bind locator like here 9e85a18, but it ends up breaking other diagnostics that rely on the other locator... maybe I didn't do this correctly. I'll get back to this as soon as possible.

I think recording should only be done by assignFixedType because that's the point which is supposed to be binding type variables. What we need to is provide Bind constraint with correct "source" locator which fixed by #28752.

Thank you @xedin I'm getting back to this, may have something soon :)

I think we have two options here: a. Try to add some special handling to applySolutionWithFixes which would detect re-typecheck and skip any warnings generated by the solver (I think it should be possible to archive by checking for presence of SubExpressionDiagnostics flag in constraint system options); b. Postpone these changes until after CSDiag is gone;

Ahh @xedin, also ... I just check for it on attempt for this fix specifically, not in applySolutionFixes because I didn't know how it would impact the tests(testing now) and if it breaks something I didn't want to fix all in this PR, so I check specific... but I can do it in another PR and fix all there. What do you think?

@xedin Just finish rebasing/updating this with master :)

Are you trying to say that this is ready for review again? :) FYI, there is a refresh icon on the side of each reviewer which allows to re-request the review after the changes have been made.

Meanwhile, if you are looking for something to work on, there is FailureDiagnosis:: visitObjectLiteralExpr which is waiting to be ported :)

Are you trying to say that this is ready for review again? :) FYI, there is a refresh icon on the side of each reviewer which allows to re-request the review after the changes have been made.

Humm... I think the implementation itself is ready for review, but we stuck on the issue with the TupleElement locator path. Maybe it could be reviewed and have some points to improve, but since it takes time, let's wait until we have this issue solved and it is ready to go.

I just wanted to ping you, and meant that since I updated it with master, the only thing left to get this ready is to try to solve the problem and I wanted to know if maybe there is something I can help working on to fix it so we can move this forward :)

Meanwhile, if you are looking for something to work on, there is FailureDiagnosis:: visitObjectLiteralExpr which is waiting to be ported :)

Awesome, I can take it meanwhile :)) Thank you!

xedin · 2019-12-11T20:59:17Z

lib/Sema/ConstraintSystem.h

@@ -1269,6 +1269,9 @@ class ConstraintSystem {
  using FixedRequirement = std::tuple<TypeBase *, RequirementKind, TypeBase *>;
  SmallVector<FixedRequirement, 4> FixedRequirements;

+  /// Intended to track information about type variable source binding locator.
+  llvm::DenseMap<const TypeVariableType *, const ConstraintLocator *> TypeVariableBindings;


One more thing we need for this to work correct is to track how many bindings we have in each SolverScope and rollback once it's done just like we do for overloads and other things, take a look at SolverScope constructor/destructor located in CSSolver.cpp

Sure, I'll take a look 👍

lib/Sema/ConstraintSystem.h

CodaFi · 2019-12-16T04:42:29Z

This patch has grown to nearly 60 commits. Could you please squash this down?

LucianoPAlmeida · 2019-12-16T13:21:18Z

This patch has grown to nearly 60 commits. Could you please squash this down?

Sure @CodaFi :) I'll do it as soon as possible 👍

xedin · 2020-03-24T23:13:44Z

No problem!

…yCoercionFailure diagnostics

…ng RemoveUnnecessaryCoercion fix on match types

…arning

…tdlib

… source points to a tuple element.

…ssaryCoercion fix and adjusting some tests

…nFailure to use solution on diagnose()

…plicitTypeCoercion path

shahmishal · 2020-10-01T06:54:15Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch 2 times, most recently from 5816a59 to b73a305 Compare October 26, 2019 17:56

LucianoPAlmeida commented Oct 27, 2019

View reviewed changes

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

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch 2 times, most recently from 04e4346 to 1ecdd76 Compare November 2, 2019 17:46

LucianoPAlmeida requested a review from xedin November 3, 2019 00:09

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch from 36b1741 to f383d9c Compare November 4, 2019 00:30

LucianoPAlmeida mentioned this pull request Nov 10, 2019

[stdlib][Qol] SR-11295 Removing stdlib unnecessary coercions #27165

Merged

xedin requested changes Nov 11, 2019

View reviewed changes

lib/Sema/ConstraintSystem.h Outdated Show resolved Hide resolved

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

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

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

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch 4 times, most recently from 20db030 to 533af73 Compare November 29, 2019 01:05

LucianoPAlmeida changed the title ~~[SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning~~ [WIP][SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning Nov 29, 2019

LucianoPAlmeida changed the title ~~[WIP][SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning~~ [SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning Dec 4, 2019

xedin reviewed Dec 11, 2019

View reviewed changes

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch 4 times, most recently from 4e3cac0 to 16e8a40 Compare December 14, 2019 22:58

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch from 8340e75 to 16dee0d Compare December 16, 2019 23:02

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch from b77b477 to 161cce3 Compare April 26, 2020 01:12

LucianoPAlmeida added 24 commits August 18, 2020 20:44

[Diagnostics] Adding diagnostic message for redundant coercions

58be8eb

[ConstraintLocator] Creating ExplicitTypeCoercion constraint locator

9408302

[CSDiagnostics] Creating RemoveUnnecessaryCoercion Fix and Unnecessar…

08c886e

…yCoercionFailure diagnostics

[ConstraintSystem] Recording the type variables bindings and attempti…

428639f

…ng RemoveUnnecessaryCoercion fix on match types

[CSSolver] Clean up type variable bindings in end of solver scope

cd2d6ce

[Frontend] Creating compiler flag to disable the redundant coercion w…

9e0ff0a

…arning

[stdlib] Fixing remaining redundant coercions in stdlib

b0e2928

[tests] Adding disable flag to invocations under the tests/ClangImporter

f8d4bd4

[tests] Fixing tests under test/Constraints

aa9abfc

[tests] Fixing tests under test/Generics, test/Interpreter and test/s…

1f2e848

…tdlib

[tests] Fixing tests under test/Sema and test/decl

867e9bb

[tests] Fixing tests under test/type, test/expr and test/Parse

3740783

[CSFix] Getting the source locator from the representative

4e512a9

[test] Add explicit coercion test for tuple element expr/cast/as_coerce

3c778a4

[CSFix] Skipping the RemoveUnnecessaryCoercion if representative type…

3f39531

… source points to a tuple element.

[tests] Adding function overload test cases

60ab6db

[CSFix] Add check to dont diagnose if has SubExpressionDiagnostics flag

933075d

[Diagnostics] Improving logic and removing old checks on RemoveUnnece…

4fc26b7

…ssaryCoercion fix and adjusting some tests

[tests] Adding few more test cases for redundant coercion warning

49ef402

[CSFix] Remove CSDiag flag on RemoveUnnecessaryCoercion::attempt

d2d32a2

[Diagnostics] Adjust RemoveUnnecessaryCoercion and UnnecessaryCoercio…

f66405c

…nFailure to use solution on diagnose()

[ConstraintLocator] Adjust isCoerce() to check for LocatorPathElt::Ex…

2a85acb

…plicitTypeCoercion path

[Diagnostics] Adjust to get expr from type node anchor

3225f62

[Sema] Fixing compilation errors caused by removed APIs

b22b1b8

LucianoPAlmeida force-pushed the SR-11295-unnecessary-coercions branch from 161cce3 to b22b1b8 Compare August 19, 2020 12:58

shahmishal closed this Oct 5, 2020

LucianoPAlmeida mentioned this pull request Nov 15, 2020

[SR-11295] There should be a warning for unnecessary casts #53696

Open

[SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning #27895

[SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning #27895

Uh oh!

Conversation

LucianoPAlmeida commented Oct 26, 2019

Uh oh!

LucianoPAlmeida commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida commented Nov 4, 2019

Uh oh!

LucianoPAlmeida commented Nov 9, 2019

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida commented Nov 12, 2019

Uh oh!

LucianoPAlmeida commented Dec 4, 2019

Uh oh!

xedin commented Dec 11, 2019

Uh oh!

LucianoPAlmeida commented Dec 11, 2019

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

CodaFi commented Dec 16, 2019

Uh oh!

LucianoPAlmeida commented Dec 16, 2019

Uh oh!

xedin commented Mar 24, 2020

Uh oh!

shahmishal commented Oct 1, 2020

Uh oh!

Uh oh!

LucianoPAlmeida commented Oct 26, 2019 •

edited

Loading

LucianoPAlmeida Dec 12, 2019 •

edited

Loading