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

Note if mismatched types have a similar name #101664

Merged
merged 4 commits into from
Sep 23, 2022
Merged

Note if mismatched types have a similar name #101664

merged 4 commits into from
Sep 23, 2022

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented Sep 10, 2022

If users get a type error between similarly named types, it will point out that these are actually different types, and where they were defined.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 10, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fee1-dead (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2022
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
let found_name = values.found.sort_string(self.tcx);
let expected_name = values.expected.sort_string(self.tcx);

diag.note(format!("{found_name} and {expected_name} have similar names, but are actually distinct types"));
Copy link
Member

Choose a reason for hiding this comment

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

how does this interact with the existing "help: maybe two different crates are being used?" message? IMO we should probably only show one or the other; it would be nice to add a test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shows both; see src/test/ui/type/type-mismatch-same-crate-name. Its output got updated with this PR.

compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
@fee1-dead
Copy link
Member

@rustbot author

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2022

Error: The "Author" shortcut only works on pull requests.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@fee1-dead fee1-dead 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 Sep 11, 2022
@mejrs
Copy link
Contributor Author

mejrs commented Sep 12, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2022
@bors
Copy link
Contributor

bors commented Sep 17, 2022

☔ The latest upstream changes (presumably #101938) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@mejrs
Copy link
Contributor Author

mejrs commented Sep 18, 2022

@rustbot ready

Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

Left a few nits. Should be good to go after reviews are addressed. Sorry for the late review!

Comment on lines 1664 to 1688
let primitive_sym = |kind: &_| match kind {
ty::Bool => Some(sym::bool),
ty::Char => Some(sym::char),
ty::Float(f) => match f {
ty::FloatTy::F32 => Some(sym::f32),
ty::FloatTy::F64 => Some(sym::f64),
},
ty::Int(f) => match f {
ty::IntTy::Isize => Some(sym::isize),
ty::IntTy::I8 => Some(sym::i8),
ty::IntTy::I16 => Some(sym::i16),
ty::IntTy::I32 => Some(sym::i32),
ty::IntTy::I64 => Some(sym::i64),
ty::IntTy::I128 => Some(sym::i128),
},
ty::Uint(f) => match f {
ty::UintTy::Usize => Some(sym::usize),
ty::UintTy::U8 => Some(sym::u8),
ty::UintTy::U16 => Some(sym::u16),
ty::UintTy::U32 => Some(sym::u32),
ty::UintTy::U64 => Some(sym::u64),
ty::UintTy::U128 => Some(sym::u128),
},
_ => None,
};
Copy link
Member

@fee1-dead fee1-dead Sep 18, 2022

Choose a reason for hiding this comment

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

This should be an associated function for TyKind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it can be an associated function on TyKind (or Symbol for that matter) because that leads to cyclic dependencies. But I'm happy to make it a free function/trait somewhere, or an associated function on rustc_middle::ty::Ty

match (fk, ek) {
(
ty::Adt(adt, _),
ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_),
Copy link
Member

Choose a reason for hiding this comment

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

This could use is_primitive instead.

None
}
(
ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_),
Copy link
Member

Choose a reason for hiding this comment

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

same above. I think you can also use an if-let guard to extract the primitive sym in this match statement directly.

Comment on lines 1 to 6
enum Option<T>{
Some(T),
None,
}

pub fn foo() -> Option<u8>{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enum Option<T>{
Some(T),
None,
}
pub fn foo() -> Option<u8>{
enum Option<T> {
Some(T),
None,
}
pub fn foo() -> Option<u8> {

Comment on lines 1 to 18
pub mod blah{
pub mod baz{
pub struct Foo;
}
}

pub mod meh{
pub struct Foo;
}

pub type Foo = blah::baz::Foo;

fn foo() -> Foo {
meh::Foo
//~^ ERROR mismatched types [E0308]
}

fn main(){}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub mod blah{
pub mod baz{
pub struct Foo;
}
}
pub mod meh{
pub struct Foo;
}
pub type Foo = blah::baz::Foo;
fn foo() -> Foo {
meh::Foo
//~^ ERROR mismatched types [E0308]
}
fn main(){}
pub mod blah {
pub mod baz {
pub struct Foo;
}
}
pub mod meh {
pub struct Foo;
}
pub type Foo = blah::baz::Foo;
fn foo() -> Foo {
meh::Foo
//~^ ERROR mismatched types [E0308]
}
fn main() {}

@mejrs
Copy link
Contributor Author

mejrs commented Sep 19, 2022

Thank you for the review. I have:

  • formatted the tests
  • moved primitive_sym to a method on rustc_middle::ty::Ty
  • (not your feedback but) changed the Similar enum to use named fields rather than tuple fields
  • changed the rest of the code to work using the above.

Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 22, 2022

📌 Commit c658660 has been approved by fee1-dead

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 22, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 22, 2022
Note if mismatched types have a similar name

If users get a type error between similarly named types, it will point out that these are actually different types, and where they were defined.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2022
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#100734 (Split out async_fn_in_trait into a separate feature)
 - rust-lang#101664 (Note if mismatched types have a similar name)
 - rust-lang#101815 (Migrated the rustc_passes annotation without effect diagnostic infrastructure)
 - rust-lang#102042 (Distribute rust-docs-json via rustup.)
 - rust-lang#102066 (rustdoc: remove unnecessary `max-width` on headers)
 - rust-lang#102095 (Deduplicate two functions that would soon have been three)
 - rust-lang#102104 (Set 'exec-env:RUST_BACKTRACE=0' in const-eval-select tests)
 - rust-lang#102112 (Allow full relro on powerpc64-unknown-linux-gnu)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c2d2535 into rust-lang:master Sep 23, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 23, 2022
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.

7 participants