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

[Parse] Enable self rebinding (to self) #15306

Merged
merged 1 commit into from Mar 26, 2018

Conversation

Projects
None yet
6 participants
@dduan
Collaborator

dduan commented Mar 16, 2018

As discussed in this thread, this patch allows "self" to be an identifier bound to self (see added tests for examples), and is useful in closures where self is weakly captured to avoid retain cycles. The current behavior of allowing

guard let `self` = self else { ... }

remains for source compatibility.

Instead of parsing identifiers and fallthrough to parsing self, treat kw_self the same as identifiers.

@dduan dduan changed the title from Enable self rebinding (to self) to [Parse] Enable self rebinding (to self) Mar 16, 2018

@rintaro

This comment has been minimized.

Member

rintaro commented Mar 17, 2018

Could you add a test cases for implicit method/property references?

class MyCls {
  func something() {}

  func test() {
    let self = OtherCls() // Even if `self` is shadowed,
    something() // this should still refer `MyCls.something`.
  }
}

I know this currently works. But it prevents accidental breakage in the future.

@dduan

This comment has been minimized.

Collaborator

dduan commented Mar 17, 2018

@rintaro added

@apple apple deleted a comment from swift-ci Mar 17, 2018

@apple apple deleted a comment from swift-ci Mar 17, 2018

@dduan

This comment has been minimized.

Collaborator

dduan commented Mar 17, 2018

@swift-ci please test

@apple apple deleted a comment from swift-ci Mar 17, 2018

@apple apple deleted a comment from swift-ci Mar 17, 2018

@dduan

This comment has been minimized.

Collaborator

dduan commented Mar 20, 2018

@rintaro does everything else look mergeable to you? cc @DougGregor (owner)

@rintaro

This comment has been minimized.

Member

rintaro commented Mar 20, 2018

No more objection from me. Defer to @DougGregor (or other core team member) on the decision.

test/Parse/self_rebinding.swift Outdated
func something() {}
func test() {
// expected-warning @+1 {{}}

This comment has been minimized.

@DougGregor

DougGregor Mar 26, 2018

Member

It'd be nice to have some part of the warning text here, for others that read the test later on. Is the warning about rebinding self to an unrelated type (which would be a really good warning, IMO!), or is it something else?

This comment has been minimized.

@dduan

dduan Mar 26, 2018

Collaborator

in this case it's just about the variable being unused. I included the text.

test/Parse/self_rebinding.swift Outdated
}
}
Writer().nonStop()

This comment has been minimized.

@DougGregor

DougGregor Mar 26, 2018

Member

You can delete this line; it doesn't add anything.

[Parse] Allow `self` rebinding
Instead of parisng identifiers and fallthrough to self, treat `kw_self` the same
as identifiers. This enables binding to self.
@dduan

This comment has been minimized.

Collaborator

dduan commented Mar 26, 2018

@swift-ci please test

@apple apple deleted a comment from swift-ci Mar 26, 2018

@apple apple deleted a comment from swift-ci Mar 26, 2018

@dduan dduan merged commit def71e0 into apple:master Mar 26, 2018

4 checks passed

Swift Test Linux Platform 10889 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 54545 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

@dduan dduan deleted the dduan:self-rebinding branch Mar 26, 2018

@acecilia

This comment has been minimized.

acecilia commented Jun 9, 2018

Is this PR linked to an evolution proposal?

There is an old evolution proposal here already merged, but in Deferred status. Seen the general acceptance shown in the discussion thread + the merge of this PR, would it make sense to consider updating the evolution proposal and taking it back to life?

@lattner

This comment has been minimized.

Collaborator

lattner commented Aug 30, 2018

@dduan and @DougGregor can you two look at revising and updating SE-0079? It is related to this patch and is causing confusion for people who think that this behavior is not supposed to be supported.

@dduan

This comment has been minimized.

Collaborator

dduan commented Aug 30, 2018

Will a simple update of status suffice? apple/swift-evolution#900

@mdiep

This comment has been minimized.

Contributor

mdiep commented Aug 31, 2018

Unfortunately, it seems like this may still not work in LLDB: https://bugs.swift.org/browse/SR-6156 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment