Skip to content

Rust: expand derive macros#19824

Merged
redsun82 merged 9 commits intomainfrom
redsun82/rust-derive-macro-expansion
Jun 23, 2025
Merged

Rust: expand derive macros#19824
redsun82 merged 9 commits intomainfrom
redsun82/rust-derive-macro-expansion

Conversation

@redsun82
Copy link
Contributor

@redsun82 redsun82 commented Jun 20, 2025

No description provided.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jun 20, 2025
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 20, 2025
View reviewed changes
rust/ql/lib/codeql/rust/elements.qll
import codeql.files.FileSystem
import codeql.rust.elements.Abi
import codeql.rust.elements.Addressable
import codeql.rust.elements.Adt

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.rust.elements.Enum
Loading
.
Redundant import, the module is already imported inside
codeql.rust.elements.Struct
Loading
.
Redundant import, the module is already imported inside
codeql.rust.elements.Union
Loading
.
rust/ql/lib/codeql/rust/elements/Adt.qll
*/

private import internal.AdtImpl
import codeql.rust.elements.Item

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.rust.elements.MacroItems
Loading
.
rust/ql/lib/codeql/rust/elements/Adt.qll

private import internal.AdtImpl
import codeql.rust.elements.Item
import codeql.rust.elements.MacroItems

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.rust.elements.Item
Loading
.
aibaars
aibaars reviewed Jun 20, 2025
View reviewed changes
redsun82
redsun82 commented Jun 20, 2025
View reviewed changes
rust/schema/ast.py

from .prelude import *

class Adt(AstNode, ):
Copy link
Contributor Author

@redsun82 redsun82 Jun 20, 2025

Choose a reason for hiding this comment

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

that's already done in the annotations

codeql/rust/schema/annotations.py

Line 19 in 52c0028

@annotate(Adt, replace_bases={AstNode: Item})

aibaars
aibaars reviewed Jun 20, 2025
View reviewed changes
aibaars
aibaars reviewed Jun 20, 2025
View reviewed changes
rust/schema/ast.py

from .prelude import *

class Adt(AstNode, ):
Copy link
Contributor

@aibaars aibaars Jun 20, 2025

Choose a reason for hiding this comment

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

Thanks! Your are changing things faster than I can review them ;-)

aibaars
aibaars reviewed Jun 20, 2025
View reviewed changes
@redsun82 redsun82 marked this pull request as ready for review June 20, 2025 15:21
@redsun82 redsun82 requested review from a team as code owners June 20, 2025 15:21
aibaars
aibaars previously approved these changes Jun 20, 2025
View reviewed changes
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@aibaars aibaars force-pushed the redsun82/rust-derive-macro-expansion branch from 9f6e446 to f4bdd4d Compare June 20, 2025 17:32
@redsun82
Copy link
Contributor Author

redsun82 commented Jun 23, 2025 •
edited
Loading

@aibaars DCA shows a significant increase of warnings on bevy, zed and diem. As far as I could see, this is not a sign of a new problem, but an existing one which is made worse by just expanding more macros, and surfacing the expansion errors as a warning. So we now get a bit more insight into what's going on there (proc-macro crate build data is missing dylib path for bevy, a more generic failed to build proc-macro in the other two cases). Those are apparently the sources of further errors that we were already seeing.

What do you think, shall we accept that for now, and investigate those three repos later?

@redsun82 redsun82 merged commit 23c449f into main Jun 23, 2025
27 checks passed
@redsun82 redsun82 deleted the redsun82/rust-derive-macro-expansion branch June 23, 2025 07:42
geoffw0
geoffw0 reviewed Jun 23, 2025
View reviewed changes
rust/ql/test/library-tests/type-inference/type-inference.expected
| main.rs:1928:41:1928:59 | ...::Foo {...} | | main.rs:72:5:72:21 | Foo |
| main.rs:1944:5:1944:15 | ...::f(...) | | {EXTERNAL LOCATION} | trait Future |
testFailures
| main.rs:2:5:2:20 | f.write_str(...) | Unexpected result: method=write_str |
Copy link
Contributor

@geoffw0 geoffw0 Jun 23, 2025

Choose a reason for hiding this comment

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

You've allowed some test failures without updating the annotations in the .rs file. This isn't a good state to leave things in as the annotations are now misleading. Is there a plan to fix these?

Copy link
Contributor

@hvitved hvitved Jun 23, 2025

Choose a reason for hiding this comment

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

I have a fix.

Copy link
Contributor

@hvitved hvitved Jun 23, 2025

Choose a reason for hiding this comment

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

#19850

@hvitved hvitved mentioned this pull request Jun 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hvitved hvitved hvitved left review comments

@geoffw0 geoffw0 geoffw0 left review comments

+1 more reviewer

@aibaars aibaars aibaars approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Rust Pull requests that update Rust code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@redsun82 @hvitved @aibaars @geoffw0