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

turn invalid_type_param_default into a FutureReleaseErrorReportInDeps #127655

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 12, 2024

@rust-lang/types I assume the plan is still to disallow this? It has been a future-compat lint for a long time, seems ripe to go for hard error.

However, turns out that outright removing it right now would lead to tons of crater regressions, so for now this PR just makes this future-compat lint show up in cargo's reports, so people are warned when they use a dependency that is affected by this.

Fixes #27336 by removing the feature gate (so there's no way to silence the lint even on nightly)
CC #36887

@rustbot
Copy link
Collaborator

rustbot commented Jul 12, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 12, 2024
@@ -230,6 +230,7 @@ pub(super) fn generics_of(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Generics {
enum Defaults {
Allowed,
// See #36887
#[allow(dead_code)] // FIXME remove if the PR moves forward
FutureCompatDisallowed,
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW this makes the default_type_parameter_fallback feature gate a NOP, since all the feature gate seems to do is silence the future-incompat warning. If the feature gate should be preserved I can rename the enum variant to FeatureGated or so. Otherwise I can also remove the feature gate entirely.

@RalfJung RalfJung changed the title make invalid_type_param_default a hard error WIP: make invalid_type_param_default a hard error Jul 12, 2024
@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jul 12, 2024

⌛ Trying commit 4282f48 with merge 6a57490...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
… r=<try>

WIP: make invalid_type_param_default a hard error

Cc rust-lang#36887
`@rust-lang/types` I assume the plan is still to disallow this? It has been a future-compat lint for a long time, seems ripe to go for hard error. This should be cratered. (But I can also make it `FutureReleaseErrorReportInDeps` first if you prefer.)
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 12, 2024

☀️ Try build successful - checks-actions
Build commit: 6a57490 (6a5749073f2ebf59d7740d30e50a932a85756ee7)

@compiler-errors
Copy link
Member

I'm in favor of this change, and I think we should make this into a hard error (or bump it to deny, if there's significant fallback). Furthermore, I feel like we should go ahead and mark default_type_parameter_fallback feature as removed gate since it's essentially abandoned and needs significant reworking on the types side.

@RalfJung
Copy link
Member Author

@craterbot check

@compiler-errors is there code inside the type/trait system that becomes dead when default_type_parameter_fallback is removed? I won't be able to clean up things that deep in the compiler. Just removing the feature is easy, of course.

@craterbot
Copy link
Collaborator

👌 Experiment pr-127655 created and queued.
🤖 Automatically detected try build 6a57490
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-127655 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@lcnr
Copy link
Contributor

lcnr commented Jul 15, 2024

@compiler-errors is there code inside the type/trait system that becomes dead when default_type_parameter_fallback is removed? I won't be able to clean up things that deep in the compiler. Just removing the feature is easy, of course.

did a quick look through the code, it does not seem like it. just removing the feature should be good enough

@craterbot
Copy link
Collaborator

🎉 Experiment pr-127655 is completed!
📊 591 regressed and 2 fixed (481462 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 15, 2024
@RalfJung
Copy link
Member Author

Ah, it's our old friend typemap... our favorite unmaintained widely-used crate.

@compiler-errors
Copy link
Member

@RalfJung: You should still be able to remove the feature gate and clean up the fallout. Do you still want to do that? Otherwise I can. And I still think we can bump this to deny/FCW/some other lint class.

@RalfJung RalfJung changed the title WIP: make invalid_type_param_default a hard error make invalid_type_param_default a hard error Jul 15, 2024
@RalfJung
Copy link
Member Author

I have made the PR just bump up the lint to something that shows up in future-compat reports.

You should still be able to remove the feature gate and clean up the fallout. Do you still want to do that? Otherwise I can. And I still think we can bump this to deny/FCW/some other lint class.

Ah, because the feature just silences the lint? Sure, can do.

@RalfJung
Copy link
Member Author

Okay, should be done. :)

r? @compiler-errors

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 25, 2024
@rfcbot
Copy link

rfcbot commented Jul 25, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@QuineDot
Copy link

QuineDot commented Aug 3, 2024

I doubt anyone cares enough to sway this FCP, but function type parameter defaults act the same as ADT type parameters defaults as far as I know. They're worse off mainly because you can't name function item types.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 4, 2024
@rfcbot
Copy link

rfcbot commented Aug 4, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@compiler-errors
Copy link
Member

Thanks for the info @QuineDot, but I don't really think users should really be using of default type params on functions anyways. If this is heavily requested, I think we can add this feature gate back with a proper lang team experiment to investigate supporting default params the right way (i.e. in a more ergonomic way).

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2024

📌 Commit 9d9b55c has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 4, 2024
…t, r=compiler-errors

turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`

`@rust-lang/types` I assume the plan is still to disallow this? It has been a future-compat lint for a long time, seems ripe to go for hard error.

However, turns out that outright removing it right now would lead to [tons of crater regressions](rust-lang#127655 (comment)), so for now this PR just makes this future-compat lint show up in cargo's reports, so people are warned when they use a dependency that is affected by this.

Fixes rust-lang#27336 by removing the feature gate (so there's no way to silence the lint even on nightly)
CC rust-lang#36887
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 4, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#127655 (turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`)
 - rust-lang#127974 (force compiling std from source if modified)
 - rust-lang#128026 (std::thread: available_parallelism implementation for vxWorks proposal.)
 - rust-lang#128362 (add test for symbol visibility of `#[naked]` functions)
 - rust-lang#128500 (Add test for updating enum discriminant through pointer)
 - rust-lang#128630 (docs(resolve): more explain about `target`)
 - rust-lang#128638 (run-make: enable msvc for `link-dedup`)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 4, 2024
…t, r=compiler-errors

turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`

``@rust-lang/types`` I assume the plan is still to disallow this? It has been a future-compat lint for a long time, seems ripe to go for hard error.

However, turns out that outright removing it right now would lead to [tons of crater regressions](rust-lang#127655 (comment)), so for now this PR just makes this future-compat lint show up in cargo's reports, so people are warned when they use a dependency that is affected by this.

Fixes rust-lang#27336 by removing the feature gate (so there's no way to silence the lint even on nightly)
CC rust-lang#36887
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 4, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#127655 (turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`)
 - rust-lang#127974 (force compiling std from source if modified)
 - rust-lang#128026 (std::thread: available_parallelism implementation for vxWorks proposal.)
 - rust-lang#128309 (Implement cursors for `BTreeSet`)
 - rust-lang#128500 (Add test for updating enum discriminant through pointer)
 - rust-lang#128630 (docs(resolve): more explain about `target`)
 - rust-lang#128631 (handle crates when they are not specified for std docs)
 - rust-lang#128638 (run-make: enable msvc for `link-dedup`)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 4, 2024
