Skip to content

[Fix-it] Support defaulted and variadic arguments in renamed fix-it #3998

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 5, 2016
Merged

[Fix-it] Support defaulted and variadic arguments in renamed fix-it #3998

merged 2 commits into from
Aug 5, 2016

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Aug 4, 2016 •
edited
Loading

What's in this pull request?

CC: @jrose-apple

Renamed fix-it used to give-up emitting fix-it for argument labels if the arguments have some defaulted or variadic arguments.

This PR should address UnsafeMutablePointer.initalize(with:count:) fix-it.
https://bugs.swift.org/browse/SR-2264

error: 'initialize(with:count:)' has been renamed to 'initialize(to:count:)'
  ptr.initialize(with: 42)
      ^~~~~~~~~~ ~~~~
      initialize to

I will add some formal test cases for these stdlib renames to test/1_stdlib/Renames.swift.
Done.

Resolved bug number: (SR-2264)

Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library only for macOS. Simulator standard libraries and
    device standard libraries are not built.
  3. lldb is not built.
  4. The test and validation-test targets are run only for macOS. The optimized
    version of these tests are not run.

A smoke test on Linux does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library incrementally.
  3. lldb is built incrementally.
  4. The swift test and validation-test targets are run. The optimized version of these
    tests are not run.
  5. lldb is tested.

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@rintaro rintaro force-pushed the rename-fixit-tupleshuffle branch from f44555e to f193169 Compare August 4, 2016 12:57
@rintaro
Copy link
Member Author

rintaro commented Aug 4, 2016

Updated UnsafeMutablePointer renamed APIs test (added fix-it).
CC: @gribozavr @atrick

@rintaro
Copy link
Member Author

rintaro commented Aug 4, 2016

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Aug 4, 2016

@DougGregor Mind reviewing this?

NachoSoto
NachoSoto reviewed Aug 4, 2016
View reviewed changes
test/1_stdlib/Renames.swift

let ptr1 = UnsafeMutablePointer<T>(allocatingCapacity: 1) // expected-error {{'init(allocatingCapacity:)' is unavailable: use 'UnsafeMutablePointer.allocate(capacity:)'}} {{none}}
ptr1.initialize(with: e, count: 1) // expected-error {{'initialize(with:count:)' has been renamed to 'initialize(to:count:)'}}
ptr1.initialize(with: e, count: 1) // expected-error {{'initialize(with:count:)' has been renamed to 'initialize(to:count:)'}} {{8-18=initialize}} {{19-23=to}} {{none}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

@rintaro rintaro force-pushed the rename-fixit-tupleshuffle branch from c6262cc to 8f5fca0 Compare August 4, 2016 14:24
@rintaro
Copy link
Member Author

rintaro commented Aug 4, 2016

@swift-ci Please smoke test

jrose-apple
jrose-apple reviewed Aug 4, 2016
View reviewed changes
lib/Sema/MiscDiagnostics.cpp
}
break;
}
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the actual case(s) here, so that it's a fully-covered switch. That way we can get warnings if someone adds a new kind of tuple shuffle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot use explicit case value here,
because the subject value is basically a index of the arguments.
I think the assertion below (suffleIdx == (I - argumentLabelIDs.begin() )
will catch any new kind of special value.
Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, that makes sense. I wasn't reading closely enough and assumed it was a real enum. Thanks!

@jrose-apple
Copy link
Contributor

This looks great! Some more possible test cases:

func defaultBeforeRequired(a: Int = 1, b: Int) {}
defaultBeforeRequired(b: 5)

func defaultPlusTrailingClosure(a: Int = 1, b: Int = 2, c: () -> Void) {}
defaultPlusTrailingClosure {}
defaultPlusTrailingClosure(c: {})
defaultPlusTrailingClosure(a: 1) {}

@rintaro
Copy link
Member Author

rintaro commented Aug 4, 2016

@jrose-apple Thank you for test case suggestion! I will add them. 👍

@rintaro
Copy link
Member Author

rintaro commented Aug 4, 2016

@swift-ci Please smoke test

DougGregor
DougGregor reviewed Aug 4, 2016
View reviewed changes
lib/Sema/MiscDiagnostics.cpp
return;
}
argExpr = args->getSubExpr();
auto I = argumentLabelIDs.begin();
for (auto suffleIdx : elementMap) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/suffleIdx/shuffleIdx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh. 💨

@rintaro rintaro force-pushed the rename-fixit-tupleshuffle branch from 9da4388 to 375b87c Compare August 4, 2016 17:06
@rintaro
Copy link
Member Author

rintaro commented Aug 5, 2016

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Aug 5, 2016

Unrelated failure on Linux. merging.

@rintaro rintaro merged commit 3db6fd2 into swiftlang:master Aug 5, 2016
@jrose-apple
Copy link
Contributor

Given all our renames, this seems useful enough to pull into the 3.0 branch. I'll make a pull request for that.

jrose-apple pushed a commit to jrose-apple/swift that referenced this pull request Aug 5, 2016
@rintaro @jrose-apple
Merge pull request swiftlang#3998 from rintaro/rename-fixit-tupleshuffle
c91da06 
[Fix-it] Support defaulted and variadic arguments in renamed fix-it
(cherry picked from commit 3db6fd2)
@rintaro rintaro deleted the rename-fixit-tupleshuffle branch August 5, 2016 16:08
jrose-apple added a commit that referenced this pull request Aug 5, 2016
@jrose-apple
[Fix-it] Support defaulted and variadic arguments in renamed fix-it (#…
819cd0d 
…4044)

Originally from pull request #3998 from rintaro/rename-fixit-tupleshuffle
@rintaro rintaro mannequin mentioned this pull request Aug 4, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rintaro @jrose-apple @NachoSoto @DougGregor