Skip to content

Conversation

@Zimmi48
Copy link
Member

@Zimmi48 Zimmi48 commented May 30, 2024

Follow-up of Zulip discussion at: https://coq.zulipchat.com/#narrow/stream/237977-Coq-users/topic/Better.20default.20options

Let's keep the momentum on this good idea. I've started populating this file with an easy one. What else should go in there and is not controversial? For instance, I saw Set Universe Polymorphism. being suggested in https://coq.zulipchat.com/#narrow/stream/237977-Coq-users/topic/Your.20favorite.20secret.20Coq.20option.3F, but is it already recommended for all users?

  • Added changelog.
  • Added / updated documentation.

@Zimmi48 Zimmi48 added needs: progress Work in progress: awaiting action from the author. kind: feature New user-facing feature request or implementation. needs: documentation Documentation was not added or updated. kind: compatibility Changes allowing for compatibility between versions. needs: changelog entry This should be documented in doc/changelog. labels May 30, 2024
@Zimmi48 Zimmi48 added this to the 8.20+rc1 milestone May 30, 2024
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label May 30, 2024
@TheoWinterhalter
Copy link
Contributor

Thanks for opening this!

I would say the relevant proposals are

  • Primitive Projections
  • Keyed Unification
  • Asymmetric Patterns
  • Uniform Inductive Parameters

For sure on the list should be the recommended config for type classes but I don't know it.

@Zimmi48
Copy link
Member Author

Zimmi48 commented May 30, 2024

If anyone disagrees with the proposed options, please voice your opinion.

BTW, @TheoWinterhalter, you may be able to push changes directly to this branch, in particular if you see the "Add more commits by pushing to the good-defaults branch on Zimmi48/coq." message above the CI status section.

@ppedrot
Copy link
Member

ppedrot commented May 30, 2024

I'm a bit wary of the Keyed Unification option, it's probably wildly undertested and thus very buggy.

@SkySkimmer
Copy link
Contributor

SkySkimmer commented May 30, 2024

We know primitive projections are pretty buggy too.

@Zimmi48
Copy link
Member Author

Zimmi48 commented May 30, 2024

OK. I advise that we avoid any controversial or insufficiently tested option. We could make a separate file ExperimentalGoodDefaults if we want to include potentially buggy options that we would still like to recommend in the long run.

@SkySkimmer
Copy link
Contributor

I guess we should look through the deprecated warnings to see if any correspond to options that may be flipped.
For instance when we added export locality to hints we could have had an option to make it the default early, and enabled the option in gooddefaults.

@ppedrot
Copy link
Member

ppedrot commented May 30, 2024

But if we do that we need to version this file per release...

@Zimmi48
Copy link
Member Author

Zimmi48 commented May 30, 2024

But if we do that we need to version this file per release...

That's already the case here.

@SkySkimmer
Copy link
Contributor

Versioning it is the whole point in fact. If we didn't version we may as well flip the global defaults.

@TheoWinterhalter
Copy link
Contributor

TheoWinterhalter commented May 30, 2024

Thanks @Zimmi48, but I actually don't want to push things, I'm merely listing options that might be worth adding, but I'm myself unsure whether they should, and I think it should be debated.

@proux01
Copy link
Contributor

proux01 commented May 31, 2024

Since this is a draft, let's remove the milestone.

@proux01 proux01 removed this from the 8.20+rc1 milestone May 31, 2024
@Zimmi48
Copy link
Member Author

Zimmi48 commented May 31, 2024

There was a good reason for the milestone: the file has a version number. If it is not merged in this version, it should be renamed.

@proux01
Copy link
Contributor

proux01 commented May 31, 2024

Then rename to 8.21 and put in 8.21+rc1 milestone.

@Zimmi48
Copy link
Member Author

Zimmi48 commented May 31, 2024

Why would I do that, exactly? The documented branching date for 8.20 is 2024-06-17. So if this PR is ready to merge before that date, it can still go in 8.20, no?

@proux01
Copy link
Contributor

proux01 commented May 31, 2024

Sure

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jun 18, 2024
@TheoWinterhalter
Copy link
Contributor

Coming back to this so it doesn't die. I think we would need have a flag to change default hint mode for typeclasses. Currently I think it's + for each argument, but it would be good to set it to ! iinstead. At least that's my understanding.
There is no way to change the default globally yet, correct?

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 18, 2024

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app bot added the stale This PR will be closed unless it is rebased. label Jul 18, 2024
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Aug 19, 2024

This PR was not rebased after 30 days despite the warning, it is now closed.

@coqbot-app coqbot-app bot closed this Aug 19, 2024
@TheoWinterhalter
Copy link
Contributor

I see #19473 is relevant here.

