Skip to content
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

chore: normalize naming convention in theorems #1897

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Nov 30, 2022

Now that the naming convention has been tested in mathlib, I think it is safe to backport the changes to lean core and normalize any remaining inconsistencies in theorem names. Here are the more prominent changes:

  • {out, opt, auto}Param -> {Out, Opt, Auto}Param. This is because these operate on types to produce another type, even if they are just annotations.
  • Lawful{Functor, Applicative, Monad} -> IsLawful{Functor, Applicative, Monad}. This is probably the most debatable of the changes here, but we want to keep the Is* naming convention for classes which are propositions, at least when the * is a noun, so that we can differentiate between IsGroup and Group. I did not apply the naming change to LawfulBEq because the grammar is a bit strained; discuss.
  • I considered but did not rename Membership to HasMem. In mathlib we are finding that we still need Has* classes when the original name is already taken; AndOp is another example of this. Using Membership is a pattern that doesn't scale.
  • Membership.mem -> Membership.Mem, following the practice that even structure fields are capitalized if they denote types or relations / propositions.
  • congr{Arg, Fun} -> congr{_arg, _fun}. This one should not be too contentious.
  • T.constr.injEq -> T.constr.inj_eq
  • by{Cases, Contradiction} -> by{_cases, _contradiction}
  • I considered but did not rename Ne -> NE. The rationale is that it is an acronym, matching the distinction between Eq and LE. Discuss.
  • ofReduce{Bool, Nat} -> of_reduce{Bool, Nat}
  • left_distrib -> mul_add, right_distrib -> add_mul. The names are more mnemonic and descriptive, and the latter names were already being provided as aliases. In mathlib4 the "distrib" aliases have already been removed and this is backporting the change to core.
  • Membership.mem.{lower, upper} -> Std.Range.mem_{lower, upper}. It's unfortunate that we lose dot notation here but the original names were way too generic (they are about the upper and lower bounds induced by membership in a Std.Range). You can still use dot notation with the less mnemonic h.1, h.2 though.
  • and_true -> and_true_eq, true_or -> true_or_eq and so on. We talked about this before. These are simp lemmas that use equality on proposition and std/mathlib would like to use the unadorned names for the iff version. Since they are usually not referenced directly, it seems reasonable to disambiguate these lemmas.

@leodemoura leodemoura added the dev meeting It will be discussed at the (next) dev meeting label Nov 30, 2022
@gebner gebner removed the dev meeting It will be discussed at the (next) dev meeting label Jan 23, 2023
@semorrison
Copy link
Collaborator

Because this has stage0 changes, there's no possibility of merging master. We'd either need to do a rather tedious rebase, or effectively start over.

I will be a bit sad if we don't do this (or at least a big chunk of it). Naming consistency is really nice, even if it is not immediately high impact.

Perhaps we could get this done in smaller parcels...?

@digama0
Copy link
Collaborator Author

digama0 commented Nov 27, 2023

Because this has stage0 changes, there's no possibility of merging master. We'd either need to do a rather tedious rebase, or effectively start over.

There is a standard procedure for rebasing PRs that contain update-stage0 commits, which is to git rebase -i, and replace the update stage0 commit with x update-stage0.sh where update-stage0.sh is:

#!/bin/sh
cd build/release
make update-stage0
git add ../../stage0/
git ci -m "chore: update stage0"

In other words, rebase the other commits normally but delete and recreate the update-stage0 commits. This is exactly why they are kept separate.

@semorrison
Copy link
Collaborator

semorrison commented Nov 27, 2023

Interesting! This script should live somewhere. :-)

It could also do with a -j 32 on the make, and use git commit instead of git ci for portability.

Also for anyone coming afterwards, you need a path to the update-stage0.sh script, e.g. x ./update-stage0.sh.

It's taken me a few tries to resolve the first set of conflicts so that update-stage0.sh succeeds... we'll see where it goes.


I've attempted the rebase, but I don't really like the result. In particular, the rename iff_self to iff_self_eq results in every call to simp failing, because it is a simpOnlyBuiltin (and just renaming it there doesn't help). I'm not quite sure how this was working out previously.

Init/Data/Nat/Basic.lean:626:17: error: unknown constant 'iff_self'

@digama0
Copy link
Collaborator Author

digama0 commented Nov 27, 2023

I've attempted the rebase, but I don't really like the result. In particular, the rename iff_self to iff_self_eq results in every call to simp failing, because it is a simpOnlyBuiltin (and just renaming it there doesn't help). I'm not quite sure how this was working out previously.

The follow up changes were never performed, it was pending a clearer 👍 regarding the specific renames in question before it seemed worth it to fix the issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants