Skip to content

Revise #file changes from SE-0274 #32700

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.

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

Merged
merged 12 commits into from
Jul 17, 2020

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Jul 3, 2020

This PR implements the changes discussed in the follow-up thread to SE-0274 (now SE-0285). Specifically, it:

  • Fixes several tests which previously violated the same-filename rule, but got away with it because they were directly calling the frontend. Also tightens up a test previously introduced by [NFC] GenericSignatureImpl: Spell conformsToProtocol & getConformsTo in terms of requirements #31734 so that it merely ensures filenames don't end up in SIL debug info, rather than in the SIL file at all.

  • Refactors a lot of magic identifier literal implementation to be driven by a new MagicIdentifierKinds.def file. MagicIdentifierLiteralExpr::Kind and some cases of DefaultArgumentKind are now driven by this table, as is much logic that simply propagates these kinds without changing them; it also specifies tokens related to magic identifier literals, so parsing and code completion now work from this file too. (It also fixes a bug in #filePath's code completion support, which was previously disabled and untested.)

  • Adds a new #fileID literal which always generates the concise file ID string.

  • Splits MagicIdentifierLiteralExpr::Kind::File (and similar cases elsewhere) into FilePathSpelledAsFile and FileIDSpelledAsFile. When -enable-experimental-concise-pound-file is false, the parser generates the former, producing current Swift 5 behavior; when it is true, the parser generates the latter, producing future "Swift 6" behavior. The rest of the compiler (including module interface emission, by preserving the compiler flag) propagates this difference so that Swift 5 declarations can be used from Swift 6 code and vice-versa without confusion. This design causes more code changes, but is less risky than alternatives.

  • Introduces a concept of "compatibility" to the diag::default_magic_identifier_mismatch diagnostic. If two magic identifiers are compatible, the diagnostic is suppressed. Compatibility is commutative. Every magic identifier is compatible with itself; Swift 6 #file is compatible with #fileID, while Swift 5 #file is compatible with #fileID, #filePath, and Swift 6 #file.

  • Makes both compiler-generated traps and standard library diagnostics use file IDs instead of file paths. In the standard library, this is temporarily being done by building with -enable-experimental-concise-pound-file, which is supported by compilers since Shorten #file and add #filePath (behind an experimental flag) #25656 in late December, rather than by actually writing #fileID, which is introduced in this change. This is merely to give updated tools time to spread; I intend to remove the flag and write #fileID in the future.

Fixes rdar://65113871 and SR-12936.

@beccadax beccadax added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Jul 3, 2020
@beccadax
Copy link
Contributor Author

beccadax commented Jul 3, 2020

@swift-ci test

@beccadax
Copy link
Contributor Author

beccadax commented Jul 3, 2020

@swift-ci test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Jul 3, 2020

Build failed
Swift Test OS X Platform
Git Sha - 89d7c4efedbe90e5e19bc83dd25ea6d02bd9e74d

@beccadax beccadax force-pushed the magical-and-evolutionary branch 2 times, most recently from c9183b4 to 47d04cc Compare July 4, 2020 02:10
@beccadax
Copy link
Contributor Author

beccadax commented Jul 4, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Jul 4, 2020

Build failed
Swift Test OS X Platform
Git Sha - 89d7c4efedbe90e5e19bc83dd25ea6d02bd9e74d

@swift-ci
Copy link
Contributor

swift-ci commented Jul 4, 2020

Build failed
Swift Test Linux Platform
Git Sha - 89d7c4efedbe90e5e19bc83dd25ea6d02bd9e74d

@beccadax
Copy link
Contributor Author

beccadax commented Jul 5, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Jul 6, 2020

Build failed
Swift Test OS X Platform
Git Sha - 47d04cc1b907a8f1915cbfeb6946146703cf02f9

@beccadax
Copy link
Contributor Author

beccadax commented Jul 8, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Jul 8, 2020

Build failed
Swift Test OS X Platform
Git Sha - 47d04cc1b907a8f1915cbfeb6946146703cf02f9

@beccadax
Copy link
Contributor Author

beccadax commented Jul 9, 2020

@swift-ci test macOS platform

@swift-ci
Copy link
Contributor

swift-ci commented Jul 9, 2020

Build failed
Swift Test Linux Platform
Git Sha - 47d04cc1b907a8f1915cbfeb6946146703cf02f9

@swift-ci
Copy link
Contributor

swift-ci commented Jul 9, 2020

Build failed
Swift Test OS X Platform
Git Sha - 47d04cc1b907a8f1915cbfeb6946146703cf02f9

@beccadax
Copy link
Contributor Author

With swiftlang/swift-syntax#226

@swift-ci please smoke test

@beccadax
Copy link
Contributor Author

With swiftlang/swift-syntax#226

@swift-ci please smoke test

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

LGTM, there are a few clever tricks in there! My comments are from simple curiosity.

@beccadax beccadax force-pushed the magical-and-evolutionary branch from 47d04cc to c966e7b Compare July 10, 2020 22:02
@beccadax beccadax marked this pull request as ready for review July 10, 2020 22:07
@beccadax beccadax marked this pull request as draft July 10, 2020 22:07
@beccadax
Copy link
Contributor Author

beccadax commented Jul 10, 2020

(Just toggled "Draft" state to get GitHub to notice that my force-push had cleared the merge conflict. Sorry about the noise.)

@beccadax
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 47d04cc1b907a8f1915cbfeb6946146703cf02f9

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 47d04cc1b907a8f1915cbfeb6946146703cf02f9

@beccadax
Copy link
Contributor Author

With swiftlang/swift-syntax#226

@swift-ci please build toolchain Linux platform

FilePath,
FileIDSpelledAsFile,
FilePathSpelledAsFile,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we cannot use MagicIdentifierKinds.def here and related functions in Serialization?

Also, while you are here, could you align the order of this enum with swift::DefaultArgumentKind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Serialization usually defines its own copies of enums like this so that it's more obvious that you'll need to bump SWIFTMODULE_VERSION_MINOR.

I suppose I can change the order.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Thank you for the explanation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ordering change is in the latest force push (along with another bump of SWIFTMODULE_VERSION_MINOR).

Copy link
Member

Choose a reason for hiding this comment

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

Hm? I don't see the aligned ordering in the diff.

Copy link
Contributor Author

@beccadax beccadax Jul 16, 2020

Choose a reason for hiding this comment

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

The order used to be:

  FileID,
  FilePath,
  FileIDSpelledAsFile,
  FilePathSpelledAsFile,

It's now:

   FileID,
   FileIDSpelledAsFile,
   FilePath,
   FilePathSpelledAsFile,

There's no commit showing the change between the old and new ordering because I also had to rebase the pull request when ModuleFormat.h changed out from under me, but it has been corrected.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, sorry. I meant this whole enum. For example Inherited should be after Normal, File et al should be after StoredProperty. That's is not super important, but this is a good opportunity to do it.

beccadax added 12 commits July 13, 2020 14:05
Tweak this test to only look for debug info references to the filename, not *any* reference to the filename.
The meaning of EnableConcisePoundFile is going to shift slightly, so it makes sense to always include #filePath in completions. Also, @rintaro confirmed that this should be using KeywordKind::pound_filePath, not KeywordKind::pound_file.
Extracts the list of magic identifier literal kinds into a separate file and updates a lot of code to use macro metaprogramming instead of naming half a dozen cases manually. This is a complicated change, but it should be NFC.
Doing this NFC renaming first helps clarify the functional changes to come.
…and modify resolveFileIDConflicts() to diagnose any such violations instead of asserting.

Swift does not allow any two files in the same module to have the same filename, even if they are in different directories. However, this is enforced in the driver, so tests that invoke the frontend directly can violate it. Turns out that a couple of those snuck into the test suite at various points.

This commit updates those tests. It also causes the frontend to diagnose the duplicate filename error just as the driver would have, which should help us understand what happened more easily if this crops up again in the future.

NFC, since invoking the frontend directly is unsupported.
This temporarily breaks -enable-experimental-concise-pound-file.

fixup adding #fileID
Such as force unwraps, as! casts, etc.
In -swift-version 5 and earlier, #file will continue to be a synonym for #filePath; in a future -swift-version (“Swift 6 mode”), it will become a synonym for #fileID. #file in libraries will be interpreted according to the language mode the library was compiled in, not the language mode its client uses.

Implement this behavior, tied to a frontend flag instead of a language version. We do so by splitting the old `MagicIdentifierLiteralExprKind::File` into two separate cases, `FileIDSpelledAsFile` and `FilePathSpelledAsFile`, and propagating this distinction throughout the AST. This seems cleaner than looking up the setting for the module the declaration belongs to every time we see `File`.

This doesn’t handle module interfaces yet; we’ll take care of those in a separate commit.
Add -experimental-enable-concise-pound-file to the list of flags preserved by module interfaces, so that when we rebuild an interface, it comes out the same way as the original file.
This change makes:

* #file compatible with #fileID in “Swift 6 mode”
* #file compatible with #filePath and #fileID in Swift 5 mode
* #file in Swift 5 mode code compatible with #file in “Swift 6 mode” code

This should keep anyone from seeing XCTAssert-wrapping noise until they adopt “Swift 6 mode” (whatever version that ends up actually being).
We ultimately want to explicitly change standard library uses of #file to #fileID, but once we do, previous compilers won’t be able to build the standard library. So instead, we will temporarily build the standard library with -enable-experimental-concise-pound-file, which should have the same effect, but will back-deploy to compilers going back several months.
@beccadax beccadax force-pushed the magical-and-evolutionary branch from 60bac94 to 87d3d3e Compare July 13, 2020 22:33
@beccadax beccadax marked this pull request as ready for review July 13, 2020 23:52
@beccadax beccadax marked this pull request as draft July 13, 2020 23:52
@beccadax
Copy link
Contributor Author

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@beccadax beccadax marked this pull request as ready for review July 16, 2020 23:25
@beccadax
Copy link
Contributor Author

With swiftlang/swift-syntax#226

@swift-ci please test

@theblixguy theblixguy added swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process and removed swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review labels Jul 16, 2020
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 87d3d3e

@beccadax
Copy link
Contributor Author

With swiftlang/swift-syntax#226

@swift-ci please test Linux platform

@beccadax beccadax merged commit 4394e92 into swiftlang:master Jul 17, 2020
edymtt added a commit to edymtt/swift that referenced this pull request Feb 10, 2025
As per swiftlang#32700, this is only meant for `Swift.swiftinterface`.

Addresses rdar://143152066
edymtt added a commit to edymtt/swift that referenced this pull request Feb 10, 2025
As per swiftlang#32700, this is only meant for `Swift.swiftinterface`.

Addresses rdar://143152066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants