Skip to content
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

Add emit extension block symbols option to dump-symbol-graph and SymbolGraphOptions #5892

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

theMomax
Copy link
Contributor

Adds support for emitting extension block symbols to the dump-symbol-graph command and other SymbolGraphOptions.

Motivation:

Extension block symbols with the respective command line flags -emit-extension-block-symbols and -omit-extension-block-symbols were introduced in swiftlang/swift#59047 and swiftlang/swift#61637 as part of an ongoing effort to solve swiftlang/swift-docc#210. These flags should be exposed in higher-level commands such as swift package dump-symbol-graph as well as the package plugin API, so that users and package manager plugins such as apple/swift-docc-plugin can access them more easily.

Modifications:

This PR adds the flag-pair --emit-extension-block-symbols/--omit-extension-block-symbols to the DumpSymbolGraph command and exposes the emitExtensionBlocks option in PluginToHostMessage.SymbolGraphOptions. SymbolGraphExtract was also extended by an emitExtensionBlockSymbols option and its extractSymbolGraph function adds the respective compiler command line flag accordingly.

Result:

swift package dump-symbol-graph --emit-extension-block-symbols dumps symbol graph files that extension block symbol format. The default behavior does not change. The --omit-extension-block-symbols flag will be used to explicitly disable the feature once the default behavior has been changed to --emit-extension-block-symbols in the future.

Package Plugins can specify the emitExtensionBlocks property requesting symbol graphs, e.g.:

import PackagePlugin

// ...
try getSymbolGraph(for: target, options: PackageManager.SymbolGraphOptions(
            minimumAccessLevel: targetMinimumAccessLevel,
            includeSynthesized: true,
            includeSPI: false,
            emitExtensionBlocks: true
        ))

The default behavior for package plugins also does not change.

@neonichu
Copy link
Contributor

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Nov 14, 2022

looks good once CI is green


enum ExtensionBlockSymbolBehavior: String, EnumerableFlag {
case emitExtensionBlockSymbols
case omitExtensionBlockSymbols
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be Boolean instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK this is the only way to achieve a flag inversion with a custom name, i.e. with a prefix other than enable/disable or no. See the ArgumentParser docs here:

To create a flag with custom names for a Boolean value[...], define an enumeration that conforms to the EnumerableFlag protocol.

IMO the emit/omit prefixes describe the semantics best and I wouldn't want to have entirely different names for SwiftPM and compiler commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@tomerd tomerd self-assigned this Nov 14, 2022
@theMomax theMomax force-pushed the add-extension-block-symbols-flags branch from 1cc4ddd to 137b508 Compare December 4, 2022 18:30
@neonichu
Copy link
Contributor

neonichu commented Dec 5, 2022

@swift-ci please smoke test

@theMomax
Copy link
Contributor Author

theMomax commented Dec 6, 2022

@neonichu Test failures are expected until the CI uses a toolchain that was built after Dec. 2nd (and therefore includes this code adding the -omit-extension-block-symbols flag to swift-driver). This last run used one from Dec. 1st.

@theMomax
Copy link
Contributor Author

@neonichu Could you run tests again, please? They should pass now.

@franklinsch
Copy link

@swift-ci please smoke test

@theMomax
Copy link
Contributor Author

It seems like the self hosted macOS runner uses Swift 5.5, which doesn't know the flags this PR introduces. I'm a little confused as to why a contribution to the main branch (which is supposed to land in Swift 5.8) must be compatible with Swift 5.5. I couldn't find any CLI flags in SwiftPM that are hidden based on the Swift version, so I assumed each version of SwiftPM only needs to be compatible with the compiler/driver version of the respective toolchain it is part of. However, looking at the branches, it seems like you're currently maintaining compatibility down to Swift 5.3 from the main branch.

Should I therefore wrap all my changes in Swift 5.8 guards so that one can actually run the Swift 5.5 compiler's swift-symbolgraph-extract with the resulting version of SwiftPM? Or would it be better to just make a Swift 5.7 branch before this PR lands to avoid messing up the code-base?

@tomerd or @neonichu could you please tell me what is the best strategy for this?

@daniel-grumberg
Copy link

@swift-ci please test

@neonichu
Copy link
Contributor

@theMomax It is intentional that we are supporting older versions, we have an API to check which flags are available in the used compiler to guard availability of newer features like this. 5.5 is a little bit older than we’d like, but generally speaking we do want to retain the ability of using and developing SwiftPM with toolchains that aren’t the latest.

A compilation conditional is not sufficient to handle this since we do want people to mix a newer SwiftPM with older compilers at runtime as much as possible. You can look at the use of this API to see how conditionality could be done based on the availability of the flags you need rather than versions.

@theMomax
Copy link
Contributor Author

@neonichu thank you for the explanation! I pushed a fix that basically just drops the flag with a warning if it is not supported by the driver. Is that how it's supposed to be done? I couldn't find a precedent in the code where a user-provided command argument had to be dropped.

let extensionBlockSymbolsFlag = emitExtensionBlockSymbols ? "-emit-extension-block-symbols" : "-omit-extension-block-symbols"
if DriverSupport.checkSupportedFrontendFlags(flags: [extensionBlockSymbolsFlag.trimmingCharacters(in: ["-"])], fileSystem: fileSystem) {
    commandLine += [extensionBlockSymbolsFlag]
} else {
    print("warning: dropped \(extensionBlockSymbolsFlag) flag because it is not supported by this compiler version")
}

People would still see the flag in the help dialog of SwiftPM even if they use it with a compiler version that doesn't support it. Since they manually configured their setup to use SwiftPM/compiler versions from different toolchain versions I guess that's ok, though. I also wouldn't know an intended way to conditionally disable an ArgumentParser flag at runtime, but if necessary I'm sure it's possible somehow.

@QuietMisdreavus
Copy link

@swift-ci Please smoke test

@theMomax theMomax force-pushed the add-extension-block-symbols-flags branch from a3a1bfb to cca49d5 Compare December 15, 2022 06:35
@neonichu
Copy link
Contributor

@swift-ci please smoke test

@neonichu
Copy link
Contributor

@theMomax emitting a warning like you're doing seems like the right thing to do

@theMomax
Copy link
Contributor Author

Looks like this is ready to go. Thanks for your help @neonichu !

@neonichu neonichu merged commit 5e7063a into swiftlang:main Dec 15, 2022
@compnerd
Copy link
Member

@shahmishal why did this not trigger the Windows build? This seems to have broken the windows build.

compnerd added a commit that referenced this pull request Dec 15, 2022
…ols and -omit-extension-block-symbols flags (#5892)"

This reverts commit 5e7063a.
compnerd added a commit that referenced this pull request Dec 15, 2022
…ols and -omit-extension-block-symbols flags (#5892)" (#5975)

This reverts commit 5e7063a.
theMomax added a commit to theMomax/swift-package-manager that referenced this pull request Dec 16, 2022
…ock-symbols and -omit-extension-block-symbols flags (swiftlang#5892)" (swiftlang#5975)"

This reverts commit 9b81979.
neonichu pushed a commit that referenced this pull request Dec 20, 2022
…nd `SymbolGraphOptions`" (#5978)

* Revert "Revert "dump-symbol-graph: add support for -emit-extension-block-symbols and -omit-extension-block-symbols flags (#5892)" (#5975)"

This reverts commit 9b81979.

* make DriverSupport a private dependency in SymbolGraphExtract
tomerd pushed a commit that referenced this pull request Dec 21, 2022
… SymbolGraphOptions" (#5985)

bring back dump-symbol-graph: add support for -emit-extension-block-symbols and -omit-extension-block-symbols flags (#5892)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants