Skip to content

TSCUtility: deprecate Bitstream interfaces #351

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 1 commit into from
Oct 7, 2022

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Oct 7, 2022

The bitstream interfaces were only used by swift-driver. The implementation has been migrated into swift-driver, allowing us to now mark the interfaces as deprecated.

The bitstream interfaces were only used by swift-driver.  The
implementation has been migrated into swift-driver, allowing us to now
mark the interfaces as deprecated.
@compnerd
Copy link
Member Author

compnerd commented Oct 7, 2022

@swift-ci please test

@tomerd tomerd merged commit 84449cd into swiftlang:main Oct 7, 2022
@compnerd compnerd deleted the not-bc-is-not-ad branch October 7, 2022 17:35
@abertelrud
Copy link
Contributor

SerializedDiagnostics also use the Bitstream interfaces, and now have warnings. SwiftPM hasn't yet switched over to use that instead of just passing the .dia file up to Xcode to parse using libclang, but the plan was to move in that direction. Is the plan to remove SerializedDiagnostics also?

@tomerd
Copy link
Contributor

tomerd commented Oct 8, 2022

we can roll this back if premature. @compnerd ?

@compnerd
Copy link
Member Author

compnerd commented Oct 8, 2022

@tomerd sure, we can revert this for now. I can try to create an internal alias to avoid the warnings. Not sure if I can do anything about removing the use of SerializedDiagnostics in SwiftPM, which would certainly be nice (otherwise we will have a copy of the bitcode logic in swift-driver and SPM).

@abertelrud
Copy link
Contributor

abertelrud commented Oct 8, 2022

Hmm... having two copies is certainly not good. But what was the reason for wanting to move BitstreamReader directly into SwiftDriver package in the first place? Is it just part of chipping away at TSC?

From SwiftPM's perspective, it certainly seems desirable for it to be able to understand .dia files without taking on external dependencies on libclang etc. SwiftPM's use of SerializedDiagnostics is still only minimal, but I was hoping to make more use of it because right now the manifest and plugin compilation APIs in libSwiftPM are kind of awkward: the SwiftPM logic doesn't actually see the errors but just passes the path of the .dia to an IDE like Xcode which then has to use libclang.dylib to read them. Every IDE using libSwiftPM has to do this separately.

Besides being ugly, this makes it hard for SwiftPM to reason about the errors, or to produce formatted output or anything like that for diagnostics from compiled manifests and plugins. So I was hoping to clean things up by having SwiftPM just load the diagnostics and then send them through the normal observability scope APIs that it uses for diagnostics. There were a couple of fixes needed for that in SerializedDiagnostics, which I have been making on a side branch that isn't quite ready for a PR yet (properly handling annotations attached to top-level scopes rather than emitting multiple top-level diagnostics, for example) and that I was going to put up a TSC PR for.

So making SerializedDiagnostics inaccessible to SwiftPM seems harmful, not nice. If it can't be in TSC, could it be in some other package that can be shared? We all agree (I think) that path and process and other general stuff shouldn't be in TSC now that we have support in SwiftSystem and Foundation etc, but there is a category of developer-focused functionality such as this that seems like it could have a place in a shared package that is geared toward developer tools. There's no way that SwiftSystem or Foundation would add SerializedDiagnostics for example, and yet it's generally useful for any package dealing with LLVM-based tools. Code that parses Mach-O files or knows about various platform peculiarities that aren't specific to SwiftPM but that are useful for any kind of developer tool seems like another category of that. I think that's what Tools Support Core was originally intended to be (though I wasn't directly involved at the time, so I might be wrong about that).

@compnerd
Copy link
Member Author

compnerd commented Oct 8, 2022

Removing SerializedDiagnostics is not the intent, the motivation here is to chip away at tools-support-core. The removal of SerializedDiagnostics would be by pushing that into swift-package-manager as that is the only client of the API. The piece I didn't realize was the internal dependency.

Currently, I'm looking at TSCUtility. We are pretty close to stripping that out in practice. swift-driver depends on Version, Diagnostics and SourceKit-LSP depends on Platform, BuildFlags. The remainder of the interfaces that are in TSCUtility are in use in SwiftPM. It would be possible to sink the rest of the interfaces into SwiftPM.

I suppose I don't fully understand the SwiftPM perspective here. It is also possible to get the diagnostics without serializing them if the desire is to just get the data. That would allow us to filter and stream it through the observability APIs. A dependency on libclang also doesn't seem too onerous to me. At least for Swift toolchain distributions, clang is a requisite component. Additionally, for SourceKit-LSP (which is really an expectation I believe), clangd and libclang are part of the distribution. So, while it might be increasing the interdependence between SPM and the rest of the toolchain, that connection is already present due to the other dependencies.

@abertelrud
Copy link
Contributor

abertelrud commented Oct 9, 2022

It is also possible to get the diagnostics without serializing them if the desire is to just get the data. That would allow us to filter and stream it through the observability APIs.

That sounds very promising! It's annoying that structured diagnostics from Clang and Swift use the bitstream format when it seems that something like a JSON format would have been easier for callers to work with. What's the other way to get these diagnostics in a structured form? I've looked at -parseable-output in the past but although it passes on the textual output of each individual compiler invocation I haven't found a way to get the structured diagnostics that way.

I'm certainly not tied to parsing the .dia — I do think that SwiftPM should understand the diagnostics coming from the compiler (and scraping the textual output is lossy so that's not a good option). But if there's another way at getting the diagnostics without serializing them, then that would be great.

@compnerd
Copy link
Member Author

compnerd commented Oct 9, 2022

I don't think that there is another structured form. The diagnostics can be formatted in specific manners (e.g. msvc, vi, or clang). GCC does support json, which we could add to clang I suppose, as we do try to be compatible with gcc. However, the diagnostics that it seems like are of interest here are the Swift ones rather than the C/C++ ones.

@abertelrud
Copy link
Contributor

I see — I thought you were saying that there was a better structured format (for Swift in particular, which is what the manifest and plugins are — the other diagnostics we let llbuild handle) that was easier to parse than the .dia files.

@compnerd
Copy link
Member Author

compnerd commented Oct 9, 2022

I wonder if we should look at -diagnostics-editor-mode as it may be the mode that we need for dealing with the diagnostics.

neonichu pushed a commit that referenced this pull request Oct 14, 2022
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.

4 participants