@TheoWinterhalter TheoWinterhalter removed the needs: changelog entry This should be documented in doc/changelog. label Jun 26, 2025
@TheoWinterhalter TheoWinterhalter added this to the 9.1+rc1 milestone Jun 26, 2025

From Corelib Require Import GoodDefaults_2025.

This file is versioned to account for the changes in recommendations as the Rocq
Copy link
Member

Choose a reason for hiding this comment

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

Do you expect these recommendations will over time become the default settings? If changes in the defaults are manageable for users, maybe that would be simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great, but it is much more work, as it can break things on the user side. For now it's what we recommend for new users / files.

@SkySkimmer SkySkimmer removed this from the 9.1+rc1 milestone Jun 27, 2025
@SkySkimmer
Copy link
Contributor

too late for 9.1

@Zimmi48
Copy link
Member Author

Zimmi48 commented Jun 27, 2025

@TheoWinterhalter You should put yourself as a co-author in the commit message (using a git trailer).

@TheoWinterhalter TheoWinterhalter force-pushed the good-defaults branch 2 times, most recently from 7cd54f6 to 960c44d Compare June 27, 2025 12:58
@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jul 3, 2025
(** These three affect pepole that set [Implicit Arguments]. *)

#[ export ] Set Strongly Strict Implicit.
#[ export ] Set Maximal Implicit Insertion.
Copy link
Member Author

Choose a reason for hiding this comment

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

@anton-trunov seemed to defend the current default in his answer on Stack Overflow back in 2016: https://stackoverflow.com/questions/37211899/purpose-of-maximal-vs-non-maximal-implicit-arguments

That being said, I don't think the issue of having to add @ when you want to refer to a non-partially-applied constant is such a pain.

And on the other hand, the same @anton-trunov supported in 2021 the switch to maximally implicit arguments for lists (which didn't land) in #13069.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, 2016 is old

@proux01
Copy link
Contributor

proux01 commented Jul 7, 2025

Just a thought: if those are really good defaults, they should probably be actual defaults. Then the compat file can be used (and recommended in the changelog entry) to retrieve the previous defaults and help transitioning.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Jul 7, 2025

Just a thought: if those are really good defaults, they should probably be actual defaults. Then the compat file can be used (and recommended in the changelog entry) to retrieve the previous defaults and help transitioning.

OK, but who does the work of porting external projects to these new defaults? (Although, of course, porting could also mean resetting the flags to their old values in the affected projects.)

@proux01
Copy link
Contributor

proux01 commented Jul 7, 2025

Nothing specific to this change compared to any other. Porting is obviously the responsibility of authors. They can simply use the compat file (with option -compat 9.1) if they want to keep the old settings/avoid porting.

@TheoWinterhalter
Copy link
Contributor

I'm not against having them as actual defaults yes. We just thought this experiment had more chances to succeed since we're not messing with anyone.

@proux01
Copy link
Contributor

proux01 commented Jul 7, 2025

I fear, this also means more chances to just remained ignored or simply unknown :(

@Zimmi48
Copy link
Member Author

Zimmi48 commented Jul 7, 2025

Sure, but this is a better middle ground to doing nothing, which has been what has been done for years for many of these defaults. There were even attempts at changing some of these defaults, which failed (e.g., #17811).

@proux01
Copy link
Contributor

proux01 commented Jul 7, 2025

We have a better CI contract now, maybe that would help?

@coqbot-app coqbot-app bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jul 8, 2025
@TheoWinterhalter
Copy link
Contributor

Now that #20820 is merged, I removed the corresponding line in the file.
I also added Set Keyed Unification to please @ppedrot.

@ppedrot
Copy link
Member

ppedrot commented Jul 12, 2025

I would also vouch to set the TC database opaque by default, see #20786.

Co-authored-by: Théo Winterhalter <[email protected]>

Follow-up of Zulip discussion at: https://coq.zulipchat.com/#narrow/stream/237977-Coq-users/topic/Better.20default.20options
Update theories/Corelib/Compat/GoodDefaults_2025.v

Co-authored-by: Théo Zimmermann <[email protected]>
@TheoWinterhalter
Copy link
Contributor

Done.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Jul 21, 2025

This should probably get merged. Then, we will have until the 9.2 release to nitpick the settings / improve the Good Defaults 2025 file before it is frozen forever.

@TheoWinterhalter
Copy link
Contributor

TheoWinterhalter commented Oct 9, 2025

Is there anything we can do to have this PR accepted?
I was thinking maybe the name could be Recommended2025 or RecommendedSettings2025 but I'd prefer it to be merged over bikeshedding (which we can do after, with other PRs).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: compatibility Changes allowing for compatibility between versions. kind: feature New user-facing feature request or implementation. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants