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

Correctly check never_type feature gating #120552

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

GuillaumeGomez
Copy link
Member

Fixes #120542.

The feature wasn't tested on return type of a generic function type, so it got under the radar in #120316.

r? @compiler-errors

@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 Feb 1, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Can you check if this also works for RPIT? Or dyn Fn() -> !?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Feb 1, 2024

Also, instead of keeping this "in generics" state as part of the visitor, please just modify this line to check for ! manually, and leave a comment saying "in generic args, we don't allow -> !" or something:

https://github.com/rust-lang/rust/pull/120316/files#diff-90854f27d73f89fc2997be31fa36f58cb9af532d1387ac39751134afdd2ff8e2R505

@compiler-errors
Copy link
Member

compiler-errors commented Feb 1, 2024

This will also likely require a beta backport depending on the timing of when this lands. I'll nominate it pre-emptively so we don't forget. This should be backported to 1.77, so if this lands on 1.77 then we're good.

@compiler-errors compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 1, 2024
@GuillaumeGomez
Copy link
Member Author

Also, instead of keeping this "in generics" state as part of the visitor, please just modify this line to check for ! manually, and leave a comment saying "in generic args, we don't allow -> !" or something:

https://github.com/rust-lang/rust/pull/120316/files#diff-90854f27d73f89fc2997be31fa36f58cb9af532d1387ac39751134afdd2ff8e2R505

Except this is the "default" implementation. It should call the visitor method and not the walk_ one. That's what my PR fixed originally. The problem here lies in the implementation of PostExpansionVisitor.

@compiler-errors
Copy link
Member

Yes, I understand clearly what the problem is. There's a discrepancy between what hir::RetTy::Return should do when it's exactly a ! type between fn decl/fn ptr position, and bound position.

I don't understand what point you're trying to make here though -- there's still no reason to introduce is_in_generics here, and the fix can be a lot more localized.

For the record, the approach that you've taken also breaks this code which compiles on stable:

trait X<const N: i32> {}

fn hello<T: X<{ fn hello() -> ! { loop {} } 1 }>>() {}

fn main() {}

@GuillaumeGomez
Copy link
Member Author

Then I misunderstood what you meant. I understood that you said to revert the change on this line and to add a code comment. However, this would not only impact PostExpansionVisitor but also all types implementing this visitor, which seems wrong to me. What were you suggesting instead?

@compiler-errors
Copy link
Member

compiler-errors commented Feb 1, 2024

Actually, that's not the right line that I suggested anyways. Just modify the visit_generic_args impl in the specific feature gate detection walker to check for Parenthesized generics, and manually look for RetTy::Return with a Never type. If you don't want to do this, I can put up a PR myself.

The manual tracking of a boolean in a walker imo is an anti-pattern, because it's very easy to get wrong with nested items and other AST nodes.

@GuillaumeGomez
Copy link
Member Author

No I'm fine with doing it. Just didn't understand what you meant.

@GuillaumeGomez
Copy link
Member Author

Applied your suggestions. Added more ui tests and also the case which should work.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me after applying changes

@@ -362,6 +362,16 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
}
}

