[SwiftWarningControl] Add ability to specify diagnostic sub-group links #3183
Conversation
artemcm
requested review from
ahoppen,
bnbarham,
hamishknight and
rintaro
as code owners
|
@swift-ci test |
There was a problem hiding this comment.
How large do you envision subGroupLinks to be? Just thinking if it would be more efficient to take the sub group relationships into account when querying for warning controls instead of when populating the tree. Kind of depends on whether you expect to have more queries or more @warn controls.
artemcm
force-pushed
the
WarningControl-GroupLinks
branch
from
a797833 to
5b18fb4
Compare
I considered both options and went with this one to take the relationships into account at tree construction: my main reason for this is that I expect the primary client use-case (such as the compiler in swiftlang/swift#85036) to pay the cost of constructing a tree and then run queries against said tree. I do expect there to be a lot of queries against a constructed tree, and it's tricky to predict how many group relationships we will end up with since that concept is still quite new. Baking these relationships into the constructed tree does also result in simpler query logic on the client side. |
|
@swift-ci test |
Sources/SwiftWarningControl/SyntaxProtocol+WarningControl.swift
Outdated
Show resolved
Hide resolved
| let newNode = WarningControlRegionNode(range: range) | ||
| for (diagnosticGroupIdentifier, control) in controls { | ||
| newNode.addWarningGroupControl(for: diagnosticGroupIdentifier, control: control) | ||
| var groups: [DiagnosticGroupIdentifier] = [diagnosticGroupIdentifier] |
There was a problem hiding this comment.
What if we have the following sub-groups?
A: B, C
B: C
Then we’d report C twice if you have a warning control for A. When using a Set, we need to think about whether we want to guarantee a deterministic output ordering of the diagnostic groups (which I think we should), so we’d need to sort somewhere. So maybe it would be easier to avoid duplicates in an Array.
Also, since the inheritance tree is user-provided, I think we need to think about cycles somewhere. Either make it the responsibility of DiagnosticGroupInheritanceTree to ensure there are no cycles (and throw if there are), or try and find cycles here. I’d prefer the former option.
There was a problem hiding this comment.
That's a great catch, thank you.
I added code to keep track of already-added groups for a given control to ensure we don't add duplicates to this array.
I also added a cycle check in the initializer of a DiagnosticGroupInheritanceTree and factored it out into a separate file DiagnosticGroupInheritanceTree.swift. It will throw an error if initialized with a subgroup dictionary which contains cycles.
artemcm
force-pushed
the
WarningControl-GroupLinks
branch
2 times, most recently
from
e59be92 to
8feabad
Compare
…ance This takes form of a new parameter: `groupInheritanceTree: DiagnosticGroupInheritanceTree? = nil` on both: - `SyntaxProtocol.warningGroupBehavior` - `SyntaxProtocol.warningGroupControlRegionTree` Where `DiagnosticGroupInheritanceTree` is a wrapper for a dictionary where the key is a super-group and values are its sub-groups. Upon encountering a warning group control (`@warn`), its corresponding region is populated with its identifier and behavior, as well as the same corresponding behavior for each of its sub-groups, direct and transitive.
artemcm
force-pushed
the
WarningControl-GroupLinks
branch
from
8feabad to
00891ac
Compare
|
@swift-ci test |
2 participants
These links establish group inheritance relationships amongst diagnostic groups. This takes form of a new parameter:
subGroupLinks: [DiagnosticGroupIdentifier: [DiagnosticGroupIdentifier]] = [:]on both:SyntaxProtocol.warningGroupBehaviorSyntaxProtocol.warningGroupControlRegionTreeWhere the dictionary key is a super-group and dictionary values are its sub-groups.Upon encountering a warning group control (
@warn), its corresponding region is populated with its identifier and behavior, as well as the same corresponding behavior for each of its sub-groups, direct and transitive.