[DiagnosticsQoI] SR-6272 Tailored diagnostics with fixits for numerical conversions by chuganzy · Pull Request #13272 · swiftlang/swift

chuganzy · 2017-12-05T09:05:41Z

If there's an explicit numeric conversion that could make the system type-check, suggest "fix it".

Resolves https://bugs.swift.org/browse/SR-6272

CodaFi · 2017-12-16T04:38:31Z

So sorry this got left by the wayside

CodaFi · 2017-12-16T04:39:56Z

test/Constraints/diagnostics.swift

@@ -4,7 +4,7 @@ protocol P {
  associatedtype SomeType


All the little whitespace changes in this file should be reverted.

CodaFi · 2017-12-16T04:45:48Z

test/Constraints/diagnostics.swift

+  enum Foo: Int {
+    case bar
+  }
+  // expected-error@+2 {{binary operator '*' cannot be applied to operands of type 'Int' and 'Float'}} {{22-28=}} {{29-30=}}


This technically changed a whole lot more tests than just this one. I'm not sure if it's worth it to go through and update them all, but it would be nice to pull some non-integral/floating types into this test just to make sure we don't accidentally try to, say, cast a string to a Float or something.

Makes sense! Will do!

CodaFi

It's a great first start. Comments inline.

This reverts commit b2059c6.

chuganzy · 2017-12-16T06:14:02Z

@CodaFi Addressed your comments. Thanks!

CodaFi · 2017-12-16T06:15:09Z

test/Constraints/diagnostics.swift

+  }
+  // expected-error@+2 {{binary operator '*' cannot be applied to operands of type 'Int' and 'Float'}} {{22-28=}} {{29-30=}}
+  // expected-note@+1 {{overloads for '*' exist with these partially matching parameter lists: (Float, Float), (Int, Int)}}
+  Foo.bar.rawValue * Float(0)


How does this fare with a contextual type?

let x: Float = Foo.bar.rawValue * Float(0)

Good point..
It suggests to fix Float(0) to 0 so far..
Will fix it.

tryIntegerCastFixIts treats the toType as a contextual type, always. You may want to try calling it on the LHS and RHS expressions of the binary operation and pass along their respective types and the contextual type. tryIntegerCastFixIts tells you if it succeeds in diagnosing part of the expression or not.

Addressed with 72cf158.

CodaFi · 2017-12-16T07:17:58Z

@swift-ci please smoke test

xedin · 2017-12-17T02:43:49Z

lib/Sema/CSDiag.cpp

      .highlight(rhsExpr->getSourceRange());
