[Merged by Bors] - fix: disable autoImplicit globally#6528
[Merged by Bors] - fix: disable autoImplicit globally#6528eric-wieser wants to merge 6 commits intomasterfrom
Conversation
668c30b to
5b8623b
Compare
|
Can you make one with it completely turned off to benchmark? |
|
That's a lot more work (and a lot more controversial). I assume you already realize, but I would expect the performance effect of the PR in its current form to be completely neutral. The goal for now is to limit the scope for accidental introduction of autoimplicits. |
|
Yes. I would expect nothing to change performance wise here. My point is that if autoimplicit itself is not performance neutral then that will need to be considered in whatever decision is reached. |
|
Thanks 🎉 bors merge |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio.
|
bors r- Looks like a fix is needed first |
|
Canceled. |
7eab314 to
d17b1a6
Compare
|
bors p=10 |
|
bors merge p=10 |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio.
…le this PR was being discussed
d17b1a6 to
abd85db
Compare
|
Canceled. |
|
bors merge |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
|
Canceled. |
68d3bda to
dae84f8
Compare
|
bors merge (Another set of PRs got merged and made this rot again. Apologies for hogging the queue, but if I don't the finish line will constantly be one step away) |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
|
Build failed: |
dae84f8 to
c629a98
Compare
|
bors merge |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
|
Build failed:
|
c629a98 to
9bc80ea
Compare
|
bors merge |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
|
Build failed: |
…le this PR was being fixed
9bc80ea to
7f3957e
Compare
|
bors merge This finally passes locally; the previous bors commands were just to ensure that another batch didn't force another 2 hour build/fix-iteration cycle on me. |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
|
Pull request successfully merged into master. Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
These work on the command line and therefore didn't fail in CI (as `lake env lean blah.lean` doesn't pick up the extra lean args?) but as soon as you open one of these files in vscode you get a sea of red errors. I tried to fix a couple in a minimal way, if there was just for example one universe level missing, for most files the "fix" for now is just reenabling autoimplicits for the file. This revealed a couple of tests that were incorrectly using an implicit type when they thought they were using Nat. Two files had issues, that we may want to follow up on `test/superscript.lean` which failed even with autoimplicits turned on when using an autoimplicit superscript variable (unsure if this was intentional). And the `ToExpr` deriving handler seems to rely on autoimplicits somehow to work properly in the presence of universe, if an explicit universe variable is added there is still an error.
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
These work on the command line and therefore didn't fail in CI (as `lake env lean blah.lean` doesn't pick up the extra lean args?) but as soon as you open one of these files in vscode you get a sea of red errors. I tried to fix a couple in a minimal way, if there was just for example one universe level missing, for most files the "fix" for now is just reenabling autoimplicits for the file. This revealed a couple of tests that were incorrectly using an implicit type when they thought they were using Nat. Two files had issues, that we may want to follow up on `test/superscript.lean` which failed even with autoimplicits turned on when using an autoimplicit superscript variable (unsure if this was intentional). And the `ToExpr` deriving handler seems to rely on autoimplicits somehow to work properly in the presence of universe, if an explicit universe variable is added there is still an error.
Autoimplicits are [highly controversial](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/.60autoImplicit.20true.60.20is.20evil/near/355142480) and also [defeat the performance-improving work](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/autoimplicit.20and.20Type*/near/383878932) in #6474. The intent of this PR is to make `autoImplicit` opt-in on a per-file basis, by disabling it in the lakefile and enabling it again with `set_option autoImplicit true` in the few files that rely on it. That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more. I claim that many of the uses of `autoImplicit` in these files are accidental; situations such as: * Assuming `variables` are in scope, but pasting the lemma in the wrong section * Pasting in a lemma from a scratch file without checking to see if the variable names are consistent with the rest of the file * Making a copy-paste error between lemmas and forgetting to add an explicit arguments. Having `set_option autoImplicit false` as the default prevents these types of mistake being made in the 90% of files where `autoImplicit`s are not used at all, and causes them to be caught by CI during review. I think there were various points during the port where we encouraged porters to delete the `universes u v` lines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings. [A Zulip poll](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/mathlib4.3A.20autoImplicit.20false.20by.20default.3F/near/384265143) (after combining overlapping votes accordingly) was in favor of this change with `5:5:18` as the `no:dontcare:yes` vote ratio. While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places, `set_option autoImplicit true` has been placed locally within a section, rather than at the top of the file.
Autoimplicits are highly controversial and also defeat the performance-improving work in #6474.
The intent of this PR is to make
autoImplicitopt-in on a per-file basis, by disabling it in the lakefile and enabling it again withset_option autoImplicit truein the few files that rely on it.That also keeps this PR small, as opposed to attempting to "fix" files to not need it any more.
I claim that many of the uses of
autoImplicitin these files are accidental; situations such as:variablesare in scope, but pasting the lemma in the wrong sectionHaving
set_option autoImplicit falseas the default prevents these types of mistake being made in the 90% of files whereautoImplicits are not used at all, and causes them to be caught by CI during review.I think there were various points during the port where we encouraged porters to delete the
universes u vlines; I think having autoparams for universe variables only would cover a lot of the cases we actually use them, while avoiding any real shortcomings.A Zulip poll (after combining overlapping votes accordingly) was in favor of this change with
5:5:18as theno:dontcare:yesvote ratio.While this PR was being reviewed, a handful of files gained some more likely-accidental autoImplicits. In these places,
set_option autoImplicit truehas been placed locally within a section, rather than at the top of the file.