[PackageLoading] Improve flexibility in formatting Package manifests and correctness in parsing Swift tools version specifications

[WowbaggersLiquidLunch]

ORIGINAL DESCRIPTION

SR-13566

This pull request changes the regex pattern on line 181 of swift-package-manager/Sources/PackageLoading/ToolsVersionLoader.swift to allow any combination of horizontal whitespace characters between "//" and "swift-tools-version" in the swift tools version specification in Package.swift

The old regex pattern is ^// swift-tools-version:(.*?)(?:;.*|$)

The new regex pattern is ^//\h*?swift-tools-version:(.*?)(?:;.*|$)

---

NEW DESCRIPTION (2020-11-04)

This PR proposes the following changes to the parsing of the Package manifest and its Swift tools version specification, starting from Swift Next (assumed to be Swift 5.4):

• Allow leading (any combination of all) whitespace characters in the Package manifest, instead of just line feed characters (U+000A).

  Rationale: This allows SwiftPM to skip all initial whitespace-only lines (if any) when looking for the Swift tools version specification, eliminating a common source of bug for Swift packages. Additionally, this allows SwiftPM to recognise all Unicode line terminators, because all of them are whitespace characters.

• Allow any combination of all horizontal whitespace characters¹ immediately after the comment marker (//) in the Swift tools version specification, instead of just a single space (U+0020).

  Rationale: Sometimes a user may prefer to follow the comment marker with a character tabulation (U+0009) or 2 space characters, or some other horizontal whitespace characters, in order to line up developer (//) and documentation (///) comments throughout the source files. For example:

  //  This is a demonstration of how comments may be lined up.
  /// A struct that represents nothing.
  struct Foo {}

• Allow any combination of all horizontal whitespace characters immediately before the version specifier in the Swift tools version specification.

  Rationale: This mirrors the most common style of type declaration in Swift sources. For example,

  let foo: Foo

• Recognise all Unicode line terminators instead of just line feed (U+000A) in the Package manifest up to the line terminator that immediately follows the Swift tools version specification line².

  Rationale: With Swift becoming supported on more and more platforms (many of them Unix-unlike or POSIX-incompliant), SwiftPM will see more and more Package manifests that use line terminators other than line feed. Further, even though many systems are not yet supported, Swift source files can still be edited on them, and many IDEs/editors silently changes the line terminators to the platforms' native ones when saving the edited files. SwiftPM needs to recognise all possible line terminators³ before it becomes a problem

• End the silent fallback to using Swift 3.1 as the lowest supported version. This used to happen when the Swift tools version specification is malformed AND SwiftPM couldn't find either swift-tool or tool-version in the specification.

  Rationale: It is actively harmful to hide this behaviour from the user. It is also actively harmful to decide on a Swift version without user consent. With source breaks from past Swift releases accumulated, this could result in unexpected behaviour in Swift packages. This behaviour was originally intended to peacefully accept Swift packages created before the Swift tools version specification was required. However, this purpose has become outdated.

Additional user-facing changes as the result of implementing the changes above:

• If the manifest has a formatting error, SwiftPM now identifies more accurately where the error is, and provides to the user a more detailed description/explanation and a suggestion tailored for each error.

• SwiftPM now throws an error if the manifest is not properly encoded in UTF-8.

Additional SwiftPM client-facing changes as the result of implementing the changes above:

• ToolsVersionLoader.Error.malformedToolsVersion(specifier: String, currentToolsVersion: ToolsVersion) is replaced by more granular error cases.

• ToolsVersionLoader.split(_ bytes: ByteString) -> (versionSpecifier: String?, rest: [UInt8]) and ToolsVersionLoader.regex are replaced by the Substring-oriented ToolsVersionLoader.split(_ manifest: String) -> ManifestComponents.

¹ Whitespace characters consists of horizontal and vertical whitespace characters. No whitespace character can be both horizontal and vertical. All vertical whitespace characters are line terminators, and vice versa. In the PR's implementation, horizontal whitespace characters are not identified directly, but through the process of elimination: All line terminators are "stripped" from the manifest first, then the horizontal whitespace characters are identified by being whitespace characters.

² Everything after the Swift tools version specification, such as the initilisation of the Package instance, is handled by the Swift compiler's lexer. It's beyond the power of ToolsVersionLoader and SwiftPM's manifest-loading in general.

³ Only Unicode line terminators are recognised, so line terminators in some other encodings such as EBCDIC are not recognised. This is fine because if a Package manifest isn't encoded in UTF-8, then it's already entirely unreadable by SwiftPM.

[WowbaggersLiquidLunch]

FIXME: Line feed and carriage return cause tests failures. I need help with fixing them.

I tried replacing the \h pattern with [\h--\v--\n--\r] and [\u0009\u0020\u00A0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\u3000], with no effect:

// This doesn't fix test failures:
static let regex = try! NSRegularExpression(
    pattern: "^//[\\h--\\v--\\n--\\r]*?swift-tools-version:(.*?)(?:;.*|$)",
    options: [.caseInsensitive])

// This doesn't fix test failures either:
static let regex = try! NSRegularExpression(
    pattern: "^//\(allowedHorizontalWhitespaceCharactersGroup)*?swift-tools-version:(.*?)(?:;.*|$)",
    options: [.caseInsensitive])

private static let allowedHorizontalWhitespaceCharactersGroup = "[\\u0009\\u0020\\u00A0\\u1680\\u2000\\u2001\\u2002\\u2003\\u2004\\u2005\\u2006\\u2007\\u2008\\u2009\\u200A\\u202F\\u205F\\u3000]"

I suspect the test failures are related to the implementation of assertFailure(_:_:file:line:) and load(_:,_:) in ToolsVersionLoaderTests.swift, but I don't fully understand how they work.

UPDATE: Carriage return does not cause test failures, only line feed does.

[abertelrud]

Thanks for the PR, @WowbaggersLiquidLunch. I will take a look and see if I can find out the issue with the regex.

[tomerd]

hi @WowbaggersLiquidLunch thanks for the contribution. Could you share some background as to the motivation for this change? Is it to match the implementation to the documentation or has the single space restriction caused real issues for you?

[WowbaggersLiquidLunch]

It started as a personal preference where I always follow slashes with a tab character, so that comments in both // and /// line up vertically. Due to this styling preference, I always wished that SPM would allow more flexible spacing between "//" and "swift-tools-version".

A few days ago, I stumbled across the documentation of regex, and thought maybe the spacing was designed to be flexible in the beginning, but overlooked in the implementation. So I decided to match the implementation with the documentation (but also change the documentation to reject vertical whitespace characters, because they don't make sense), and file a bug report and open this PR.

The only issue I have with the single space restriction is just a minor annoyance, that all comments and documentation in my source files line up, except for the // swift-tools-version: line.

[abertelrud]

Thanks for providing some background; it's certainly reasonable to want some flexibility here, all the more so since the comments already state that that was the intent.

Looking at the regular expression again: I'm not actually familiar with \h+ — did you try just doing \s+, which would match any whitespace? This regex should not be using the multiline option anyway (since it's by definition just the one line), so that should have the effect of just matching horizontal whitespace. I haven't yet tried it with the unit tests, but will do so tomorrow.

I think this is a good change and my only concerns would be:

• if there is any external code (such as in IDEs or other clients of libSwiftPM) that look at the first line as part of determining whether a particular file called Package.swift is indeed a package manifest, and that would now fail to detect it as such
• because SwiftPM versions 5.3 and earlier would not recognize anything other than just one space, it would now be possible to create a valid manifest in the next version of SwiftPM that's invalid in earlier versions

I think that the first case is probably a small risk only, and libSwiftPM should provide a function that could look at the start of a file to see if that file looks like a package manifest. I think that any code that looks at this line would be updated fairly quickly.

To deal with the second concern, there should probably be an error if the tools version is set to 5.3 or earlier and the space after the comment is not exactly one whitespace character (ASCII 32). The error should explain that if supporting tools version 5.3 or earlier, a single space needs to be used so that they can properly parse the tools version.

[WowbaggersLiquidLunch]

Looking at the regular expression again: I'm not actually familiar with \h+ — did you try just doing \s+, which would match any whitespace? This regex should not be using the multiline option anyway (since it's by definition just the one line), so that should have the effect of just matching horizontal whitespace.

I used * (actually *?) instead of + in the regex, so that //swift-tools-version would also be allowed.

\h is in ICU regex metacharacters, so NSRegularExpression should support it.

I just tried replacing \h with \s, and changed the tests from

assertFailure("//\nswift-tools-version:5.3\n", "//\nswift-tools-version:5.3")
assertFailure("// \nswift-tools-version:5.3\n", "// \nswift-tools-version:5.3")
assertFailure("//\n swift-tools-version:5.3\n", "//\n swift-tools-version:5.3")
assertFailure("//\rswift-tools-version:5.3\n", "//\rswift-tools-version:5.3")
assertFailure("// \rswift-tools-version:5.3\n", "// \rswift-tools-version:5.3")
assertFailure("//\r swift-tools-version:5.3\n", "//\r swift-tools-version:5.3")
assertFailure("//\r\nswift-tools-version:5.3\n", "//\r\nswift-tools-version:5.3")
assertFailure("//\n\rswift-tools-version:5.3\n", "//\n\rswift-tools-version:5.3")
assertFailure("//\u{B}swift-tools-version:5.3\n", "//\u{B}swift-tools-version:5.3")
assertFailure("//\u{2028}swift-tools-version:5.3\n", "//\u{2028}swift-tools-version:5.3")
assertFailure("//\u{2029}swift-tools-version:5.3\n", "//\u{2029}swift-tools-version:5.3")

to

let validVersions = [
    /* other valid versions omitted here */
    "//\nswift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "// \nswift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\n swift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\rswift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "// \rswift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\r swift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\r\nswift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\n\rswift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\u{B}swift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\u{2028}swift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
    "//\u{2029}swift-tools-version:5.3\n": (5, 3, 0, "5.3.0"),
]

for (version, result) in validVersions {
    try load(ByteString(encodingAsUTF8: version)) { toolsVersion in
        XCTAssertEqual(toolsVersion.major, result.0)
        XCTAssertEqual(toolsVersion.minor, result.1)
        XCTAssertEqual(toolsVersion.patch, result.2)
        XCTAssertEqual(toolsVersion.description, result.3)
    }
}

\n causes test failures again. These are the errors:

They are very similar to the errors from when the regex uses \h:

In both cases, it seems like \n is making the assertions compare the parsed version with 3.1.0.

---

I'm not familiar with how SPM finds the "swift-tools-version" line, so I'm probably wrong: If vertical whitespace characters are allowed, would there be a risk of misrecognising the actual "swift-tools-version" line?

For example, would SPM mistakenly use this for the swift version:

let package = Package(
    name: """
//
swift-tools-version:5.3
""",
    /* other parameters omitted here */
)

[WowbaggersLiquidLunch]

because SwiftPM versions 5.3 and earlier would not recognize anything other than just one space, it would now be possible to create a valid manifest in the next version of SwiftPM that's invalid in earlier versions

there should probably be an error if the tools version is set to 5.3 or earlier and the space after the comment is not exactly one whitespace character (ASCII 32). The error should explain that if supporting tools version 5.3 or earlier, a single space needs to be used so that they can properly parse the tools version.

This is a very good point. I'll add a comparison between the user-specified version and 5.3, and present an error if the user-specified one is lower or equal.

The tests will need to be changed too. What method should I use for asserting that SPM presents an error? I'm not sure if XCTAssertError is the right one here.

[WowbaggersLiquidLunch]

I think I figured out why \n is so troublesome: ToolsVersionLoader looks for the first line by splitting the entire Package.swift by \n. When regex matching the first line fails, the tools version defaults to ToolsVersion.v3, which is defined to represent Swift 3.1.0.

This discovery raises another question from me: How does SPM deal with other systems that use characters other than \n for newline?

regex already has $ in it, so maybe the splitting isn't necessary?

[abertelrud]

Thanks for all the additional information, and sorry for the delay in replying. I didn't realize that the existing code looked specifically for \n. That would explain it, and I agree that the splitting should not be needed here. There is also a comment that the code shouldn't read the whole file, though in practice I don't think this makes a difference, due to the relatively small sizes of manifests and how I/O reads whole blocks at a time anyway.

How does SPM deal with other systems that use characters other than \n for newline?

Rather poorly, I suspect. It doesn't currently run on any platforms in which \n isn't special — even Windows treats it as part of a line ending. Classic MacOS used only \r, but isn't interesting anymore. So I think in theory it would be good to be more neutral, and support anything in the .newlines character set, in practice I don't think it has caused any problems on modern systems. But it would be good to fix the places in the code that do rely on it, just to make it cleaner.

[MaxDesiatov]

@swift-ci please smoke test

[WowbaggersLiquidLunch]

I just pushed a new commit that makes SPM to throw an error if anything but a single U+0020 is used as the spacing after //.

While reading through ToolsVersionLoader, I got a lot more questions (might be out of order, and overlapping each other):

1. Somewhat in line with this pull request, should the spacing after swift-tools-version: be relaxed as well? For example, make // swift-tools-version: 5.4 valid. Maybe even relaxing the exact spelling of swift-tools-version to allow swift tools version and Swift tools version too? IMO a more flexible specification format makes SPM smarter and avoids unexpected default to 3.1. For example, if a user writes // swift tools version:5.4, SPM currently does not throw an error, so the user would expect that the lowest supported version is 5.4, but SPM silently chooses 3.1 instead.

2. Maybe the regex pattern can be improved to have many more capture groups, with which SPM can provide more granular diagnosis of malformed tools version specifications, and avoid defaulting the version to 3.1 altogether? I left a TODO comment where the current misspelling diagnosis is.

3. The regex pattern is suffixed with (.*?)(?:;.*|$). What are (.*?) and ; for? I haven't been able to construct a string where the (.*?) part matches anything, and I can't find any documentation of ; on ICU.
   EDIT: I mistook (?:;.*|$) as a capturing group.

[WowbaggersLiquidLunch]

How does SPM deal with other systems that use characters other than \n for newline?

Rather poorly, I suspect. It doesn't currently run on any platforms in which \n isn't special — even Windows treats it as part of a line ending. Classic MacOS used only \r, but isn't interesting anymore. So I think in theory it would be good to be more neutral, and support anything in the .newlines character set, in practice I don't think it has caused any problems on modern systems. But it would be good to fix the places in the code that do rely on it, just to make it cleaner.

Although SPM currently doesn't run on any platform in which \n isn't special, manifest files can still be edited on them. For example, if a manifest file is edited on a computer running Classic MacOS like this:

// swift-tools-version:5.3
42

where between "5.3" and "42" is a single carriage return, and "42" is left there as a mistake/typo.

SPM still will interpret this correctly as 5.3, instead of 5.342, but the splitting-manifest-by-newline part will just return a longer "first line". This probably doesn't matter much, but maybe can be improved by a future pull request?

[WowbaggersLiquidLunch]

One more question (very likely not my last): Should I add an entry to CHANGELOG.md as part of pull request? I feel that I should, but at the same time the document hasn't been updated for 2 years, so maybe I shouldn't?

[MaxDesiatov]

@swift-ci please smoke test

[abertelrud]

Thanks again for your work on this, and sorry again for the delay in replying.

I completely see your point about how editing might be done on different platforms than those on which SwiftPM runs. My comment about MacOS Classic not being interesting anymore was more of a pragmatic observation about that particular platform, but I do agree in general, and the parsing logic should really consider anything up to the first member of CharacterSet.newlines (i.e. U+000A ~ U+000D, U+0085, U+2028, and U+2029).

I think that it makes sense to look for alternate spellings, but as a diagnostic rather than something that gets accepted as a given value. I don't think there's a lot of value in allowing a great deal of flexibility, especially since older SwiftPM's couldn't parse them, but as you point out there is definitely value in diagnosing improper spellings rather than just silently falling back on 3.1. So diagnosing malformed expressions would be great, even if it's just a warning saying that it's falling back on 3.1 (but ideally also "did you mean swift-tools-version: x.y?" when the invalid text looks "close to" the valid syntax, which could be really helpful).

Lastly, about CHANGLOG.md: yes, this file should definitely be updated. As you point out it's seriously out-of-date, so in addition to updating that file for new PRs, the intent is for us to also go back and fill in old information to bring it up-to-date. It will take some time to catch up, but having it be start to be updated again as part of on-going main-branch work is definitely where we want to get to.

Thanks!

[abertelrud]

BTW, the two tests that failed seem to be in the compiler itself and seem unrelated to this change.

[WowbaggersLiquidLunch]

Thanks for the answers.

I want to work on making the parsing logic recognise all line terminating characters, and on improving the diagnostics for malformed "swift-tools-version". Should I include them in this PR, or should I open new PRs for them?

I don't think there's a lot of value in allowing a great deal of flexibility, especially since older SwiftPM's couldn't parse them

I'm not really convinced by this argument, because old versions of SwiftPM's couldn't parse horizontal whitespace characters other than a single space, either. Do you mean there are different levels to "couldn't parse", where some are more disruptive than others?

[abertelrud]

If the code is separate, or if one change cleanly builds on another, then separate PRs would make sense just to make them easier to review and test. But if the code would have to be written one way for one PR and then immediately changed for another, the it makes more sense to put them in the same PR. I could see handling of arbitrary newlines being in this PR, and then maybe diagnostics for "slightly malformed" tools version declarations be a separate PR.

You make a good point about the single space — I guess either way, that means that older versions of SwiftPM will not recognize the tools version declaration at all, meaning that (I think) they'll fall back to SwiftPM 3.1 since that was when this comment format was introduce (meaning: if a recognized comment isn't found, the manifest is assumed to be so old that it predates the introduction of the swift tools version comment). What I was trying to express was that allowing for more variations seems to just invite more ways to express this string that old versions can't read.

[WowbaggersLiquidLunch]

But if the code would have to be written one way for one PR and then immediately changed for another, the it makes more sense to put them in the same PR.

and then maybe diagnostics for "slightly malformed" tools version declarations be a separate PR.

The diagnostics improvement I'm aiming for is the ideal version of this:

So diagnosing malformed expressions would be great, even if it's just a warning saying that it's falling back on 3.1 (but ideally also "did you mean swift-tools-version: x.y?" when the invalid text looks "close to" the valid syntax, which could be really helpful).

I think the change involves additional capturing groups in the regex pattern, which overlaps with the regex change already in this PR. It will change test cases already changed by this PR as well. So maybe the diagnostics improvement need to be put in this PR, along with handling of arbitrary newlines?

---

For emitting warnings, should I use DiagnosticsEngine? I read about it in PR #2933.

[WowbaggersLiquidLunch]

I think I should give my motivation for wanting to allow spacing between : and the version number, like this:

make // swift-tools-version: 5.4 valid

Mostly it's because in Swift source code, colons are often followed by a space, e.g.

let foo: Int
let foobar = [foo1: bar1, foo2: bar2]

So a user might use (and/or prefer) a space following : in the specification out of habit and visual consistency.

Additionally, I think since the PR is already changing the spacing in one place, spacing change in a second place maybe could just tag along.

What I was trying to express was that allowing for more variations seems to just invite more ways to express this string that old versions can't read.

If SPM (with the diagnostics improvement) is able to catch the backwards-incompatibility while understanding the version the user intends to specify, and suggest the user a correction, would it still be a problem?

[WowbaggersLiquidLunch]

I intend to introduce a possibly source-breaking change, and I need some guidance for it.

The motivation come in 2 parts. The first part is a problem with the current ToolsVersionLoader.split(_:).

It uses Collection.split(separator:maxSplits:omittingEmptySubsequences:) to separate the byte array by the first \n. It has an unaddressed edge case: \n at the beginning of the file. For example, currently, this

// swift-tools-version:5.3

(with a trailing \n) is parsed the same as

// swift-tools-version:5.3

(with a leading \n).

The second part is that in order to recognise all Unicode line terminators, ToolsVersionLoader's logic needs to change from working with ByteString (a wrapper type over [UInt8]) to String. This is doubly inefficient, but almost necessary, because some of the line terminators such as U+2028 are more than 1 byte.

Now that I'm working with String, I want to take advantage of its instance methods and resolve split's ambiguity by replacing this

let splitted = bytes.contents.split(separator: UInt8(ascii: "\n"), maxSplits: 1, omittingEmptySubsequences: false)

with something like this

/// The position of the first Unicode line terminator in (or, in lieu thereof, the end position of) the manifest.
let indexOfFirstNewline = manifestContents.firstIndex(where: \.isNewline) ?? manifestContents.endIndex
/// The first line of the manifest, excluding the line terminator, if any.
let firstLineOfManifest = manifestContents.prefix(upTo: indexOfFirstNewline)
/// The manifest excluding the first line.
let manifestContentsSansFirstLine = manifestContents.suffix(from: indexOfFirstNewline)

One side effect of this change is that all package manifests that start with a single line terminator will result in an error, regardless of the second line's validity as the tools version specification.

There are 3 possible solutions that I can think of:

1. Disallow empty first lines. This is source breaking.

2. Strip the first line terminator before splitting the manifest. This is backward incompatible, because a single leading \r will work in future SPM versions, but result in error in old versions, because old versions don't recognise line terminators other than \n.

3. Strip all leading line terminators before splitting the manifest, but throw an error if the specified version ≤ 5.3 and more than 1 line terminator is stripped. This has the same backward incompatibility like solution 2. However, it also removes the requirement of starting the specification comment on the first line, which is a common source of typo bugs for things like shebangs.

For both solutions 2 and 3, SPM can check if the first leading line terminator is \n.If it's not \n and the specified version ≤ 5.3, throw an error and inform the user. However, this could potentially confuse some users if they see that leading empty lines in some manifest but not others, without knowing that they are different line terminators.

