[Diag] Make fixItReplace slightly smart #4499

rintaro · 2016-08-25T04:04:03Z

What's in this pull request?

When replace something with a punctuator, we often prefer adding spaces around it.
For instance,

  typealias Foo: Int
  // fix it
  typealias Foo = Int

In this case we want to add a space before =, but not after that.

With this change, we can simply fixItReplace(ColonLoc, " = ").
fixItReplace() automatically adjust the spaces around it.

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:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
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:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
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 · 2016-08-25T04:26:15Z

@swift-ci Please smoke test

jtbandes · 2016-08-25T04:39:26Z

test/Parse/typealias.swift

-typealias Foo : Int  // expected-error {{expected '=' in typealias declaration}} {{15-16==}}
+typealias Foo1 : Int  // expected-error {{expected '=' in typealias declaration}} {{16-17==}}
+typealias Foo2: Int  // expected-error {{expected '=' in typealias declaration}} {{15-16= =}}
+typealias Foo3 :Int  // expected-error {{expected '=' in typealias declaration}} {{16-17== }}



Test case for comments here too? typealias Foo4:/*comment*/Int

Done. Thanks!

jtbandes · 2016-08-25T04:39:39Z

Nice 😄

When replace something with a punctuator, we often prefer adding spaces around it. For instance, func foo(): bar {} // fix it func foo() -> bar {} In this case we want to add a space before '->', but not after that. With this change, we can simply `fixItReplace(ColonLoc, " -> ")`. `fixItReplace()` automatically adjust the spaces around it.

rintaro · 2016-08-25T04:53:14Z

@swift-ci Please smoke test

rintaro · 2016-08-25T06:25:30Z

@swift-ci Please smoke test Linux platform

DougGregor · 2016-08-25T15:55:52Z

lib/AST/DiagnosticEngine.cpp

+
+  // If we're replacing with something that wants spaces around it, do a bit of
+  // extra work so that we don't suggest extra spaces.
+  if (Str.back() == ' ') {


I suggest defending against the empty string here, just in case...

@DougGregor
It's redundant because a few lines above:

if (Str.empty()) return fixItRemove(R);

jrose-apple · 2016-08-25T15:56:01Z

That's pretty good. Is there a way to share the logic with fixItRemove?

jrose-apple · 2016-08-25T17:36:11Z

lib/AST/DiagnosticEngine.cpp

+/// \brief Extract a character at \p Loc. If \p Loc is the end of the buffer,
+/// return '\f'.
+static char extractCharAfter(SourceManager &SM, SourceLoc Loc) {
+    auto chars = SM.extractText({Loc, 1});


Nitpick: indentation is two characters.

jrose-apple · 2016-08-25T17:39:04Z

@swift-ci Please test

rintaro · 2016-08-25T17:55:41Z

@swift-ci Please smoke test

rintaro · 2016-08-25T17:59:25Z

Before indentation fix(b9c2089):
macOS: https://ci.swift.org/view/Pull%20Request/job/swift-PR-osx/3430/
Linux: https://ci.swift.org/view/Pull%20Request/job/swift-PR-Linux/3596/

HEAD(96600fe):
macOS: https://ci.swift.org/view/Pull%20Request/job/swift-PR-osx-smoke-test/1387/
Linux: https://ci.swift.org/view/Pull%20Request/job/swift-PR-linux-smoke-test/870/

shahmishal · 2016-08-25T18:09:28Z

@swift-ci Please test

swift-ci · 2016-08-25T18:41:59Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 96600fe
Test requested by - @shahmishal

rintaro · 2016-08-25T19:02:01Z

Unrelated non-deterministic error on Linux?

11:41:33 
TestFoundation/TestNSOperationQueue.swift:63: error: TestNSOperationQueue.test_OperationPriorities : XCTAssertEqual failed: ("Operation1 executed") is not equal to ("Operation3 executed") - 
11:41:33 TestFoundation/TestNSOperationQueue.swift:64: error: TestNSOperationQueue.test_OperationPriorities : XCTAssertEqual failed: ("Operation3 executed") is not equal to ("Operation1 executed") -

@swift-ci Please test Linux platform

jrose-apple · 2016-08-30T17:52:58Z

Just to make sure nothing's changed,

@swift-ci Please smoke test

jrose-apple · 2016-08-30T23:43:05Z

@swift-ci Please smoke test Linux platform

jrose-apple · 2016-08-31T00:28:21Z

@rintaro, this is ready to merge, yes?

rintaro · 2016-08-31T00:54:52Z

@jrose-apple
Yes, please merge. I'm not authorized to merge into master.

rintaro force-pushed the fixitreplace-smart branch from 4e4b1e7 to 37fc797 Compare August 25, 2016 04:17

rintaro mentioned this pull request Aug 25, 2016

[QoI] Diagnose initializers written as typed patterns (SR-1461) #4371

Merged

1 task

jtbandes reviewed Aug 25, 2016
View reviewed changes

rintaro force-pushed the fixitreplace-smart branch from 37fc797 to ba982bc Compare August 25, 2016 04:52

DougGregor reviewed Aug 25, 2016
View reviewed changes

rintaro force-pushed the fixitreplace-smart branch from 1c21996 to b9c2089 Compare August 25, 2016 17:35

jrose-apple reviewed Aug 25, 2016
View reviewed changes

[Diag] Factor-out some common logic

96600fe

rintaro force-pushed the fixitreplace-smart branch from b9c2089 to 96600fe Compare August 25, 2016 17:40

jrose-apple merged commit e44e0d9 into swiftlang:master Aug 31, 2016

rintaro deleted the fixitreplace-smart branch August 31, 2016 07:15

[Diag] Make fixItReplace slightly smart #4499

[Diag] Make fixItReplace slightly smart #4499

Uh oh!

Conversation

rintaro commented Aug 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Uh oh!

rintaro commented Aug 25, 2016

Uh oh!

jtbandes Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

rintaro Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

jtbandes commented Aug 25, 2016

Uh oh!

rintaro commented Aug 25, 2016

Uh oh!

rintaro commented Aug 25, 2016

Uh oh!

DougGregor Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

rintaro Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 25, 2016

Uh oh!

jrose-apple Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 25, 2016

Uh oh!

rintaro commented Aug 25, 2016

Uh oh!

rintaro commented Aug 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shahmishal commented Aug 25, 2016

Uh oh!

swift-ci commented Aug 25, 2016

Uh oh!

rintaro commented Aug 25, 2016

Uh oh!

jrose-apple commented Aug 30, 2016

Uh oh!

jrose-apple commented Aug 30, 2016

Uh oh!

jrose-apple commented Aug 31, 2016

Uh oh!

rintaro commented Aug 31, 2016

Uh oh!

Uh oh!

rintaro commented Aug 25, 2016 •

edited

Loading

rintaro commented Aug 25, 2016 •

edited

Loading