Add new error diagnostic for escaping optional closures #30746

ismetanin · 2020-04-01T06:01:03Z

This PR will add a new specialized error diagnostic when @escaping attribute is applied to an optional closure

Resolves SR-4637

include/swift/AST/DiagnosticsSema.def

test/attr/attr_escaping.swift

lib/Sema/TypeCheckType.cpp

include/swift/AST/DiagnosticsSema.def

AnthonyLatsis · 2020-04-02T09:42:51Z

lib/Sema/TypeCheckType.cpp

@@ -2428,7 +2428,7 @@ Type TypeResolver::resolveAttributedType(TypeAttributes &attrs,
        // a function type, we'll remove it.
        if (i == TAK_escaping) {
          diag.fixItRemove(getTypeAttrRangeWithAt(Context,
-                                                  attrs.getLoc(TAK_escaping)));
+                                                attrs.getLoc(TAK_escaping)));


Oops, not this one though, line 2421. Thanks!

I'd suggest running clang-format on your changes @ismetanin

Thanks for your suggestion, now everything should be ok (or at least clang-formatted)

It seems like you've only run it on the last commit.

Looks like you're right. I've run clang-format over all my changes now
Sorry for these formatting mistakes, now I know how to properly format code

No worries, thanks for being patient!

Maybe would be good if those indentation changes, could be squashed in a single commit?

@xedin can squash them upon merging

No problems, I've squashed indentation changes with the second commit

…f original message when object type is a function type

xedin · 2020-04-02T19:13:04Z

lib/Sema/TypeCheckType.cpp

-          diag.flush();
-          diagnoseInvalid(repr, repr->getLoc(),
-                          diag::escaping_optional_type_argument);
+      if (i == TAK_escaping && ty->getOptionalObjectType()) {


Unfortunately checking for ty->getOptionalObjectType() is not enough, to produce escaping_optional_type_argument we have to make sure that object type is indeed a function type (AnyFunctionType to be precise). I think it makes sense to try and unify this if/else into a single if for i == TAK_escaping and have some additional logic to pick which message to produce inside it because both cases share logic to emit a fix-it.

So, I tried but couldn't unify if/else into a single if for i == TAK_escaping 😕
We can't reassign InFlightDiagnostic so I couldn't reuse one fixItRemove

You could define a lambda for the correct InFlightDiagnostic outside the loop and call it instead of diagnoseInvalid. As a bonus, we get to adjust the flow to only check against an optional when i == TAK_escaping.

I didn't catch the idea with lambda 😢

If we declare lambda outside it will anyway produce InFlightDiagnostic that we can't reassign so we'll still have to use double-check i == TAK_escaping and double-use fixItRemove
Am I wrong?

The lambda emits the correct diagnostic and returns an InFlightDiagnostic you can then attach stuff to; no need to reassign anything:

const auto diagnoseInvalidAttr = [&](TypeAttrKind kind) { if (kind == TAK_escaping) { Type optionalObjectType = ty->getOptionalObjectType(); if (optionalObjectType && optionalObjectType->is<AnyFunctionType>()) return diagnoseInvalid(...); } return diagnoseInvalid(...); };

Agree with @AnthonyLatsis, You can even pass the attr so you don't have to define the lambda inside the loop :)

const auto diagnoseInvalidAttr = [&](Type attributeType, TypeAttrKind attrKind) { if (attributeType && attributeType->is<AnyFunctionType>()) { return diagnoseInvalid(repr, attrs.getLoc(attrKind), ...); } else { return diagnoseInvalid(...); } }; for (auto i : FunctionAttrs) { if (!attrs.has(i)) continue; Type optionalObjectType = ty->getOptionalObjectType(); if (i == TAK_escaping) { diagnoseInvalidAttr(optionalObjectType, i) .fixItRemove( getTypeAttrRangeWithAt(Context, attrs.getLoc(TAK_escaping))); } attrs.clearAttribute(i); }

Oh my gosh, thanks a lot, now it looks way better

@LucianoPAlmeida I'm not sure that it will work when i != TAK_escaping, will it?

Ahh, sure ... this was just a sketch real quick to show a possible approach :)
I think you right, it has to work for other function attrs the only applies to function types e.g. @autoclosure

xedin · 2020-04-02T19:14:10Z

test/attr/attr_escaping.swift

@@ -72,15 +72,14 @@ struct GenericStruct<T> {}

 func misuseEscaping(_ a: @escaping Int) {} // expected-error{{@escaping attribute only applies to function types}} {{26-36=}}
 func misuseEscaping(_ a: (@escaping Int)?) {} // expected-error{{@escaping attribute only applies to function types}} {{27-36=}}
-func misuseEscaping(opt a: @escaping ((Int) -> Int)?) {} // expected-error{{@escaping attribute only applies to function types}} {{28-38=}}
-  // expected-note@-1{{closure is already escaping in optional type argument}}
+func misuseEscaping(opt a: @escaping ((Int) -> Int)?) {} // expected-error{{closure is already escaping in optional type argument}} {{28-38=}}

 func misuseEscaping(_ a: (@escaping (Int) -> Int)?) {} // expected-error{{@escaping attribute may only be used in function parameter position}} {{27-36=}}


I believe in this call and all cases below it we wouldn't actually produce a @escaping attribute may only be used in function parameter position diagnostic anymore.

Unfortunately, we would. They produced from a different part of code, to be precise from 2387-2394 lines in TypeCheckType.cpp
I'm not sure that we should fix these cases too because it's correct to produce @escaping attribute may only be used in function parameter position here cause escaping inside closure declaration and it's an error too
But I can fix this behavior 🧐
@xedin

Let's try to fix all these cases because optional closure diagnostic supersedes the one about function type.

No problems, I’ll give it a try 👍🏻

xedin

It's starting to shape up pretty well, thank you! Regarding clang-format there is a git-clang-format in llvm-project repo you can add to your PATH which could be used to format files after you git add them.

xedin

Looks good!

xedin · 2020-04-06T17:04:04Z

@swift-ci please smoke test

xedin · 2020-04-06T19:04:04Z

Thank you, @ismetanin! Please don't forget to resolve associated JIRA.

Add new error diagnostic for escaping optional closures

3dacbed

theblixguy requested a review from xedin April 1, 2020 08:48

AnthonyLatsis reviewed Apr 1, 2020

View reviewed changes

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved

xedin reviewed Apr 1, 2020

View reviewed changes

test/attr/attr_escaping.swift Outdated Show resolved Hide resolved

AnthonyLatsis reviewed Apr 2, 2020

View reviewed changes

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

AnthonyLatsis reviewed Apr 2, 2020

View reviewed changes

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved

AnthonyLatsis reviewed Apr 2, 2020

View reviewed changes

Make escaping_optional_type_argument to be an error, use it instead o…

5f68f7a

…f original message when object type is a function type

xedin reviewed Apr 2, 2020

View reviewed changes

xedin requested changes Apr 2, 2020

View reviewed changes

ismetanin added 2 commits April 4, 2020 20:22

Fix escaping_optional_type_argument producing behavior

f9b1a52

Unify diagnose invalid attr producing

0beddca

xedin approved these changes Apr 6, 2020

View reviewed changes

xedin merged commit cbf3cab into apple:master Apr 6, 2020

swift-ci mentioned this pull request Apr 6, 2020

[SR-4637] Add fix-it for removing '@escaping' from optional closure parameter #47214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new error diagnostic for escaping optional closures #30746

Add new error diagnostic for escaping optional closures #30746

ismetanin commented Apr 1, 2020

AnthonyLatsis Apr 2, 2020

theblixguy Apr 2, 2020

ismetanin Apr 2, 2020

AnthonyLatsis Apr 2, 2020

ismetanin Apr 2, 2020

AnthonyLatsis Apr 2, 2020

LucianoPAlmeida Apr 2, 2020

AnthonyLatsis Apr 2, 2020

ismetanin Apr 2, 2020

xedin Apr 2, 2020

ismetanin Apr 4, 2020

AnthonyLatsis Apr 4, 2020

ismetanin Apr 5, 2020

AnthonyLatsis Apr 5, 2020

LucianoPAlmeida Apr 5, 2020 •

edited

ismetanin Apr 5, 2020

ismetanin Apr 5, 2020

LucianoPAlmeida Apr 5, 2020

xedin Apr 2, 2020

ismetanin Apr 3, 2020

xedin Apr 3, 2020

ismetanin Apr 3, 2020

xedin left a comment

xedin left a comment

xedin commented Apr 6, 2020

xedin commented Apr 6, 2020

Add new error diagnostic for escaping optional closures #30746

Add new error diagnostic for escaping optional closures #30746

Conversation

ismetanin commented Apr 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianoPAlmeida Apr 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin left a comment

Choose a reason for hiding this comment

xedin left a comment

Choose a reason for hiding this comment

xedin commented Apr 6, 2020

xedin commented Apr 6, 2020

LucianoPAlmeida Apr 5, 2020 •

edited