consolidate and improve manifest validation #4203
Conversation
|
@neonichu @abertelrud this needs a bit more work to get the tests aligned, but wanted early feedback on the approach |
Sources/Workspace/Workspace.swift
Outdated
| } | ||
| manifestLoadingScope.emit(manifestLoadingDiagnostics) |
There was a problem hiding this comment.
here is the key change (and motivation): it allows us to get rid of the fake observability scope and special casing Diagnostics.fatalError
There was a problem hiding this comment.
That looks like a good approach. And this means the didLoadManifest() callback and the diagnostics handler will always see all the diagnostics, from the looks of it.
Does that mean that a client that just wanted to capture diagnostics at the end (in didLoadManifest()) could pass a no-op manifestLoadingScope and just implement the delegate callback? In practice it seems that any given client would only want to do one or the other, right, otherwise it would get duplicate diagnostics.
There was a problem hiding this comment.
I realize this might not be any different from before because of this PR, but it's always been somewhat unclear here what a client should ideally do to avoid duplication.
There was a problem hiding this comment.
Does that mean that a client that just wanted to capture diagnostics at the end (in didLoadManifest()) could pass a no-op manifestLoadingScope and just implement the delegate callback? In practice it seems that any given client would only want to do one or the other, right, otherwise it would get duplicate diagnostics.
@abertelrud who would be the client in this case? this is private function, and so the observabilityScope passed to it may contain other diagnostics above and beyond manifest loading.
fwiw I dont love the current design - I think a better one may be to return a diagnostics key in the delegate (instead of actual diagnostics) such a libSwiftPM client can use to filter the relevant diagnostics entries by the key. that kind of change will be more disruptive to clients of libSwiftPM. If we feel its worth doing we can certainly change break the delegate API in that direction
There was a problem hiding this comment.
Sorry, I misread this and didn't realize that it's a private function (thus the client would be the client of libSwiftPM, but that point is moot, as it's private).
I agree with the other sentiment and think we'll want to revisit the delegate here at some point. There's a bit of a split personality between the delegate callbacks but then the diagnostics in the scope.
| _ contents: String, | ||
| toolsVersion: ToolsVersion? = nil, | ||
| packageKind: PackageReference.Kind? = nil, | ||
| observabilityScope: ObservabilityScope, | ||
| file: StaticString = #file, | ||
| line: UInt = #line | ||
| ) throws -> Manifest { | ||
| try self.loadManifest( | ||
| ) throws -> (manifest: Manifest, diagnostics: [Basics.Diagnostic]) { |
There was a problem hiding this comment.
the validator does not take an observability scope, instead it returns an array of diagnostics which the caller can decide what to with
There was a problem hiding this comment.
What's the advantage of doing it that way rather than a custom scope that just captures them and then passes them on? That would allow the validation to use the normal pattern for emitting diagnostics.
There was a problem hiding this comment.
an API design preference: a validator is designed to return a list of validation issues vs. some functional api that may need to emit diagnostics so has to takes a "scope" as argument to be able to do so
tomerd
changed the title
tomerd
force-pushed
the
refactor/consolidate-manifest-validation
branch
from
cab831f to
0a7a30b
Compare
tomerd
changed the title
|
@swift-ci smoke test |
tomerd
added
the
ready
tomerd
force-pushed
the
refactor/consolidate-manifest-validation
branch
2 times, most recently
from
4c5a4b1 to
f595624
Compare
|
@swift-ci smoke test |
|
@abertelrud @neonichu this has been rebased and ready for review |
|
Looks good, and the only nit now was about what looks like a boilerplate source file header (which I guess that new script could fix actually :-). |
|
@swift-ci smoke test |
tomerd
force-pushed
the
refactor/consolidate-manifest-validation
branch
from
2aaaf15 to
25aa094
Compare
|
@swift-ci smoke test |
motivation: make code easier to reason about and maintain changes: * create new ManfeistValidator utility * move manifest validation code out of ManifestLoader and Workspace and into the new validation utility * seperate out manifst validation diagnostics from manifest loading diagnostics such that a fake diagnostics scope is no longer required * adjust call sites and test
tomerd
force-pushed
the
refactor/consolidate-manifest-validation
branch
from
25aa094 to
7fcd79e
Compare
|
@swift-ci smoke test |
As part of https://github.com/apple/swift-evolution/blob/main/proposals/0339-module-aliasing-for-disambiguation.md, a new API with an optional `package` parameter was accidentally added. We can safely remove this API since #4203 added validation that prohibited the use of product declarations without a package anyway. As part of this, I also cleaned up that diagnostic since "unknown package 'unknown package name'" looks quite ugly. rdar://114303935 fixes #7158
As part of https://github.com/apple/swift-evolution/blob/main/proposals/0339-module-aliasing-for-disambiguation.md, a new API with an optional `package` parameter was accidentally added. We can safely remove this API since #4203 added validation that prohibited the use of product declarations without a package anyway. As part of this, I also cleaned up that diagnostic since "unknown package 'unknown package name'" looks quite ugly. rdar://114303935 fixes #7158
motivation: make code easier to reason about and maintain
changes: