Skip to content

Conversation

@Pasta-coder
Copy link
Contributor

@Pasta-coder Pasta-coder commented Jan 6, 2026

The DeriveVisitor previously dispatched calls to specific handlers (like DeriveClone) without validating the input item type. This caused internal compiler errors (ICEs) when users attempted to derive traits on invalid items like functions or traits, as the handlers assumed the input was a struct, enum, or union.

This patch adds a validation predicate to the DeriveVisitor to reject invalid items gracefully with a diagnostic error before dispatching.

Fixes #3875

gcc/rust/ChangeLog:

* expand/rust-derive.h: Add validate_item prototype.
* expand/rust-derive.cc (DeriveVisitor::derive): Use visitor validation.
(DeriveVisitor::validate_item): New function to check item kind.

gcc/testsuite/ChangeLog:

* rust/compile/derive-on-invalid-item.rs: New test.

@dkm
Copy link
Member

dkm commented Jan 6, 2026

Hello, thanks for your contribution!

Small nitpick on the Changelog part...

* expand/rust-derive.h: Add validate_item prototype.

Does not target a specific "item". You could be more precise using:

* expand/rust-derive.h (DeriveVisitor::validate_item): New.

@Pasta-coder Pasta-coder force-pushed the issue-3875 branch 3 times, most recently from ff0806a to 9e84d88 Compare January 6, 2026 16:06
@Pasta-coder
Copy link
Contributor Author

Pasta-coder commented Jan 6, 2026

@dkm hello,

commit message regarding changelog changed .

thanks

@Pasta-coder Pasta-coder force-pushed the issue-3875 branch 5 times, most recently from 11bd185 to 475edf5 Compare January 7, 2026 04:57
@Pasta-coder
Copy link
Contributor Author

I realized there might be a potential false positive with Tuple Structs (e.g., struct Foo(i32);).

If AST::Item::Kind differentiates between named Structs and TupleStructs, my current whitelist might incorrectly reject valid tuple structs.

I am going to verify this locally by adding a test case for a tuple struct. If it fails, I will update validate_item to explicitly include Kind::TupleStruct and push the fix along with the test.

@dkm
Copy link
Member

dkm commented Jan 8, 2026

@dkm hello,

commit message regarding changelog changed .

thanks

Thanks!

@Pasta-coder Pasta-coder force-pushed the issue-3875 branch 2 times, most recently from 9cee832 to eb4177c Compare January 9, 2026 11:33
@Pasta-coder
Copy link
Contributor Author

Pasta-coder commented Jan 9, 2026

@dkm hello,

I reverted the extra Kind check since TupleStruct falls under Kind::Struct. I kept the test case to verify that tuple structs are correctly accepted.

Open for review .

thankyou

@Pasta-coder
Copy link
Contributor Author

@P-E-P please review .

thankyou .

@Pasta-coder
Copy link
Contributor Author

@CohenArthur please review .

thankyou .

Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

I don't really get what this PR brings on the table. Sure there is a new fancy helper function, but it has a side effect, we're emitting an error. Moving the error out of that helper means it becomes so short a function may not even be worth it anymore.

The check was already there so we didn't get anything from that.

We do get a new test which is a bit more complete compared to rust/compile/issue-3971.rs.

Am I missing something ?

@Pasta-coder
Copy link
Contributor Author

introducing helper function does have a side effect .

why i did what i did ? the ICE was solved in #4211 but issue #3875 was still open with comments suggesting a helper function , also test was missing TupleStructs check . so , i opened a pr .

  1. should i remove the helper function or return bool from helper and emit error from caller ? (do you prefer such helpers?)
  2. or should this pr be solely for adding a new test ?

thankyou

@P-E-P
Copy link
Member

P-E-P commented Jan 26, 2026

introducing helper function does have a side effect .

why i did what i did ? the ICE was solved in #4211 but issue #3875 was still open with comments suggesting a helper function , also test was missing TupleStructs check . so , i opened a pr .

1. should i remove the helper function or return `bool` from helper and emit error from caller ? (do you prefer such helpers?)

2. or should this pr be solely for adding a new test ?

thankyou

I think we should definitely keep the test, but I wouldn't keep the rest of the changes.

@Pasta-coder Pasta-coder force-pushed the issue-3875 branch 3 times, most recently from f4ee91d to 207332f Compare January 26, 2026 22:12
@Pasta-coder
Copy link
Contributor Author

@P-E-P no this pr only adds the test .

thankyou.

// { dg-options "-w" }

#[derive(Clone)] // { dg-error "derive may only be applied to structs, enums and unions" }
fn foo() {} // { dg-excess-errors "" }
Copy link
Member

@P-E-P P-E-P Jan 27, 2026

Choose a reason for hiding this comment

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

I wonder, maybe you could remove those dg-excess-errors if you declare Clone. Have you tried putting those traits ?

Suggested change
fn foo() {} // { dg-excess-errors "" }
#![feature(lang_items)]
fn foo() {}
#[lang = "sized"]
pub trait Sized {}
#[lang = "clone"]
pub trait Clone: Sized {}

Excess errors should usually be avoided as they'll catch any errors, and this means that if the compiler's behavior change in the future they won't catch an error change and things may break silently.

@Pasta-coder Pasta-coder force-pushed the issue-3875 branch 4 times, most recently from a3e1065 to ea8c59e Compare January 27, 2026 18:30
This patch adds a regression test ensuring that deriving on invalid items
(such as functions or traits) triggers a diagnostic error instead of an
Internal Compiler Error (ICE). It also verifies that Tuple Structs are
accepted as valid targets for derivation.

Fixes Rust-GCC#3875

gcc/testsuite/ChangeLog:

	* rust/compile/issue-3875.rs: New test.

Signed-off-by: Jayant Chauhan <0001jayant@gmail.com>
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.

ICE in go, at rust/expand/rust-derive-clone.cc:119

3 participants