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

Add error message for E0532 #3118

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Add error message for E0532 #3118

merged 1 commit into from
Aug 6, 2024

Conversation

liamnaddell
Copy link
Contributor

@liamnaddell liamnaddell commented Aug 4, 2024

I took the error message from rustc, but I really don't like it. Isn't the error message the direct opposite of the problem?

Rustc error message: expected unit struct, unit variant or constant, found tuple variant
I want something like: referenced type is a non-unit enum variant, consider specifying the relevant variant fields

    gcc/rust/ChangeLog:
            * typecheck/rust-hir-type-check-pattern.cc:
            Emit E0532 when trying to reference a Tuple or Struct variant
            using a non Tuple or Struct pattern.

    gcc/testsuite/ChangeLog:
            * rust/compile/issue-2324-1.rs:
            add test for E0532 with tuple enum variant
            * rust/compile/issue-2324-1.rs:
            add test for E0532 with struct enum variant

Fixes #2324 , addresses #2553


Adds error message for E0532

gcc/rust/ChangeLog:
	* typecheck/rust-hir-type-check-pattern.cc:
	Emit E0532 when trying to reference a Tuple or Struct variant
	using a non Tuple or Struct pattern.

gcc/testsuite/ChangeLog:
	* rust/compile/issue-2324-1.rs:
	add test for E0532 with tuple enum variant
	* rust/compile/issue-2324-2.rs:
	add test for E0532 with struct enum variant

Signed-off-by: Liam Naddell <[email protected]>
@liamnaddell liamnaddell marked this pull request as draft August 4, 2024 15:36
@liamnaddell liamnaddell force-pushed the pattern branch 5 times, most recently from 91ee735 to 1231ee6 Compare August 4, 2024 16:15
@liamnaddell liamnaddell marked this pull request as ready for review August 4, 2024 16:15
@liamnaddell liamnaddell force-pushed the pattern branch 2 times, most recently from d378385 to 2fd3eac Compare August 4, 2024 22:47
@P-E-P P-E-P self-requested a review August 6, 2024 14:13
@P-E-P P-E-P requested a review from CohenArthur August 6, 2024 14:25
@P-E-P
Copy link
Member

P-E-P commented Aug 6, 2024

I took the error message from rustc, but I really don't like it

I can see why. Honestly as long as the error code is correct the message doesn't need to match rustc's message exactly. Let's keep it like this, if we want to match rustc's message in the future we won't have to change it, furthermore rustc may get a better error message for this error specifically in the future. Moreover keeping the error message as is will get it in line with the error code https://doc.rust-lang.org/stable/error_codes/E0533.html.

@liamnaddell
Copy link
Contributor Author

I took the error message from rustc, but I really don't like it

I can see why. Honestly as long as the error code is correct the message doesn't need to match rustc's message exactly. Let's keep it like this, if we want to match rustc's message in the future we won't have to change it, furthermore rustc may get a better error message for this error specifically in the future. Moreover keeping the error message as is will get it in line with the error code https://doc.rust-lang.org/stable/error_codes/E0533.html.

I'm inclined to agree, it's probably best to leave it as is

@P-E-P P-E-P added this pull request to the merge queue Aug 6, 2024
Merged via the queue into Rust-GCC:master with commit be1e78b Aug 6, 2024
9 checks passed
@MahadMuhammad
Copy link
Contributor

Thanks @liamnaddell for fixing the issue. I agree to @P-E-P

Honestly as long as the error code is correct the message doesn't need to match rustc's message exactly.

I spent quite a time to making gccrs error message similar to rustc, but sometimes it's just not possible. As long as the error code is correct, the regex pattern in dg-error will pass.

Furthermore, the tool I am working on to convert rustc testcases to DejaGnu format doesn't include error messages, since rustc and gccrs error messages differs alot

Comment on lines +69 to +71
rust_error_at (
pattern.get_final_segment ().get_locus (), ErrorCode::E0532,
"expected unit struct, unit variant or constant, found tuple variant");
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message seems incorrect. found tuple variant should be changed to found %s variant. I will fix this in #3125

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'm happy with this. I only put it in because its what rustc uses

Comment on lines +54 to +55
rust_assert (infered->get_kind () == TyTy::TypeKind::ADT);
TyTy::ADTType *adt = static_cast<TyTy::ADTType *> (infered);
Copy link
Contributor

@tamaroning tamaroning Aug 10, 2024

Choose a reason for hiding this comment

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

Also it is okay that patterns reference non-ADT const items.

fn print_on_failure(state: &State) {
match *state {
State::Succeeded => (),
State::Failed => (), // { dg-error "expected unit struct, unit variant or constant, found tuple variant" }
Copy link
Contributor

Choose a reason for hiding this comment

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

should be found struct variant

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 believe this should say "found unit variant"
Since State::Failed is a unit variant, but we needed the struct variant State::Failed { x:y}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are also right too. I feel a bit weird for this message.
We (and rustc) should rethink about this message :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expected tuple struct or tuple variant, found struct variant [E0532]
4 participants