I'm leaning towards solution 3, with the additional \n check included to maintain backward compatibility. I want to seek some opinions from the maintainers first, before committing it.

EDIT: This is partially wrong. Using Collection.split(separator:maxSplits:omittingEmptySubsequences:) allows for any number of \ns, not just 0 or 1.

[WowbaggersLiquidLunch]

My understanding is that it's not source-breaking, but rather that this change makes the formatting rules more lenient so that it's be possible for a package author to make changes that old versions of SwiftPM can't read. But since they get a warning if they specify an old tools version and change the formatting, I don't think that anyone can break their package by accident.

If my understanding is wrong, then we'd need to discuss it, because changes to make old packages no longer work would not be something we could merge. The term "source-breaking" when used with SwiftPM would mean that old packages would no longer work in a new SwiftPM, and that isn't something we could merge.

I think the removal of the silent fallback to Swift 3.1 is possibly source breaking. I'm not very confident in my assessment, though. As you pointed in an earlier review, SwiftPM already rejects Swift < 4:

Removing the fallback makes sense to me. Falling back to 3.1 made sense when there were packages that were so old that they didn't have a tools version comment at all. But since SwiftPM 5.0 (commit 3fedbbd), it seems that SwiftPM anyway emits error: package at '/path/to/package' is using Swift tools version 3.1.0 which is no longer supported. So this isn't really the compatibility problem that I'd thought, and making that error instead point out the actual error (such as a missing tools version designation) makes a lot of sense.

