SE-0285: Ease the transition to concise magic file strings

The review of SE-0285: Ease the transition to concise magic file strings begins now and runs through July 16, 2020.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via email or direct message in the Swift forums).

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.

When reviewing a proposal, here are some questions to consider:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Thanks,
Tom Doron
Review Manager

9 Likes

+1.

Yes. While the scope of the change is minor, the value for libraries supporting back multiple Swift versions is high.

Yes, it is a straightforward refinement of a previously accepted enhancement that does not meaningfully affect the feel of the language.

In-depth study.

1 Like

The idea of SE-0274 was to avoid including full paths in compiled binaries to reduce bloat, avoid differences depending on the machine you build on, and avoid leaking details of the build environment in binaries you pass around. If the result of SE-0274 is libraries migrating from #file to #filePath in their function default arguments, then SE-0274 has clearly failed its goals.

I think giving more time so libraries can migrate to #filePath is an anti-goal. Giving more time so they don't have to migrate to #filePath is better. So what will be the result of giving more time?

Personally, I'm a bit disappointed that SE-0274 still allows libraries to take the caller's file path implicitly. With print(#filePath) it's clear you're leaking your current file path whereas with printSourcePath() it isn't. I think it's fine to have #filePath, but it probably shouldn't be allowed as a default argument as it undermines the initial motivation for this change.

2 Likes

I don’t see how the “burdensome” conclusion is actually burdensome. Am I missing something?

The maintainers of large Swift libraries found that, in practice, this duplication was very painful. For instance, consider what Corelibs XCTest (which fortunately doesn’t need to maintain source compatibility quite so stringently) might have to do for just one #filePath-capturing assertion:

#if compiler(>=5.3)
/// This function emits a test failure if the general `Boolean` expression passed
/// to it evaluates to `false`.
///
/// - Requires: This and all other XCTAssert* functions must be called from
///   within a test method, as passed to `XCTMain`.
///   Assertion failures that occur outside of a test method will *not* be
///   reported as failures.
///
/// - Parameter expression: A boolean test. If it evaluates to `false`, the
///   assertion fails and emits a test failure.
/// - Parameter message: An optional message to use in the failure if the
///   assertion fails. If no message is supplied a default message is used.
/// - Parameter file: The file name to use in the error message if the assertion
///   fails. Default is the file containing the call to this function. It is
///   rare to provide this parameter when calling this function.
/// - Parameter line: The line number to use in the error message if the
///   assertion fails. Default is the line number of the call to this function
///   in the calling file. It is rare to provide this parameter when calling
///   this function.
///
/// - Note: It is rare to provide the `file` and `line` parameters when calling
///   this function, although you may consider doing so when creating your own
///   assertion functions. For example, consider the following custom assertion:
///
///   ```
///   // AssertEmpty.swift
///
///   func AssertEmpty<T>(_ elements: [T]) {
///       XCTAssertEqual(elements.count, 0, "Array is not empty")
///   }
///   ```
///
///  Calling this assertion will cause XCTest to report the failure occurred
///  in the file where `AssertEmpty()` is defined, and on the line where
///  `XCTAssertEqual` is called from within that function:
///
///  ```
///  // MyFile.swift
///
///  AssertEmpty([1, 2, 3]) // Emits "AssertEmpty.swift:3: error: ..."
///  ```
///
///  To have XCTest properly report the file and line where the assertion
///  failed, you may specify the file and line yourself:
///
///  ```
///  // AssertEmpty.swift
///
///  func AssertEmpty<T>(_ elements: [T], file: StaticString = #file, line: UInt = #line) {
///      XCTAssertEqual(elements.count, 0, "Array is not empty", file: file, line: line)
///  }
///  ```
///
///  Now calling failures in `AssertEmpty` will be reported in the file and on
///  the line that the assert function is *called*, not where it is defined.
public func XCTAssert(_ expression: @autoclosure () throws -> Bool, _ message: @autoclosure () -> String = "", file: StaticString = #filePath, line: UInt = #line) {
    XCTAssertTrue(try expression(), message(), file: file, line: line)
}
#else
/// This function emits a test failure if the general `Boolean` expression passed
/// to it evaluates to `false`.
///
/// - Requires: This and all other XCTAssert* functions must be called from
///   within a test method, as passed to `XCTMain`.
///   Assertion failures that occur outside of a test method will *not* be
///   reported as failures.
///
/// - Parameter expression: A boolean test. If it evaluates to `false`, the
///   assertion fails and emits a test failure.
/// - Parameter message: An optional message to use in the failure if the
///   assertion fails. If no message is supplied a default message is used.
/// - Parameter file: The file name to use in the error message if the assertion
///   fails. Default is the file containing the call to this function. It is
///   rare to provide this parameter when calling this function.
/// - Parameter line: The line number to use in the error message if the
///   assertion fails. Default is the line number of the call to this function
///   in the calling file. It is rare to provide this parameter when calling
///   this function.
///
/// - Note: It is rare to provide the `file` and `line` parameters when calling
///   this function, although you may consider doing so when creating your own
///   assertion functions. For example, consider the following custom assertion:
///
///   ```
///   // AssertEmpty.swift
///
///   func AssertEmpty<T>(_ elements: [T]) {
///       XCTAssertEqual(elements.count, 0, "Array is not empty")
///   }
///   ```
///
///  Calling this assertion will cause XCTest to report the failure occurred
///  in the file where `AssertEmpty()` is defined, and on the line where
///  `XCTAssertEqual` is called from within that function:
///
///  ```
///  // MyFile.swift
///
///  AssertEmpty([1, 2, 3]) // Emits "AssertEmpty.swift:3: error: ..."
///  ```
///
///  To have XCTest properly report the file and line where the assertion
///  failed, you may specify the file and line yourself:
///
///  ```
///  // AssertEmpty.swift
///
///  func AssertEmpty<T>(_ elements: [T], file: StaticString = #file, line: UInt = #line) {
///      XCTAssertEqual(elements.count, 0, "Array is not empty", file: file, line: line)
///  }
///  ```
///
///  Now calling failures in `AssertEmpty` will be reported in the file and on
///  the line that the assert function is *called*, not where it is defined.
public func XCTAssert(_ expression: @autoclosure () throws -> Bool, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line) {
    XCTAssertTrue(try expression(), message(), file: file, line: line)
}
#endif

(Yes, you have to repeat the entire doc comment.)

That’s...rather unfortunate. Library maintainers basically said that, if they had realized they’d need to duplicate so much code to adopt #filePath without breaking backwards compatibility, they would have asked for the proposal to be revised.

Thanks for the clarification. If the docs didn’t need to be duplicated I would think the work of making the separate functions is fine, but I agree that copying everything is burdensome.

Yeah, it's super inconvenient to repeat the whole thing. But at the same time this is the perfect example of something that SE-0285 wanted to change: a library implicitly leaking the complete file path of its caller in the executable. Switching to #filePath here should be a non-goal.

The question is: are we considering a delay in order for libraries to be able to more easily switch to #filePath, or until the surrounding tooling can handle the new #file? If the former, we might as well just abandon SE-0285 because if every library ends up using #filePath the new pathless form will stay marginal and useless at achieving its goals.

1 Like

Except that XCTAssert is used in tests; the privacy justifications don’t really matter for tests because test code isn’t distributed to anyone, and the performance of the test assertions themselves (as opposed to the library under test) is not really sensitive to the size difference.

My understanding is that most (possibly all) of the places where libraries wanted to use #filePath were in test code. But they still wanted to make sure that this test code worked on multiple compiler versions, and doing so was still quite painful for them.

1 Like

The test code will still work though. It'll just print errors at non-path locations.

Using a file-id, you can run tests on an external CI system have errors nicely mapped to a different path on your local copy. And you can more easily compare the raw output of those tests built and run from two different machines because you won't have to sort out the difference in paths. The test output is more deterministic and reproducible.

In my opinion #file is a better choice for assertion errors, including in unit tests. I understand tools parsing the output might need more time to cope (which is fine by me). But when the stated motivation is migrating to using #filePath I find it disappointing to see this is all to avoid embracing the benefits of the new file-id.

4 Likes

Tests whose failure cannot be easily diagnosed are very troublesome. This goes double for those rare but real cases where the test failure occurs only in CI and cannot be reproduced locally.

A large amount of the open-source ecosystem supports several Swift releases back (e.g. SwiftNIO supports back to 5.0). These libraries get markedly worse test infrastructure on 5.3 because adopting #filePath/#file is so painful. This seems an excessive punishment.

  • What is your evaluation of the proposal?

+1, I supported the original proposal but now think it makes sense to stage in the changes from SE-0274 more gradually.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes, I think the bug reports from library maintainers using Swift 5.3 snapshots demonstrate the original migration plan creates compatibility issues that are difficult to work around without excessive code duplication.

  • Does this proposal fit well with the feel and direction of Swift?

Yes, the proposed solution seems in line with Swift's source compatibility principles.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I followed the original proposal, and read some of the early bug reports from when SwiftNIO tried to adopt the original changes and ran into issues maintaining compatibility with swift 5.2 & 5.3.

Toolchain links if anyone wants to try this out: macOS, Linux. These are based on the master branch as of Friday night.

+1 thank you @beccadax

Proposal Accepted

Please see the acceptance post in the Announcements section.

2 Likes