[Requirement Machine] Diagnose redundant requirements. #41664
Conversation
…struction, and diagnose trivially redundant rules in the rewrite system.
rewrite system. This ID can be used to index into the WrittenRequirements array in the rewrite system to retrieve the structural requirement, e.g. for the purpose of diagnostics.
…imization of the rewrite system.
rules in a separate pass after homotopy reduction. RewriteSystem::propagateExplicitBits was too eager in propagating IDs from explicit rules within rewrite loops, which resulted in bogus redundancy warnings when the canonical form of an explicit rule was given a different requirement ID. Instead, propagate requirement IDs after homotopy reduction when redundant rules are computed and rewrite loops are simplified.
…edundant
rule has a non-explicit, non-redundant rule in its rewrite path.
This fixes bogus redundancy diagnostics in cases where the canonical form
of a redundant rule is not explicit in source, e.g.
protocol Swappable2 {
associatedtype A
associatedtype B
associatedtype Swapped : Swappable2
where Swapped.B == A,
Swapped.Swapped == Self
}
in the above case, the canonical rule for `Swapped.B == A` is the rule
[Swappable2:Swapped].[Swappable2:A] => [Swappable2:B], which is not
explicit.
non-explicit, non-redundant replacement rule is not in the rewrite system's minimization domain.
There was a problem hiding this comment.
This looks great!
I have some suggestions for further cleanups, but only two I think are important:
- Untangling isExplicit() / setRequirementID() since they're now computed in separate places
- Replacing the std::tuple with a named type
For the rest it's up to you if you want to do them in this PR, another PR or not bother at all.
| if (!step.isInContext() && !evaluator.isInContext()) | ||
| rulesInEmptyContext.insert(step.getRuleID()); | ||
|
|
||
| ++ruleFrequency[step.getRuleID()]; |
There was a problem hiding this comment.
Ok, I agree that 'frequency' is probably better than 'multiplicity' :)
| @@ -656,6 +715,8 @@ void RewriteSystem::performHomotopyReduction( | |||
|
|
|||
| deleteRule(ruleID, replacementPath); | |||
| } | |||
|
|
|||
| propagateRedundantRequirementIDs(); | |||
There was a problem hiding this comment.
Maybe this should be done once at the end? (performHomotopyReduction() is called three times)
| @@ -104,7 +104,12 @@ struct RuleBuilder { | |||
|
|
|||
| /// New rules derived from requirements written by the user, which can be | |||
| /// eliminated by homotopy reduction. | |||
| std::vector<std::pair<MutableTerm, MutableTerm>> RequirementRules; | |||
| std::vector<std::tuple<MutableTerm, MutableTerm, Optional<unsigned>>> | |||
There was a problem hiding this comment.
I prefer to define a named type if I need a tuple of length > 2 because std::get<N>() is kind of ugly. Maybe RequirementRule or something?
| std::vector<std::tuple<MutableTerm, MutableTerm, Optional<unsigned>>> | ||
| RequirementRules; | ||
|
|
||
| /// Requirements written in source code. The requirement ID in the above |
There was a problem hiding this comment.
These have been desugared at this point right? Might want to note that.
| @@ -225,6 +239,12 @@ class RewriteSystem final { | |||
| /// top-level generic signature and this array is empty. | |||
| ArrayRef<const ProtocolDecl *> Protos; | |||
|
|
|||
| /// The requirements written in source code. | |||
| std::vector<StructuralRequirement> WrittenRequirements; | |||
There was a problem hiding this comment.
In a similar vein to the Errors vector, I'm wondering if its better to avoid storing this in the rewrite system, and instead pass it down from the request evaluator request to getRedundantRequirements() since it's only used there to get the Requirement and SourceLoc for the diagnostic.
|
|
||
| // Map redundant rule IDs to their rewrite path for easy access | ||
| // in the `isRedundantRule` lambda. | ||
| llvm::SmallDenseMap<unsigned, RewritePath> redundantRules; |
There was a problem hiding this comment.
I doubt it actually matters for efficiency but it's a bit unfortunate to copy the RewritePath here since it's backed by an std::vector. Since the property of the rewrite path that you use is whether it contains an 'implied rule', can you precompute that in the map value instead?
…tion out into a method on RewritePath.
hborla
force-pushed
the
redundant-requirements
branch
from
45cf619 to
b5d7a01
Compare
…other diagnostics in the rewrite system, pass down the 'errors' vector from the top-level requests.
…ement ID for a rewrite rule.
initialization of the rewrite system. Instead, the rewrite system can determine trivially redundant requirements by finding structural requirements with no associated rewrite rules.
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Linux |
3 similar comments
|
@swift-ci please smoke test Linux |
|
@swift-ci please smoke test Linux |
|
@swift-ci please smoke test Linux |
Plumb source location info through the rewrite system by storing the structural requirements used to derive explicit rules, and store the structural requirement ID in the rule itself. Structural requirement IDs are propagated from redundant rules to their replacements via
RewriteSystem::propagateRedundantRequirementIDsafter homotopy reduction.RewriteSystem::getRedundantRequirementscomputes the set of redundant requirement diagnostics to be emitted fromRequirementSignatureRequestRQMandInferredGenericSignatureRequestRQM.This change does not (yet) emit notes to show where a requirement is made redundant.
Resolves: rdar://88135415