Skip to content

[Merged by Bors] - feat: change Type to Sort in Algebra/Classes, fix some priorities#10354

Closed
sgouezel wants to merge 1 commit intomasterfrom
SG_adjust_prio
Closed

[Merged by Bors] - feat: change Type to Sort in Algebra/Classes, fix some priorities#10354
sgouezel wants to merge 1 commit intomasterfrom
SG_adjust_prio

Conversation

@sgouezel
Copy link
Copy Markdown
Contributor

@sgouezel sgouezel commented Feb 8, 2024


These changes were needed to fix the build in the (unsatisfactory) experiment #10335. They seem useful to have in any case, as independent refactors could stumble on the same issues.

Open in Gitpod


-- porting note: removed `outParam`
class IsSymmOp (α : Type u) (β : Type v) (op : α → α → β) : Prop where
class IsSymmOp (α : Sort u) (β : Sort v) (op : α → α → β) : Prop where
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please record a summary (maybe as library note?) that motivates why these classes should be defined on Sorts instead of Types?
It's not clear to me why we would want to allow Props here...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I had to fix in #10335 was that the IsTrans defined in this file gained higher priority, and then Lean tried to apply it to Eq in calc blocks and failed because of universe issues, as IsTrans was only defined on Type u and it was trying to apply it to Prop. One fix was to extend the IsTrans to Sort instead of Type, and then I thought that it would make sense to have all these definitions on Sort as it makes universe unification a bit easier, for essentially no cost. A better fix, IMHO, would be to just remove this file as we don't really use it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also discard the PR, it's not as if it's really important.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your motivation is valid. Let's just merge this.

bors merge

@jcommelin jcommelin added awaiting-author A reviewer has asked the author a question or requested changes. and removed awaiting-review labels Feb 8, 2024
@sgouezel sgouezel added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes. labels Feb 8, 2024
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Feb 8, 2024

Build failed (retrying...):

@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Feb 8, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat: change Type to Sort in Algebra/Classes, fix some priorities [Merged by Bors] - feat: change Type to Sort in Algebra/Classes, fix some priorities Feb 8, 2024
@mathlib-bors mathlib-bors bot closed this Feb 8, 2024
@mathlib-bors mathlib-bors bot deleted the SG_adjust_prio branch February 8, 2024 15:58
@sgouezel sgouezel added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has been sent to bors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants