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

Make generic const type mismatches not hide trait impls from the trait solver #120059

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jan 17, 2024

pulled out of #119895

It does improve diagnostics somewhat, but also causes some extraneous diagnostics in potentially misleading order.

The issue was that a const type mismatch, instead of reporting an error, would silently poison the constant, only for that information to be thrown away and the impl to be treated as "not matching". In #119895 this would cause ICEs as well as errors on impls stating that the impl needs to exist for itself to be valid.

@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2024

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@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 Jan 17, 2024
@compiler-errors
Copy link
Member

Why aren't you happy with this change? It doesn't seem to make diagnostics much worse -- the only material change is that we flip a type error into a coherence overlap error, but both of them are valid errors, so the choice seems a bit arbitrary.

I'm happy to approve this if you want to actually give it a real description and title and stuff. Otherwise, I'd like to hear more about your reservations with this change.

Comment on lines +1 to +5
error: the constant `13` is not of type `u64`
--> $DIR/bad-subst-const-kind.rs:13:24
|
LL | pub fn test() -> [u8; <[u8; 13] as Q>::ASSOC] { todo!() }
| ^^^^^^^^ expected `u64`, found `usize`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the compiler suggests using a u64 as an array length

Copy link
Member

Choose a reason for hiding this comment

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

yeah we have bad spans for this in general

Comment on lines 15 to 20
error[E0308]: mismatched types
--> $DIR/bad-subst-const-kind.rs:8:31
|
LL | impl<const N: u64> Q for [u8; N] {
| ^ expected `usize`, found `u64`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only diagnostic I would expect to see. But I haven't figured out yet how to poison the impl so it doesn't get picked up

Comment on lines +32 to +37
error[E0308]: mismatched types
--> $DIR/type_mismatch.rs:8:31
|
LL | impl<const N: u64> Q for [u8; N] {}
| ^ expected `usize`, found `u64`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is good, I want this error. But now we also got a bunch of "follow-up" errors happening before. Maybe we can move whatever causes this error into a query that is guaranteed to be called before impl overlap checking

@oli-obk oli-obk changed the title WIP: Experimental extraction of a change from a larger PR Make generic const type mismatches not hide trait impls from the trait solver Jan 18, 2024
@oli-obk oli-obk marked this pull request as ready for review January 18, 2024 09:05
@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2024

Type relation code was changed

cc @compiler-errors, @lcnr

@oli-obk oli-obk force-pushed the const_arg_type_mismatch branch from 9265e13 to 2040c1c Compare January 18, 2024 09:54
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.

I'm ok w the state of these errors right now.

r=me unless you want to continue to iterate on this, though I guess you could/should probably file a follow-up diagnostic issue for the really bad case of tests/ui/const-generics/bad-subst-const-kind.rs since I agree that one is particularly confusing.

@compiler-errors compiler-errors 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 Jan 18, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 18, 2024

@bors r=compiler-errors

I have tried a few things, but it's very hard to improve the span here. All we have is the span of the array, and it's a ty::Array with an already evaluated length const.

I'll keep digging

@bors
Copy link
Contributor

bors commented Jan 18, 2024

📌 Commit 2040c1c has been approved by compiler-errors

It is now in the queue for this repository.

@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 Jan 18, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 20, 2024
…=compiler-errors

Make generic const type mismatches not hide trait impls from the trait solver

pulled out of rust-lang#119895

It does improve diagnostics somewhat, but also causes some extraneous diagnostics in potentially misleading order.

The issue was that a const type mismatch, instead of reporting an error, would silently poison the constant, only for that information to be thrown away and the impl to be treated as "not matching". In rust-lang#119895 this would cause ICEs as well as errors on impls stating that the impl needs to exist for itself to be valid.
@compiler-errors
Copy link
Member

@bors r-

#120162 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 20, 2024
@oli-obk oli-obk force-pushed the const_arg_type_mismatch branch from 2040c1c to 9454b51 Compare January 22, 2024 13:23
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 22, 2024

@bors r=compiler-errors

after a rebase over #119869 the missing diagnostic is back 🎉

@bors
Copy link
Contributor

bors commented Jan 22, 2024

📌 Commit 9454b51 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Jan 22, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…=compiler-errors

Make generic const type mismatches not hide trait impls from the trait solver

pulled out of rust-lang#119895

It does improve diagnostics somewhat, but also causes some extraneous diagnostics in potentially misleading order.

The issue was that a const type mismatch, instead of reporting an error, would silently poison the constant, only for that information to be thrown away and the impl to be treated as "not matching". In rust-lang#119895 this would cause ICEs as well as errors on impls stating that the impl needs to exist for itself to be valid.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#117910 (Refactor uses of `objc_msgSend` to no longer have clashing definitions)
 - rust-lang#118639 (Undeprecate lint `unstable_features` and make use of it in the compiler)
 - rust-lang#119801 (Fix deallocation with wrong allocator in (A)Rc::from_box_in)
 - rust-lang#120058 (bootstrap: improvements for compiler builds)
 - rust-lang#120059 (Make generic const type mismatches not hide trait impls from the trait solver)
 - rust-lang#120097 (Report unreachable subpatterns consistently)
 - rust-lang#120137 (Validate AggregateKind types in MIR)
 - rust-lang#120164 (`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`)
 - rust-lang#120181 (Allow any `const` expression blocks in `thread_local!`)
 - rust-lang#120204 (Builtin macros effectively have implicit #[collapse_debuginfo(yes)])
 - rust-lang#120218 (rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg)

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

Rollup of 10 pull requests

Successful merges:

 - rust-lang#117910 (Refactor uses of `objc_msgSend` to no longer have clashing definitions)
 - rust-lang#118639 (Undeprecate lint `unstable_features` and make use of it in the compiler)
 - rust-lang#119801 (Fix deallocation with wrong allocator in (A)Rc::from_box_in)
 - rust-lang#120058 (bootstrap: improvements for compiler builds)
 - rust-lang#120059 (Make generic const type mismatches not hide trait impls from the trait solver)
 - rust-lang#120097 (Report unreachable subpatterns consistently)
 - rust-lang#120137 (Validate AggregateKind types in MIR)
 - rust-lang#120164 (`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`)
 - rust-lang#120181 (Allow any `const` expression blocks in `thread_local!`)
 - rust-lang#120218 (rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d942357 into rust-lang:master Jan 22, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
Rollup merge of rust-lang#120059 - oli-obk:const_arg_type_mismatch, r=compiler-errors

Make generic const type mismatches not hide trait impls from the trait solver

pulled out of rust-lang#119895

It does improve diagnostics somewhat, but also causes some extraneous diagnostics in potentially misleading order.

The issue was that a const type mismatch, instead of reporting an error, would silently poison the constant, only for that information to be thrown away and the impl to be treated as "not matching". In rust-lang#119895 this would cause ICEs as well as errors on impls stating that the impl needs to exist for itself to be valid.
@oli-obk oli-obk deleted the const_arg_type_mismatch branch January 23, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants