Skip to content
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.

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

Allow implicit self in escaping closures when self is explicitly captured #23934

Open
wants to merge 13 commits into
base: master
from

Conversation

@Jumhyn
Copy link
Contributor

commented Apr 10, 2019

Update the lookup logic for closures to allow resolution of implicit self to resolve to any direct captures of self (e.g. { [self] in ... }) in closures. Also introduce new diagnostic logic to suggest such a capture as a fix-it for implicit self in an escaping closure.

This is the implementation for an incoming Swift Evolution proposal.

Resolves SR-10218.

@Jumhyn Jumhyn force-pushed the Jumhyn:implicit-self-capture branch from 6778567 to 3446add Jul 21, 2019

@gregomni
Copy link
Collaborator

left a comment

This is awesome, just a few suggestions.

include/swift/AST/Expr.h Show resolved Hide resolved
include/swift/AST/Expr.h Outdated Show resolved Hide resolved
lib/AST/UnqualifiedLookup.cpp Outdated Show resolved Hide resolved
lib/Sema/MiscDiagnostics.cpp Outdated Show resolved Hide resolved
test/expr/closure/closures.swift Show resolved Hide resolved
@Jumhyn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Hi @gregomni, thanks for the comments! I’ll have a chance to make changes re: your feedback this weekend.

@@ -178,6 +178,7 @@ namespace {
const bool isOriginallyTypeLookup;
const NLOptions baseNLOptions;
// Transputs
DeclContext *capturedSelfContext;

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 4, 2019

Author Contributor

Is this an appropriate place for this? It's definitely not an input or an output...

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 13, 2019

Collaborator

It looks to me like this is just some state used within UnqualifiedLookupFactory; it's neither an input nor an output. Is this correct?

If so, I think you should put it below all of the other member variables, make it private, and add a comment explaining what it's for and what it represents.

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 13, 2019

Author Contributor

Yeah that's correct. Will do.

@Jumhyn Jumhyn force-pushed the Jumhyn:implicit-self-capture branch from c0efff6 to df5f6a1 Aug 4, 2019

@brentdax
Copy link
Collaborator

left a comment

This looks pretty good! I have some minor suggestions, but they're mostly stylistic, plus a couple of edge cases.

SmallVectorImpl<CaptureListEntry> &captureList,
VarDecl *&capturedSelfParamDecl,

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 13, 2019

Collaborator

Nit: Reindent the parameters so that all of them are four spaces in from the line above. (Someone before you followed Xcode's default indenting instead of the style guide.)

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 13, 2019

Author Contributor

@brentdax Not 100% clear what you mean here—do you mean that this should be indented like this,

bool parseClosureSignatureIfPresent(
    SourceRange &bracketRange,
    SmallVectorImpl<CaptureListEntry> &captureList,
    VarDecl *&capturedSelfParamDecl,
    ParameterList *&params,
    SourceLoc &throwsLoc,
    SourceLoc &arrowLoc,
    TypeRepr *&explicitResultType,
    SourceLoc &inLoc);

like this,

bool parseClosureSignatureIfPresent(
                                  SourceRange &bracketRange,
                              SmallVectorImpl<CaptureListEntry> &captureList,
                                  VarDecl *&capturedSelfParamDecl,
                                  ParameterList *&params,
                                  SourceLoc &throwsLoc,
                                  SourceLoc &arrowLoc,
                                  TypeRepr *&explicitResultType,
                                  SourceLoc &inLoc);

or something else? Also, I couldn't immediately find the style guide you're referencing. Is it published somewhere?

This comment has been minimized.

Copy link
@gregomni

gregomni Aug 13, 2019

Collaborator

The style guide is https://llvm.org/docs/CodingStandards.html

You may want to google "git-clang-format", which is a really convenient tool to format any code that you've currently changed to fit the formatting guidelines.

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 13, 2019

Author Contributor

@gregomni Thank you! Will make use of that.

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 14, 2019

Collaborator

I meant this one:

bool parseClosureSignatureIfPresent(
    SourceRange &bracketRange,
    SmallVectorImpl<CaptureListEntry> &captureList,
    VarDecl *&capturedSelfParamDecl,
    ParameterList *&params,
    SourceLoc &throwsLoc,
    SourceLoc &arrowLoc,
    TypeRepr *&explicitResultType,
    SourceLoc &inLoc);

We are not totally consistent in our formatting, unfortunately, but especially when something is too long like captureList is, this is the preferred format.

@@ -178,6 +178,7 @@ namespace {
const bool isOriginallyTypeLookup;
const NLOptions baseNLOptions;
// Transputs
DeclContext *capturedSelfContext;

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 13, 2019

Collaborator

It looks to me like this is just some state used within UnqualifiedLookupFactory; it's neither an input nor an output. Is this correct?

If so, I think you should put it below all of the other member variables, make it private, and add a comment explaining what it's for and what it represents.

diag.fixItInsertAfter(range.Start, "self, ");
}
}
TC.diagnose(memberLoc, diag::note_reference_self_explicitly)

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 13, 2019

Collaborator

I think it might be better to reverse the order of the two fix-its, offering self. first. The reason is teachability: Capture lists are a relatively advanced feature that we don't want people who are learning to program to worry about.

(This is just my opinion, though.)

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 13, 2019

Author Contributor

That seems reasonable to me. Initially had the other fix-it first since the capture list occurs syntactically "before" the actual member usage, but if the ordering doesn't matter in that respect I'm not attached to the ordering used here.

This comment has been minimized.

Copy link
@paiv

paiv Aug 14, 2019

For teaching, capture list is better suited to crystallize the concept of capturing. "Explicit self. is confusing" is one of the reasons for this SR. Maybe leave this to community discussion?

if (range.Start.getAdvancedLoc(1) == range.End)
diag.fixItInsertAfter(range.Start, "self");
else
diag.fixItInsertAfter(range.Start, "self, ");

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 13, 2019

Collaborator

Rather than having four very slightly different strings, I wonder if this could be handled more elegantly by multiple fixItInsertAfter() calls. Something like:

auto brackets = CE->getBracketRange();
if (brackets.isInvalid())
  diag.fixItInsertAfter(CE->getLoc(), "[");

diag.fixItInsertAfter(CE->getLoc(), "self");

if (brackets.isInvalid())
  diag.fixItInsertAfter(CE->getLoc(), "]");
else if (brackets.Start.getAdvancedLoc(1) != brackets.End)
  diag.fixItInsertAfter(CE->getLoc(), ", ");

if (CE->getInLoc().isInvalid())
  diagnostics.fixItInsertAfter(CE->getLoc(), " in");

Speaking of which, how does that getAdvancedLoc() thing work out if you have a capture list which is not textually empty, but has no tokens in it? For instance, [ ] in or [ /* this space intentionally left blank */ ] in?

And are there cases where it will insert an in which will merge with the token after it?

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 13, 2019

Author Contributor

Oh neat, didn't realize that fixItInsertAfter could be used like that--that's much nicer. The trailing logic in lines 1425-1430 should handle the case of a token immediately following the opening brace, but let me know if I'm missing an edge case there.

You're right about the getAdvancedLoc(1) logic there. Will have to do a little more work to handle that case.

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 14, 2019

Collaborator

Well, I'm not 100% sure how it will work out. 😅 If it does work, I think it would be easier to understand. (Maybe it'd be even easier to understand if you had named variables for the different conditions, like needsBrackets, needsComma, needsIn, etc.)

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 15, 2019

Author Contributor

Seems like breaking up the fixItInsertAfter calls unfortunately offers separate fix-its :( will refactor this to make the logic a bit clearer though.

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

If you fix the merge conflict and @ me, I'll kick off tests.

SourceRange BracketRange;

/// The (possibly null) VarDecl captured with the literal name "self".
VarDecl *CapturedSelfDecl;

This comment has been minimized.

Copy link
@brentdax

brentdax Aug 13, 2019

Collaborator

Oh, one more suggestion: Since the question of "why didn't you implement this by reading the capture list?" has come up a few times from different people, it might make sense to include a comment explaining why you're using the capturedSelfContext member variable instead. That way people will know the reason just by reading the code.

This comment has been minimized.

Copy link
@Jumhyn

Jumhyn Aug 13, 2019

Author Contributor

Sounds good!

@Jumhyn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

@brentdax Implementation updated and conflicts resolved. Made some new changes in lib/Parse/Lexer.cpp to better handle the bracket cases, so might want to give some thought there as to whether it will affect anything that I didn't consider. I think the change should be safe, and tests all run locally for me, but it's a member function with a lot of clients so there may be an edge case that I'm not thinking of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.