…t, r=compiler-errors

turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`

```@rust-lang/types``` I assume the plan is still to disallow this? It has been a future-compat lint for a long time, seems ripe to go for hard error.

However, turns out that outright removing it right now would lead to [tons of crater regressions](rust-lang#127655 (comment)), so for now this PR just makes this future-compat lint show up in cargo's reports, so people are warned when they use a dependency that is affected by this.

Fixes rust-lang#27336 by removing the feature gate (so there's no way to silence the lint even on nightly)
CC rust-lang#36887
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 4, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#127655 (turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`)
 - rust-lang#127974 (force compiling std from source if modified)
 - rust-lang#128026 (std::thread: available_parallelism implementation for vxWorks proposal.)
 - rust-lang#128309 (Implement cursors for `BTreeSet`)
 - rust-lang#128500 (Add test for updating enum discriminant through pointer)
 - rust-lang#128630 (docs(resolve): more explain about `target`)
 - rust-lang#128638 (run-make: enable msvc for `link-dedup`)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2024
…t, r=compiler-errors

turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`

````@rust-lang/types```` I assume the plan is still to disallow this? It has been a future-compat lint for a long time, seems ripe to go for hard error.

However, turns out that outright removing it right now would lead to [tons of crater regressions](rust-lang#127655 (comment)), so for now this PR just makes this future-compat lint show up in cargo's reports, so people are warned when they use a dependency that is affected by this.

Fixes rust-lang#27336 by removing the feature gate (so there's no way to silence the lint even on nightly)
CC rust-lang#36887
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#127655 (turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`)
 - rust-lang#127907 (built-in derive: remove BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE hack and lint)
 - rust-lang#127974 (force compiling std from source if modified)
 - rust-lang#128309 (Implement cursors for `BTreeSet`)
 - rust-lang#128500 (Add test for updating enum discriminant through pointer)
 - rust-lang#128623 (Do not fire unhandled attribute assertion on multi-segment `AttributeType::Normal` attributes with builtin attribute as first segment)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cc61dc8 into rust-lang:master Aug 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
Rollup merge of rust-lang#127655 - RalfJung:invalid_type_param_default, r=compiler-errors

turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`

`````@rust-lang/types````` I assume the plan is still to disallow this? It has been a future-compat lint for a long time, seems ripe to go for hard error.

However, turns out that outright removing it right now would lead to [tons of crater regressions](rust-lang#127655 (comment)), so for now this PR just makes this future-compat lint show up in cargo's reports, so people are warned when they use a dependency that is affected by this.

Fixes rust-lang#27336 by removing the feature gate (so there's no way to silence the lint even on nightly)
CC rust-lang#36887
@RalfJung RalfJung deleted the invalid_type_param_default branch August 5, 2024 12:00
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for RFC 213: Default Type Parameter Fallback