fn visit_generic_args(&mut self, args: &'a ast::GenericArgs) {
if let ast::GenericArgs::Parenthesized(generic_args) = args
Copy link
Member

Choose a reason for hiding this comment

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

pls leave a comment explaining why this is being checked here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!


trait X<const N: i32> {}

fn hello<T: X<{ fn hello() -> ! { loop {} } 1 }>>() {}
Copy link
Member

Choose a reason for hiding this comment

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

could you rename this? it's not really that it's in a "const context", it's more about it being in a nested item.

Maybe never-type-in-nested-fn-decl

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@GuillaumeGomez
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Feb 1, 2024

📌 Commit 0f21e45 has been approved by compiler-errors

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 1, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 1, 2024
…gate, r=compiler-errors

Correctly check `never_type` feature gating

Fixes rust-lang#120542.

The feature wasn't tested on return type of a generic function type, so it got under the radar in rust-lang#120316.

r? `@compiler-errors`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 1, 2024
…gate, r=compiler-errors

Correctly check `never_type` feature gating

Fixes rust-lang#120542.

The feature wasn't tested on return type of a generic function type, so it got under the radar in rust-lang#120316.

r? ``@compiler-errors``
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests)
 - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types)
 - rust-lang#120473 (Only suggest removal of `as_*` and `to_` conversion methods on E0308)
 - rust-lang#120520 (Some cleanups around diagnostic levels.)
 - rust-lang#120540 (add test for try-block-in-match-arm)
 - rust-lang#120547 (`#![feature(inline_const_pat)]` is no longer incomplete)
 - rust-lang#120552 (Correctly check `never_type` feature gating)
 - rust-lang#120555 (put pnkfelix (me) back on the review queue.)
 - rust-lang#120556 (Improve the diagnostics for unused generic parameters)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests)
 - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types)
 - rust-lang#120473 (Only suggest removal of `as_*` and `to_` conversion methods on E0308)
 - rust-lang#120520 (Some cleanups around diagnostic levels.)
 - rust-lang#120540 (add test for try-block-in-match-arm)
 - rust-lang#120547 (`#![feature(inline_const_pat)]` is no longer incomplete)
 - rust-lang#120552 (Correctly check `never_type` feature gating)
 - rust-lang#120555 (put pnkfelix (me) back on the review queue.)
 - rust-lang#120556 (Improve the diagnostics for unused generic parameters)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests)
 - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types)
 - rust-lang#120473 (Only suggest removal of `as_*` and `to_` conversion methods on E0308)
 - rust-lang#120540 (add test for try-block-in-match-arm)
 - rust-lang#120547 (`#![feature(inline_const_pat)]` is no longer incomplete)
 - rust-lang#120552 (Correctly check `never_type` feature gating)
 - rust-lang#120555 (put pnkfelix (me) back on the review queue.)
 - rust-lang#120556 (Improve the diagnostics for unused generic parameters)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c2ad283 into rust-lang:master Feb 5, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
Rollup merge of rust-lang#120552 - GuillaumeGomez:never-type-feature-gate, r=compiler-errors

Correctly check `never_type` feature gating

Fixes rust-lang#120542.

The feature wasn't tested on return type of a generic function type, so it got under the radar in rust-lang#120316.

r? ```@compiler-errors```
@GuillaumeGomez GuillaumeGomez deleted the never-type-feature-gate branch February 5, 2024 10:15
@apiraino
Copy link
Contributor

apiraino commented Feb 8, 2024

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 8, 2024
@cuviper cuviper mentioned this pull request Feb 14, 2024
@cuviper cuviper modified the milestones: 1.78.0, 1.77.0 Feb 14, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2024
[beta] backports

- Correct paths for hexagon-unknown-none-elf platform doc rust-lang#120533
- CI: Use ninja on apple builders rust-lang#120543
- Correctly check `never_type` feature gating rust-lang#120552
- Revert unsound libcore changes of 119911 rust-lang#120562

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2024
[beta] backports

- Correct paths for hexagon-unknown-none-elf platform doc rust-lang#120533
- CI: Use ninja on apple builders rust-lang#120543
- Correctly check `never_type` feature gating rust-lang#120552
- Revert unsound libcore changes of 119911 rust-lang#120562

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2024
[beta] backports

- Correct paths for hexagon-unknown-none-elf platform doc rust-lang#120533
- CI: Use ninja on apple builders rust-lang#120543
- Correctly check `never_type` feature gating rust-lang#120552
- Revert unsound libcore changes of 119911 rust-lang#120562
- Downgrade xcode rust-lang#120914

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 16, 2024
[beta] backports

- Correct paths for hexagon-unknown-none-elf platform doc rust-lang#120533
- CI: Use ninja on apple builders rust-lang#120543
- Correctly check `never_type` feature gating rust-lang#120552
- Revert unsound libcore changes of 119911 rust-lang#120562
- Downgrade xcode rust-lang#120914
- Update jobserver-rs to 0.1.28 rust-lang#120846
- [beta] Update LLVM submodule rust-lang#121132

r? cuviper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No feature gate error for -> ! used in a trait bound
6 participants