-
Notifications
You must be signed in to change notification settings - Fork 388
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
feat: validate reducibility attribute setting #4052
Conversation
Mathlib CI status (docs):
|
@@ -65,15 +70,75 @@ def setReducibilityStatusCore (env : Environment) (declName : Name) (status : Re | |||
private def setReducibilityStatusImp (env : Environment) (declName : Name) (status : ReducibilityStatus) : Environment := | |||
setReducibilityStatusCore env declName status .global .anonymous | |||
|
|||
/- | |||
TODO: it would be great if we could distinguish between the following two situations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ReducibilityHints.abbrev
be used to distinguish between these cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it does not help. The key issue here is that the current attribute management system does not tell us whether an attribute is being set at a declaration time or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, having the attribute know whether it is applied at declaration time would be the best solution. However, I was proposing a potential partial solution assuming that was too complicated for now.
If I understand correctly, abbrev
is just a @[reducible, inline] def
with ReducibilityHints.abbrev
. Thus, the primary use case for a declaring @[reducible] def
over an abbrev
is when the definition is meant to be reducible but not inlined. In such a case, @[noinline] abbrev
could serve the same purpose. Thus, to avoid the potential for applying @[reducible]
after a declaration, we could forbid @[reducible]
from being applied to anything except abbrev
at declaration-time (i.e., when it is semireducible
). This would avoid the need to make the declaration time distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tydeu This is a good suggestion. Then, we would not even have the attribute [reducible]
. It seems there are very few instances of @[reducible] def
in Mathlib. So, the impact would be minimal. @semorrison What do you think? We can use a different PR for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, adding a flag to AttributeImpl.add
that tells us whether this is a post-hoc application seems like a simple (apart from downstream breakage) and generally very useful change to me
Co-authored-by: Mario Carneiro <[email protected]>
and new option
set_option allowUnsafeReductibility true
to override validation.