The only thing I'm not sure about is whether this "Swift ≥ 4" restriction is for the root manifest only, or for all levels of manifests. If it's for root manifest only, then the removal of the fallback is still source-breaking.

However, in either case, the silent fallback seems harmful to me, so maybe this souce-break is justified, if it is a break?

[WowbaggersLiquidLunch]

Just in case they're lost in this long thread, there are 3 unresolved reviews from much earlier:

#2937 (comment)
#2937 (comment)
#2937 (comment)

The code they refer to are now outdated, but I think they're worth a second look and some discussion.

[abertelrud]

I think the removal of the silent fallback to Swift 3.1 is possibly source breaking. I'm not very confident in my assessment, though. As you pointed in an earlier review, SwiftPM already rejects Swift < 4:

Right, since SwiftPM already rejects older packages, this isn't source-breaking (it just shows a different error message). So I don't see an issue here.

The only thing I'm not sure about is whether this "Swift ≥ 4" restriction is for the root manifest only, or for all levels of manifests. If it's for root manifest only, then the removal of the fallback is still source-breaking.

No, this seems to be for all packages, so I have no concerns about that.

[abertelrud]

Thank you for summarizing the remaining issues — they have indeed become lost in all the discussion. Your thoroughness is really appreciated, and I'm keen to see this merged sooner rather than later, so we should try to see what can be left to follow-up PRs.

Just in case they're lost in this long thread, there are 3 unresolved reviews from much earlier:

Thanks, I have commented on all three. I think that the commit I referenced earlier extended the check for < 4 to all packages, so I don't think there is a source-breaking change here.

It seems to me that this PR is in great shape, and I would like to see how we can get this merged. While there can always be improvements, those could be done as follow-on PRs if you feel that the major outstanding issues have been resolved.

With regard to the versioning, I think that it is unlikely that this will go into a 5.3.x toolchain. I think we've been using 999.0 as the generic "future toolchain" (usually through its symbol, vNext), which will then get replaced with the next actual number once there is one. I don't think there is any word yet on the actual version number of the next release.

Thanks again for all your work on this! I'd suggest wrapping this up so it can be merged and then doing anything further in follow-on PRs. As far as I know, the only thing remaining in this PR would be to change to vNext. Thanks!

[abertelrud]

@swift-ci please smoke test

[WowbaggersLiquidLunch]

With regard to the versioning, I think that it is unlikely that this will go into a 5.3.x toolchain. I think we've been using 999.0 as the generic "future toolchain" (usually through its symbol, vNext), which will then get replaced with the next actual number once there is one. I don't think there is any word yet on the actual version number of the next release.

As far as I know, the only thing remaining in this PR would be to change to vNext.

I replaced most references of Swift 5.3 and 5.3.1 with some form of Swift Next in 70bc2e2. The rest are the ones that have to be hard-coded, such those as tests and comments; they are changed as well but differently: First, all hard-coded references to 5.3 and 5.3.1 are replaced with references to 5.4. (assumed to be 5.4 because of swiftlang/swift-evolution#1199 and a short discussion it spawned on the Swift Forums that I can't find anymore; UPDATE: Swift Next confirmed to be 5.4). These include intentionally misspelt version numbers and those for testing/drawing attention to boundary conditions, in both comments and test cases. Then, all hard-coded version numbers for testing valid Swift tools version specifications under the new relaxed rule are prefixed with "999" to let the tests pass. Those for testing valid specifications under the old rule are not changed; all those for invalid specifications are not changed either. I added 2 FIXMEs, each at the top of ToolsVersoinLoader.swift and ToolsVersoinLoaderTests.swift to help explain what to look out for and what to do (e.g. removing "999" prefixes) when someone replaces Swift Next to an actual version in the future.

It seems to me that this PR is in great shape, and I would like to see how we can get this merged. While there can always be improvements, those could be done as follow-on PRs if you feel that the major outstanding issues have been resolved.

Yep, I think this PR should be ready for merging now. There are still some FIXMEs, but they are just notes for possible optimisation and refactoring on minor parts of the logic. I agree that they should be left for future PRs, instead of holding up this one. One of them would require a new language feature, so it's definitely not possible to resolve right now.

---

Thank you for the continuous review! It really helped this PR grow from a regex correction to a more comprehensive and better thought-out improvement.

[abertelrud]

Great, thank you for the work on this! I'll take one last look while testing, but then this sounds ready to be merged!

[abertelrud]

@swift-ci please smoke test

[abertelrud]

@swift-ci please smoke test

[abertelrud]

The unit test failures look unrelated:

[595/595] Testing WorkspaceTests.WorkspaceTests/testSimpleAPI

**** FAILURE EXECUTING SUBPROCESS ****
output: Fetching https://github.com/IBM-Swift/BlueCryptor.git
Fetching https://github.com/lorentey/BigInt.git
Fetching https://github.com/attaswift/SipHash

stderr: error: because BlueCryptor <1.0.12 contains incompatible tools version and root depends on BlueCryptor 0.8.16..<1.0.0, version solving failed.

Not really sure what's going on there — it seems unexpected that there would be actual network access to GitHub during the unit tests, especially in a way that could fail if remote packages change.

[WowbaggersLiquidLunch]

I have no idea of what went wrong in the tests. SwiftPM doesn't seem to depend on any of those packages fetched. For what it's worth, I locally rebased a copy of my branch onto the main branch, and all tests passed (on macOS 10.15.7, Swift 5.3.1).

[WowbaggersLiquidLunch]

Could someone summon the CI bot again? I don't have commit access.

[tomerd]

@swift-ci please smoke test

[tomerd]

@swift-ci please smoke test

[abertelrud]

Looks like everything passed this time. @WowbaggersLiquidLunch, is this ready to merge as far as you are concerned? Thanks again for all your work on this!

[WowbaggersLiquidLunch]

Yes, I think it's ready to merge.

[abertelrud]

Great, thanks!

[WowbaggersLiquidLunch]

Yay!