+
+      auto contextualType = CS.getContextualType();
+      if (contextualType && contextualType->isEqual(rhsType)) {


Thanks for the patch, @chuganzy! Unfortunately it's not always correct to use contextual type this way because it applies to the whole expression, while the problem could be local to sub-expression that might have some other type different from (or equal to) contextual type e.g.

struct A: ExpressibleByIntegerLiteral { typealias IntegerLiteralType = Int init(integerLiteral: Int) {} static func +(lhs: A, rhs: Int) -> Float { return 42.0 } } let x: Float = 1.0 let _: Float = A(integerLiteral: 42) + x let _: Float = A(integerLiteral: 42) + 42.0 let _: Float = A(integerLiteral: 42) + x + 1.0

Most of the cases here are going to get suggestion to wrap x into A but there is no overload like that, or in case of first example is going to suggest wrapping A into Float which also might be incorrect because it's going to call a different overload all together.

Even without contextual type, I think we shouldn't try to suggest fix-its like that without taking into account more information e.g. what overloads are available for sub-expression and if suggestion is going to result in different overload choice or not.

Hmm, makes sense.

Addressed!
c80b569
5c37ccf

xedin · 2017-12-18T20:32:54Z

lib/Sema/CSDiag.cpp

      .highlight(rhsExpr->getSourceRange());
+
+      for (auto &candidate : calleeInfo.candidates) {
+        auto tupleType = dyn_cast<TupleType>(candidate.getArgumentType().getPointer());


It would be better to use:

if (auto *fnType = candidate.getUncurriedFunctionType()) { auto params = fnType->getParams(); }

here since we are trying to move away from using parameter types directly.

The problem though is that even though we could diagnose one of the candidates with cast fix-it it doesn't necessarily mean that we should do that... E.g. if I had +(A, Int) -> Float and +(A, Double) -> Float and I have A() + x where x: Float expression like in my previous example, the question is which fix-it should I suggest, Int or Double? IMHO none, because it's ambiguous what the user wanted to do here...

So what is supposed to do in following case?

let x: Int = 0 let y: Float = 0 x * y

We have 2 candidates to suggest "fix-it" - x * Int(y) or Float(x) * y.

You think we should not suggest in this case?

Yes, that's right, I don't think it makes sense to suggest fix-it in this case because it's unclear what is intended.

Addressed 0df987a

xedin · 2018-01-11T19:08:23Z

@chuganzy I haven't forgotten about this, will take a look soon!

xedin · 2018-01-11T23:08:52Z

@swift-ci please smoke test

xedin · 2018-01-12T09:06:37Z

Thank you, @chuganzy!

SR-6272: Show “Fix it” for numerical conversions

a9db3f2

chuganzy changed the title ~~SR-6272 Tailored diagnostics with fixits for numerical conversions~~ [Compiler] SR-6272 Tailored diagnostics with fixits for numerical conversions Dec 5, 2017

chuganzy changed the title ~~[Compiler] SR-6272 Tailored diagnostics with fixits for numerical conversions~~ [DiagnosticsQoI] SR-6272 Tailored diagnostics with fixits for numerical conversions Dec 6, 2017

SR-6272: Add tests

b2059c6

chuganzy force-pushed the SR-6272 branch from 8c6fb72 to b2059c6 Compare December 6, 2017 06:33

CodaFi reviewed Dec 16, 2017

View reviewed changes

CodaFi requested changes Dec 16, 2017

View reviewed changes

chuganzy added 2 commits December 16, 2017 14:39

Revert "SR-6272: Add tests"

06b9f78

This reverts commit b2059c6.

SR-6272: Add more tests

5a56d4d

CodaFi reviewed Dec 16, 2017

View reviewed changes

SR-6272: Use contextual type if available

72cf158

CodaFi approved these changes Dec 16, 2017

View reviewed changes

rudkx requested a review from xedin December 16, 2017 10:47

xedin requested changes Dec 17, 2017

View reviewed changes

chuganzy force-pushed the SR-6272 branch from da436f4 to 72cf158 Compare December 17, 2017 11:53

chuganzy added 2 commits December 17, 2017 22:06

SR-6272 Use calleeInfo instead

c80b569

SR-6272 Skip if both lhs and rhs are different

5c37ccf

xedin reviewed Dec 18, 2017

View reviewed changes

chuganzy force-pushed the SR-6272 branch from d80501f to d522e39 Compare December 19, 2017 03:56

SR-6272 Add fix-it if there is only one candidate

0df987a

chuganzy force-pushed the SR-6272 branch from d522e39 to 0df987a Compare January 3, 2018 21:27

xedin approved these changes Jan 12, 2018

View reviewed changes

xedin merged commit ce847cc into swiftlang:master Jan 12, 2018

chuganzy deleted the SR-6272 branch January 12, 2018 19:10

swift-ci mentioned this pull request Jan 13, 2018

[SR-6272] Tailored diagnostics with fixits for numerical conversions #48822

Closed

Conversation

chuganzy commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuganzy Dec 16, 2017 • 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!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

chuganzy commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuganzy Dec 16, 2017 • 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!

CodaFi Dec 16, 2017 • 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!

CodaFi commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuganzy Dec 17, 2017 • 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!

chuganzy Dec 17, 2017 • 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!

chuganzy Dec 18, 2017 • 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!

xedin commented Jan 11, 2018

Uh oh!

xedin commented Jan 11, 2018

Uh oh!

xedin commented Jan 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chuganzy commented Dec 5, 2017 •

edited

Loading

chuganzy Dec 16, 2017 •

edited

Loading

chuganzy Dec 16, 2017 •

edited

Loading

CodaFi Dec 16, 2017 •

edited

Loading

chuganzy Dec 17, 2017 •

edited

Loading

chuganzy Dec 17, 2017 •

edited

Loading

chuganzy Dec 18, 2017 •

edited

Loading