Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[PackageLoading] Improve flexibility in formatting Package manifests …
…and correctness in parsing Swift tools version specifications (#2937) * allow any number of any horizontal whitespace character between "//" and "swift-tools-version" - Replaced the regex pattern for matching 1 single space character (" ") with the pattern `\h*?` for matching 0 or more (as few as possible) horizontal whitespace character between "//" and "swift-tools-version". - Made the comments right above `regex` into `regex`'s documentation by changing "//"s into "///"s. - Specified that only _horizontal_ whitespace characters are allowed. - Expanded test cases to account for different numbers of different whitespace characters. - FIXME: Newline and line feed cause tests failures. * [NFC] improve `regex`'s documentation style * [NFC][Gardening] add missing period puctuation in `regex`'s documentation * throw an error for Swift tools version ≤ 5.3 when anything but a single U+0020 is used as the spacing between "//" and "swift-tools-version" Up to and through Swift 5.3, the format of the swift tools version specification must be prefixed exactly with "// swift-tools-version:", where there is 1 and only 1 space character (U+0020) between "//" and "swift-tools-version". Even though future versions (> 5.3) will support any combination of horizontal whitespace characters for the spacing, if the specified Swift tools version ≤ 5.3, then the specification must use "// swift-tools-version:" to be backward compatible with lower Swift versions. A new capture group is added to the regex pattern, for the continuous sequence of whitespace characters between "//" and "swift-tools-version". The capture group for the version specifier remains unchanged, but becomes the 2nd capture group and the 3rd matching range. If both the whitespace sequence and the version specifier are present, and if the version specifier is valid, then compare the version with 5.3. If Swift tools version ≤ 5.3, then throw an error if the whitespace sequence contains anything but a single U+0020. The error informs the user the whitespace characters used, and informs the user that only a single U+0020 is valid. The test function `testBasics()` is renamed to `testValidVersions` to better reflect what it tests. All valid versions that use anything but a single U+0020 as spacing have their version specifier raised to above 5.3. A new test function `testBackwardCompatibilityError()` is added to verify that if Swift tools version ≤ 5.3, anything but U+0020 is rejected as the spacing between "//" and "swift-tools-version". Version specifications are moved from `testNonMatching()` to a new test function `testDefault()`, because Swift tools version defaults to 3.1 if the specification reaches a newline character before any pre-defined misspelling. * replace all occurrences of "`Package.swift`" with "the manifest" in the previous commit (981d23e) The manifest is not necessarily always `Package.swift`. There could be version-specific manifests. * [NFC][Gardening] correct comments on valid versions "Swift ≥ 5.3" is corrected to "Swift > 5.3". "for Swift > 5.3" is added to those missing this qualifier. At the moment of this commit, build fails because of the following errors in `SwiftDriver`: - Cannot find 'SwiftDriverExecutor' in scope - No type named 'ExternalDependencyArtifactMap' in module 'SwiftDriver' * [NFC] add an entry describing changes introduced by #2937 * use the existing constant of known version 5.3 * use raw string for regex pattern Using raw string avoids the confusion around consecutive backslashes in regex pattern. * [Gardening][NFC] fix comment typos * [NFC][Gardening] fix typo * [NFC][Gardening] improve comments for valid versions and backward-compatibility test cases 1. Replaced parenthesised examples for whitespace characters with their code points. The examples had been silently converted to spaces (U+0020) when it was edited in Xcode last time. 2. Replaced "space character" with "space", "tab character" with "character tabulation", and "no space" with "no spacing". 3. Removed redundant slashes. * [NFC][Gardening] improve the description of additional valid spacing between "//" and "swift-tools-version" in the tools version specification - Removed "at the top of", because the Swift tools version specification can start from the second line, with only 1 U+000A in front of it. - Emphasized "horizontal". - Replaced the middle one of the 3 consecutive U+0020 in the second example with the a U+0009. This was the intention when the entry was first added, but the editor silently changed the character tabulation to a space. * allow any combination of line terminators at the start of a manifest file ## Principal Changes This commit introduces 2 backward-incompatible improvements to the parsing of the Swift tools version specification in a manifest file: 1. Instead of just line feed (`U+000A`), now all [Unicode line terminators](https://www.unicode.org/reports/tr14/) are recognised. This improvement is also source breaking. 2. Instead of either 0 or 1 line feed, now a manifest accepts unlimited number of Unicode line terminators at the start of file. This improvement partially is introduced to help with the 1st's backward-compatibility. ### Recognition of all Unicode line terminators when searching for the tools version specification in a manifest file Up through Swift 5.3.0, SPM looked for the tools version specification line by splitting the raw bytes (`[UInt8]`) of a manifest at the first `0x0A` it finds. This is done in [`ToolsVersionLoader.split(_ bytes: ByteString) -> (versionSpecifier: String?, rest: [UInt8])`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L160), and once again in [``ToolsVersionLoader.load(file: AbsolutePath, fileSystem: FileSystem) throws -> ToolsVersion`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L129)'s [first `guard` statement's `else` clause](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L137) if the the specification is invalid. Because some Unicode line terminators such as `U+2028` are more than 1-byte long regardless of the encoding, a manifest file needs to have its contents represented in `String` instead of `[UInt8]`, in order for all line terminators to be recognised easily and correctly. A new function is introduced for this task: `ToolsVersionLoader.func split(_ manifestContents: String) -> ManifestComponents`.Through the new type `ManifestComponents`, the tools version specification line is returned directly. This eliminates the need for a 2nd splicing if the specification is invalid. This new function also resolves an old [`FIXME`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L173), by returning `Substring`s in the new type `ManifestComponents`. #### Source-breakages 1. Because the new function works with `String`, a manifest must be UTF-8-encoded, or SPM throws an error informing the user that the file is not correctly encoded. This behaviour is source breaking for all existing manifests that contain invalid [invalid byte sequences](https://en.wikipedia.org/wiki/UTF-8#Invalid_sequences_and_error_handling) such as `0x7F8F`. 2. Until this commit, SPM throws a `ToolsVersionLoader.Error.malformedToolsVersion` when it sees a tools version specification (represented in `String`) such as `"// swift-\rtool-version:5.3\n"`. This is because although the specification is invalid, it contains one of the 2 pre-defined misspellings: "swift-tool" and "tool-version". With this commit, `"\r"` (`U+000D`) becomes a recognised line terminator, so the tools version specification becomes `"// swift-"` for the same manifest file. Because `"// swift-"` does not contain either misspellings, SPM silently falls back to using Swift 3.1 as the lowest supported version. ### Unlimited leading line terminators in a manifest file The old [`ToolsVersionLoader.split(_:)`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L160) uses `Collection.split(separator:maxSplits:omittingEmptySubsequences:)` to split a manifest at the first `U+000A`. This approach has an unaddressed edge case, in which if a manifest starts with a `U+000A`, the second line is mistakenly treated as the tools version specification line. This becomes a problem when the new `ToolsVersionLoader.split(_:)`, introduced in the 1st improvement, takes advantage of `String`'s subrange-accessing subscripts that always correctly slices out the actual first line from a manifest as its tools version specification. To maintain source stability, the new function skips (but records) all leading line terminators, and treat only the first line that starts with a non-line-terminating character as the tools version specification line. Additionally, this new behaviour eliminates this common source of typo bugs where a shebang-like directive is not at the start of file. #### Backward-incompatibility For Swift ≤ 5.3, SPM allows at most 1 `U+000A` and nothing more before the tools version specification. The new function records the entire sequence of leading line terminators in a manifest, then its calling function checks if the sequence is either empty or 1 `U+000A`. If not, SPM throws an error informing the user that the line terminators used are not backward-compatible, and suggests a correction. ### Deprecations 1. Although the old [`ToolsVersionLoader.split(_:)`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L160) is replaced by the new one, it's not removed. Because it's declared `public`, removing it is source-breaking. It is instead restored to how it is in the main branch (with only a minor modification to the regex matching part to preserve its main-branch behaviour), then marked as deprecated, with a message that directs the user to the new function. All calls to the old function in the project are replaced by calls to the new one, with the exception of [the one in `ToolsVersionWriter.swift`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/Workspace/ToolsVersionWriter.swift#L37). The one in `ToolsVersionWriter.swift` needs the old function to pass all tests. 2. [`ToolsVersionLoader.Error.malformedToolsVersion(_:_:)`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L104) is replaced by `ToolsVersionLoader.Error.malformedToolsVersionSpecification(_:)`. The new case distinguishes different kinds of malformation in a tools version specification, and inform the user accordingly. `malformedToolsVersion(_:_:)` is not removed because [`ToolsVersionLoader.Error`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Sources/PackageLoading/ToolsVersionLoader.swift#L90) is declared `public`. It's marked as deprecated, with a message that directs the user to the new case. All calls to the old case in this project are replaced by calls to the new one. ### Tests - 8 failing test cases with line terminators other than `U+000A` are kept and/or introduced, in order to draw attention to the source breakage. - New test cases are added to ensure that all line terminators are handled correctly. - Some test failure messages are improved, to clarify for future maintainers of what went wrong. - [`ToolsVersionLoader.assertFailure(_:_:file:line:)`](/~https://github.com/WowbaggersLiquidLunch/swift-package-manager/blob/49cfc46bc5defd3ce8e0c0261e3e2cb475bcdb91/Tests/PackageLoadingTests/ToolsVersionLoaderTests.swift#L184) is expanded to handle different kinds of malformations in a tools version specification, shadowing the introduction of `ToolsVersionLoader.Error.malformedToolsVersionSpecification(_:)`. ### Alternative approach A similar result could be achieved by changing the regex pattern to `^($)*//(\h*?)swift-tools-version:(.*?)(?:;.*|$)`, and try to match the entire manifest to it. However, it comes with 3 drawbacks: 1. Because the entire manifest is matched, the entire manifest needs to be decoded using UTF-8. This is source-breaking, as explained above. 2. Matching an entire manifest is inefficient. 3. Regular expression is usually less understandable than `String`'s index-find and substring-getting methods. It also becomes much harder to maintain when the pattern gets more complicated. * [NFC][Gardening] use more approchable language in the changelog "ye olde days" is quirky, but not very approachable to readers whose first language isn't English. Also, not everyone is familiar with the shell notation. Co-Authored-By: Anders Bertelrud <anders@apple.com> * [NFC][Gardening] refer to Swift Package Manager as "the package manager" in the changelog This follows the precedence set by the Swift 3.0 entry in the changelog. * [NFC][Gardening] remove the invalid byte sequence example in the changelog "`0x7F8F`" is removed, so nobody gets the idea that there's something special about that code. "UTF-8" clarifies what encoding the byte sequence is invalid in. Co-Authored-By: Anders Bertelrud <anders@apple.com> * remove `ToolsVersionLoader.Error.malformedToolsVersion(_:_:)` It can be removed instead of just deprecated, because `libSwiftPM` isn't API-stable yet. * [NFC][Gardening] remove deprecation list Moved notice of `ToolsVersionLoader.Error.malformedToolsVersion(specifier: String, currentToolsVersion: ToolsVersion)` from deprecation to removal in the changelog following 2e753b8. Removed notice of deprecation of `ToolsVersionLoader.split(_ bytes: ByteString) -> (versionSpecifier: String?, rest: [UInt8])`. * [NFC][Gardening] use 4 spaces instead of a tab for indentation * [Gardening] use shortcut reference links in the change log, like how apple/swift does it * improve diagnostics of manifest files Previous silent fallback to Swift 3.1 is replaced with errors visible to the user. Manifest and Swift tools version specification malformation errors are now more fine-grained. Each error points to exactly where the misspelling and malformation is and suggests a correction. Regular expressions are replaced by working with substrings directly, wherever possible. `regex` is returned to its main-branch form, because it's not used anywhere but in the original (now deprecated) `ToolsVersionLoader.split(_:)`. `ToolsVersionLoaderTests.assertFailute(_:_:_:file:line:)` is removed because new test cases introduced in this commit already test at a higher resolution than `assertFailure` can provide. * use strings instead of substrings in associated values for `ToolsVersionLoader.Error` cases `String` is a currency type in Swift. `Substring` is not. * restructure `ToolsVersionLoader.Error.ToolsVersionSpecificationMalformationLocation` to extract (or distribute?) common errors from different locations * recognise any number of newline characters (U+000A) to be backward compatible with Swift ≤ 5.3 * [NFC] correct documentation for the deprecated `split(_:)` and explanation for not using `Collection.split(separator:maxSplits:omittingEmptySubsequences:)` following commit d8887d0 * use strings instead of substrings in associated values for `ToolsVersionLoader.Error.BackwardIncompatibilityPre5_3_1` cases `String` is a currency type in Swift; `Substring` is not. * allow leading whitespace (instead of just line terminators) and spacing after the label A lot of documentation is improved as well. * [NFC] update FIXME for `ToolsVersionLoader.split(_: ByteString)` * [NFC] remove FIXME on error messages * fix mostly typo errors in `ToolsVersionLoaderTests` * [NFC] fix a typo: "whatespace" → "whitespace" * [NFC] improve documentation on the order of diagnosis of a manifest's correctness - Replaced some references to the Swift tools version specification with reference to the manifest, to make the documentation more correct. For example, leading whitespace is not part of the specification, but part of the manifest. - Expanded the order with sub-orders, because understanding the order of diagnosis and errors thrown requires documentation on the most detailed ordering. - Added explanations/rationales for why the ordering is designed this way. - Remove the FIXME callout. The rationale makes it clear the current ordering is likely the optimal one. * [NFC] fix typo: "Swift version specification" → "Swift tools version specification" * [NFC] remove FIXME label for the UTF-8-related source breakage The breakage is the intention, not an error. * [NFC] replace the FIXME callout to label's case-insensitivity with an explanation * improve handling of unforeseen consequences - `fatalError`s are replaced by `ToolsVersionLoader.Error` cases, so SwiftPM will emit diagnosis instead of simply crashing when it encounters an unaccounted-for error in the manifest. - The error messages are reworded to be more detailed and more user-friendly, now with potential suggestions for recovery and bug-reporting included. * reword some error messages: "lowest supported version by" → "lowest Swift version supported by" * [NFC] draw attention to the mismatch between `regex`'s behaviour and its documentation by adding a `Bug` callout that describes the mismatch. * fix typo: "ToolsVersion.currentVersion" → "ToolsVersion.currentToolsVersion" (cherry picked from commit 98dcd5f) * add missing parameter in `Error.backwardIncompatiblePre5_3_1(.unidentified, specifiedVersion: version)` and change the error message to suggest the specified version instead of the current version (cherry picked from commit 336cad2) * [NFC] fix typo: "valid" → "invalid" (cherry picked from commit d739dd2) * adapt `writeToolsVersion(at:version:fs:)` to using the new `ToolsVersionLoader.split(_:)` This allows the old `ToolsVersionLoader.split(_:)` and `regex` to be fully removed instead of just deprecated. In implementing this change, the logic of the new `ToolsVersionLoader.split(_:)` is updated for the following 2 purposes: 1. Primarily to partially match the old `split(_:)`'s behaviour in deciding if the label of the Swift tools version specification is malformed. 2. Secondarily to provide a special path of diagnosis for when the label is prefixed with "swift-tools-version:" (case-insensitive). The comments left in the source provides much better details. (cherry picked from commit 857655a) * [NFC] explain why a "missing version specifier" error might be a misspelt label or version specifier in truth. * replace "newline characters" with "line feeds" to disambiguate * [NFC] minor re-wording of a documentation comment line * replace reference to Swift 5.3 and 5.3.1 with Swift Next Some places, such as test cases and comments, must have hard-coded version numbers. For them, some of the version numbers are changed from assuming 5.3.1 to assuming 5.4 as the Swift version where pull request #2937 lands. Many version numbers in the tests are prepended with "999" to make them work. 2 FIXME comments are added to the beginning of `ToolsVersionLoader.swift` and `ToolsVersionLoaderTests.swift` explaining what to look out for, and what has to be done, when replacing Swift Next with a concurrent version. * [NFC] use space for indentation Co-authored-by: Anders Bertelrud <anders@apple.com>
- Loading branch information
1 parent
4569980
commit 38f996c