Conversation
PR summary 8077d16145Import changes for modified filesNo significant changes to the import graph Import changes for all files
|
|
!radar |
|
Benchmark results for df1ccc6 against 3a33b9d are in. No significant results found. @JovanGerb
No significant changes detected. |
Vierkantor
left a comment
There was a problem hiding this comment.
I'm not completely sure about the default behaviour. But the heuristic seems to be a strict improvement at least, so I can easily be convinced that we are in a local maximum :)
| with `relevant_arg` defaulting to the first argument. -/ | ||
| translateClass (cls : Name) : CoreM Unit := do | ||
| if (findTranslation? (← getEnv) t cls).isNone then | ||
| modifyEnv (t.translations.addEntry · (cls, { translation := cls, relevantArg := .arg 0 })) |
There was a problem hiding this comment.
I can imagine that the first argument is often the right one, but it seems like it might misbehave on something unbundled like IsOrderedSMul? (Or maybe even IsOrderConnected?)
There was a problem hiding this comment.
Thanks, this could indeed go wrong (although I don't think there are examples in practice). I've fixed it by running the translation from the type of cls to itself, and using the relevant_arg inferred from that.
…ays assuming it.
This PR improves the heuristic around
relevant_argthatto_dualandto_additiveuse. Currently, the we eagerly set(relevant_arg := 1)whenever we cannot find any relevant argument. This is problematic in a setting where there really should be no relevant argument, such as forComplexShape.After this PR, we only default to
(relevant_arg := 1)for types (i.e. anything whose type isSort _). For terms that aren't types, we instead purely rely on the algorithm that determines the relevant argument (which was added in #35318). In order for this algorithm to work in all cases,relevant_argneeds to be set on all relevant types. This includes some types that are dual to themselves, likeLEandLinearOrder, and hence do not have ato_dualattribute (and hence also don't have arelevant_arg). To fix this, I added the following trick: when we add a translation for a class projection or class constructor, and if that class has no translation, then we first add a translation from that class to itself.As a result, classes like
LEandLinearOrderare now part of the translation dictionary, and are being translated into themselves.